Re: [PULL 19/23] util/qemu-sockets: Refactor success and failure paths in inet_listen_saddr()

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Jul 10, 2025 at 01:17:40PM +0100, Peter Maydell wrote:
> On Thu, 22 May 2025 at 11:33, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > From: Juraj Marcin <jmarcin@redhat.com>
> >
> > 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 <jmarcin@redhat.com>
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  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 :|