On 2025-07-10 13:17, Peter Maydell wrote:
> On Thu, 22 May 2025 at 11:33, Daniel P. Berrangé <berra...@redhat.com> wrote:
> >
> > From: Juraj Marcin <jmar...@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 <jmar...@redhat.com>
> > Reviewed-by: Daniel P. Berrangé <berra...@redhat.com>
> > Signed-off-by: Daniel P. Berrangé <berra...@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 ?
Hi Peter!
The intention was to not leave an invalid socket number in the variable
after it's closed, similarly how it was done before refactoring:
https://gitlab.com/qemu-project/qemu/-/blob/b8b5278aca78be4a1c2e7cbb11c6be176f63706d/util/qemu-sockets.c#L346
I haven't noticed it's technically not needed anymore; unless there is
something added in-between in the future. I can send a patch that
removes it if necessary.
Best regards
Juraj Marcin
>
> thanks
> -- PMM
>
