On Thu, 10 Jul 2025 at 13:56, Daniel P. Berrangé <[email protected]> wrote: > > On Thu, Jul 10, 2025 at 01:17:40PM +0100, Peter Maydell wrote: > > On Thu, 22 May 2025 at 11:33, Daniel P. Berrangé <[email protected]> > > wrote: > > > > > > From: Juraj Marcin <[email protected]> > > > > > > To get a listening socket, we need to first create a socket, try binding > > > it to a certain port, and lastly starting listening to it. Each of these > > > operations can fail due to various reasons, one of them being that the > > > requested address/port is already in use. In such case, the function > > > tries the same process with a new port number. > > > > > > This patch refactors the port number loop, so the success path is no > > > longer buried inside the 'if' statements in the middle of the loop. Now, > > > the success path is not nested and ends at the end of the iteration > > > after successful socket creation, binding, and listening. In case any of > > > the operations fails, it either continues to the next iteration (and the > > > next port) or jumps out of the loop to handle the error and exits the > > > function. > > > > > > Signed-off-by: Juraj Marcin <[email protected]> > > > Reviewed-by: Daniel P. Berrangé <[email protected]> > > > Signed-off-by: Daniel P. Berrangé <[email protected]> > > > --- > > > util/qemu-sockets.c | 51 ++++++++++++++++++++++++--------------------- > > > 1 file changed, 27 insertions(+), 24 deletions(-) > > > > > > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c > > > index 4a878e0527..329fdbfd97 100644 > > > --- a/util/qemu-sockets.c > > > +++ b/util/qemu-sockets.c > > > > > > Hi; Coverity complains about this code (CID 1610306): > > > > > @@ -303,11 +303,20 @@ static int inet_listen_saddr(InetSocketAddress > > > *saddr, > > > port_min = inet_getport(e); > > > port_max = saddr->has_to ? saddr->to + port_offset : port_min; > > > for (p = port_min; p <= port_max; p++) { > > > + if (slisten >= 0) { > > > + /* > > > + * We have a socket we tried with the previous port. It > > > cannot > > > + * be rebound, we need to close it and create a new one. > > > + */ > > > + close(slisten); > > > + slisten = -1; > > > > Here we set slisten to -1 ... > > > > > + } > > > inet_setport(e, p); > > > > ...but then two lines later we unconditionally set slisten to > > something else, so the -1 assignment is overwritten without being > > used. > > > > > slisten = create_fast_reuse_socket(e); > > > > What was the intention here ? > > The overwriting is correct and intentional - it is best practice > to never leave a variable initialized to an FD number that is now > invalid. IMHO that best practice applies, even if we are going > to re-initialize the variable a short while later, because this > mitigates risk from later code refactoring. > > TL;DR: coverity is correctly identifying a redundant assignment, > but the assignment is none the less justified.
OK; I will mark this as a false-positive. -- PMM
