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
@@ -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;
+ }
inet_setport(e, p);
slisten = create_fast_reuse_socket(e);
if (slisten < 0) {
- /* First time we expect we might fail to create the socket
+ /*
+ * First time we expect we might fail to create the socket
* eg if 'e' has AF_INET6 but ipv6 kmod is not loaded.
* Later iterations should always succeed if first iteration
* worked though, so treat that as fatal.
@@ -317,40 +326,38 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
} else {
error_setg_errno(errp, errno,
"Failed to recreate failed listening socket");
- goto listen_failed;
+ goto fail;
}
}
socket_created = true;
rc = try_bind(slisten, saddr, e);
if (rc < 0) {
- if (errno != EADDRINUSE) {
- error_setg_errno(errp, errno, "Failed to bind socket");
- goto listen_failed;
- }
- } else {
- if (!listen(slisten, num)) {
- goto listen_ok;
+ if (errno == EADDRINUSE) {
+ /* This port is already used, try the next one */
+ continue;
}
- if (errno != EADDRINUSE) {
- error_setg_errno(errp, errno, "Failed to listen on socket");
- goto listen_failed;
+ error_setg_errno(errp, errno, "Failed to bind socket");
+ goto fail;
+ }
+ if (listen(slisten, num)) {
+ if (errno == EADDRINUSE) {
+ /* This port is already used, try the next one */
+ continue;
}
+ error_setg_errno(errp, errno, "Failed to listen on socket");
+ goto fail;
}
- /* Someone else managed to bind to the same port and beat us
- * to listen on it! Socket semantics does not allow us to
- * recover from this situation, so we need to recreate the
- * socket to allow bind attempts for subsequent ports:
- */
- close(slisten);
- slisten = -1;
+ /* We have a listening socket */
+ freeaddrinfo(res);
+ return slisten;
}
}
error_setg_errno(errp, errno,
socket_created ?
"Failed to find an available port" :
"Failed to create a socket");
-listen_failed:
+fail:
saved_errno = errno;
if (slisten >= 0) {
close(slisten);
@@ -358,10 +365,6 @@ listen_failed:
freeaddrinfo(res);
errno = saved_errno;
return -1;
-
-listen_ok:
- freeaddrinfo(res);
- return slisten;
}
#ifdef _WIN32
--
2.49.0
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 ?
thanks
-- PMM
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 :|
On Thu, 10 Jul 2025 at 13:56, Daniel P. Berrangé <berrange@redhat.com> 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é <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.
OK; I will mark this as a false-positive.
-- PMM
On 2025-07-10 13:17, 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 ?
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
>
© 2016 - 2025 Red Hat, Inc.