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. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
