| Message ID | 20210310173004.420190-1-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | sockets: update SOCKET_ADDRESS_TYPE_FD listen(2) backlog | expand |
On Wed, Mar 10, 2021 at 05:30:04PM +0000, Stefan Hajnoczi wrote: > socket_get_fd() fails with the error "socket_get_fd: too many > connections" if the given listen backlog value is not 1. > > Not all callers set the backlog to 1. For example, commit > 582d4210eb2f2ab5baac328fe4b479cd86da1647 ("qemu-nbd: Use SOMAXCONN for > socket listen() backlog") uses SOMAXCONN. This will always fail with in > socket_get_fd(). > > This patch calls listen(2) on the fd to update the backlog value. The > socket may already be in the listen state. I have tested that this works > on Linux 5.10 and macOS Catalina. > > As a bonus this allows us to detect when the fd cannot listen. Now we'll > be able to catch unbound or connected fds in socket_listen(). > > Drop the num argument from socket_get_fd() since this function is also > called by socket_connect() where a listen backlog value does not make > sense. > > Fixes: e5b6353cf25c99c3f08bf51e29933352f7140e8f ("socket: Add backlog parameter to socket_listen") > Reported-by: Richard W.M. Jones <rjones@redhat.com> > Cc: Juan Quintela <quintela@redhat.com> > Cc: Eric Blake <eblake@redhat.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> I can confirm it fixes the problem found by the libnbd interop test, so: Tested-by: Richard W.M. Jones <rjones@redhat.com> Rich. > --- > util/qemu-sockets.c | 29 ++++++++++++++++++++++------- > 1 file changed, 22 insertions(+), 7 deletions(-) > > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c > index 8af0278f15..2463c49773 100644 > --- a/util/qemu-sockets.c > +++ b/util/qemu-sockets.c > @@ -1116,14 +1116,10 @@ fail: > return NULL; > } > > -static int socket_get_fd(const char *fdstr, int num, Error **errp) > +static int socket_get_fd(const char *fdstr, Error **errp) > { > Monitor *cur_mon = monitor_cur(); > int fd; > - if (num != 1) { > - error_setg_errno(errp, EINVAL, "socket_get_fd: too many connections"); > - return -1; > - } > if (cur_mon) { > fd = monitor_get_fd(cur_mon, fdstr, errp); > if (fd < 0) { > @@ -1159,7 +1155,7 @@ int socket_connect(SocketAddress *addr, Error **errp) > break; > > case SOCKET_ADDRESS_TYPE_FD: > - fd = socket_get_fd(addr->u.fd.str, 1, errp); > + fd = socket_get_fd(addr->u.fd.str, errp); > break; > > case SOCKET_ADDRESS_TYPE_VSOCK: > @@ -1187,7 +1183,26 @@ int socket_listen(SocketAddress *addr, int num, Error **errp) > break; > > case SOCKET_ADDRESS_TYPE_FD: > - fd = socket_get_fd(addr->u.fd.str, num, errp); > + fd = socket_get_fd(addr->u.fd.str, errp); > + if (fd < 0) { > + return -1; > + } > + > + /* > + * If the socket is not yet in the listen state, then transition it to > + * the listen state now. > + * > + * If it's already listening then this updates the backlog value as > + * requested. > + * > + * If this socket cannot listen because it's already in another state > + * (e.g. unbound or connected) then we'll catch the error here. > + */ > + if (listen(fd, num) != 0) { > + error_setg_errno(errp, errno, "Failed to listen on fd socket"); > + closesocket(fd); > + return -1; > + } > break; > > case SOCKET_ADDRESS_TYPE_VSOCK: > -- > 2.29.2 >
On 3/10/21 11:30 AM, Stefan Hajnoczi wrote: > socket_get_fd() fails with the error "socket_get_fd: too many > connections" if the given listen backlog value is not 1. > > Not all callers set the backlog to 1. For example, commit > 582d4210eb2f2ab5baac328fe4b479cd86da1647 ("qemu-nbd: Use SOMAXCONN for > socket listen() backlog") uses SOMAXCONN. This will always fail with in > socket_get_fd(). > > This patch calls listen(2) on the fd to update the backlog value. The > socket may already be in the listen state. I have tested that this works > on Linux 5.10 and macOS Catalina. > > As a bonus this allows us to detect when the fd cannot listen. Now we'll > be able to catch unbound or connected fds in socket_listen(). > > Drop the num argument from socket_get_fd() since this function is also > called by socket_connect() where a listen backlog value does not make > sense. > > Fixes: e5b6353cf25c99c3f08bf51e29933352f7140e8f ("socket: Add backlog parameter to socket_listen") > Reported-by: Richard W.M. Jones <rjones@redhat.com> > Cc: Juan Quintela <quintela@redhat.com> > Cc: Eric Blake <eblake@redhat.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > util/qemu-sockets.c | 29 ++++++++++++++++++++++------- > 1 file changed, 22 insertions(+), 7 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com>
On Wed, Mar 10, 2021 at 05:30:04PM +0000, Stefan Hajnoczi wrote: >socket_get_fd() fails with the error "socket_get_fd: too many >connections" if the given listen backlog value is not 1. > >Not all callers set the backlog to 1. For example, commit >582d4210eb2f2ab5baac328fe4b479cd86da1647 ("qemu-nbd: Use SOMAXCONN for >socket listen() backlog") uses SOMAXCONN. This will always fail with in >socket_get_fd(). > >This patch calls listen(2) on the fd to update the backlog value. The >socket may already be in the listen state. I have tested that this works >on Linux 5.10 and macOS Catalina. Confirmed also from the listen(2) man page of FreeBSD: "A subsequent listen() system call on the listening socket allows the caller to change the maximum queue length using a new backlog argument." > >As a bonus this allows us to detect when the fd cannot listen. Now we'll >be able to catch unbound or connected fds in socket_listen(). > >Drop the num argument from socket_get_fd() since this function is also >called by socket_connect() where a listen backlog value does not make >sense. > >Fixes: e5b6353cf25c99c3f08bf51e29933352f7140e8f ("socket: Add backlog parameter to socket_listen") >Reported-by: Richard W.M. Jones <rjones@redhat.com> >Cc: Juan Quintela <quintela@redhat.com> >Cc: Eric Blake <eblake@redhat.com> >Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> >--- > util/qemu-sockets.c | 29 ++++++++++++++++++++++------- > 1 file changed, 22 insertions(+), 7 deletions(-) Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> Thanks, Stefano
On Wed, Mar 10, 2021 at 05:30:04PM +0000, Stefan Hajnoczi wrote: > socket_get_fd() fails with the error "socket_get_fd: too many > connections" if the given listen backlog value is not 1. > > Not all callers set the backlog to 1. For example, commit > 582d4210eb2f2ab5baac328fe4b479cd86da1647 ("qemu-nbd: Use SOMAXCONN for > socket listen() backlog") uses SOMAXCONN. This will always fail with in > socket_get_fd(). > > This patch calls listen(2) on the fd to update the backlog value. The > socket may already be in the listen state. I have tested that this works > on Linux 5.10 and macOS Catalina. > > As a bonus this allows us to detect when the fd cannot listen. Now we'll > be able to catch unbound or connected fds in socket_listen(). > > Drop the num argument from socket_get_fd() since this function is also > called by socket_connect() where a listen backlog value does not make > sense. > > Fixes: e5b6353cf25c99c3f08bf51e29933352f7140e8f ("socket: Add backlog parameter to socket_listen") > Reported-by: Richard W.M. Jones <rjones@redhat.com> > Cc: Juan Quintela <quintela@redhat.com> > Cc: Eric Blake <eblake@redhat.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > util/qemu-sockets.c | 29 ++++++++++++++++++++++------- > 1 file changed, 22 insertions(+), 7 deletions(-) Dan and Gerd: Can this go via one of your trees? Thanks, Stefan > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c > index 8af0278f15..2463c49773 100644 > --- a/util/qemu-sockets.c > +++ b/util/qemu-sockets.c > @@ -1116,14 +1116,10 @@ fail: > return NULL; > } > > -static int socket_get_fd(const char *fdstr, int num, Error **errp) > +static int socket_get_fd(const char *fdstr, Error **errp) > { > Monitor *cur_mon = monitor_cur(); > int fd; > - if (num != 1) { > - error_setg_errno(errp, EINVAL, "socket_get_fd: too many connections"); > - return -1; > - } > if (cur_mon) { > fd = monitor_get_fd(cur_mon, fdstr, errp); > if (fd < 0) { > @@ -1159,7 +1155,7 @@ int socket_connect(SocketAddress *addr, Error **errp) > break; > > case SOCKET_ADDRESS_TYPE_FD: > - fd = socket_get_fd(addr->u.fd.str, 1, errp); > + fd = socket_get_fd(addr->u.fd.str, errp); > break; > > case SOCKET_ADDRESS_TYPE_VSOCK: > @@ -1187,7 +1183,26 @@ int socket_listen(SocketAddress *addr, int num, Error **errp) > break; > > case SOCKET_ADDRESS_TYPE_FD: > - fd = socket_get_fd(addr->u.fd.str, num, errp); > + fd = socket_get_fd(addr->u.fd.str, errp); > + if (fd < 0) { > + return -1; > + } > + > + /* > + * If the socket is not yet in the listen state, then transition it to > + * the listen state now. > + * > + * If it's already listening then this updates the backlog value as > + * requested. > + * > + * If this socket cannot listen because it's already in another state > + * (e.g. unbound or connected) then we'll catch the error here. > + */ > + if (listen(fd, num) != 0) { > + error_setg_errno(errp, errno, "Failed to listen on fd socket"); > + closesocket(fd); > + return -1; > + } > break; > > case SOCKET_ADDRESS_TYPE_VSOCK: > -- > 2.29.2 >
On 3/16/21 4:10 AM, Stefan Hajnoczi wrote: > On Wed, Mar 10, 2021 at 05:30:04PM +0000, Stefan Hajnoczi wrote: >> socket_get_fd() fails with the error "socket_get_fd: too many >> connections" if the given listen backlog value is not 1. >> >> Not all callers set the backlog to 1. For example, commit >> 582d4210eb2f2ab5baac328fe4b479cd86da1647 ("qemu-nbd: Use SOMAXCONN for >> socket listen() backlog") uses SOMAXCONN. This will always fail with in >> socket_get_fd(). >> >> This patch calls listen(2) on the fd to update the backlog value. The >> socket may already be in the listen state. I have tested that this works >> on Linux 5.10 and macOS Catalina. >> >> As a bonus this allows us to detect when the fd cannot listen. Now we'll >> be able to catch unbound or connected fds in socket_listen(). >> >> Drop the num argument from socket_get_fd() since this function is also >> called by socket_connect() where a listen backlog value does not make >> sense. >> >> Fixes: e5b6353cf25c99c3f08bf51e29933352f7140e8f ("socket: Add backlog parameter to socket_listen") >> Reported-by: Richard W.M. Jones <rjones@redhat.com> >> Cc: Juan Quintela <quintela@redhat.com> >> Cc: Eric Blake <eblake@redhat.com> >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> >> --- >> util/qemu-sockets.c | 29 ++++++++++++++++++++++------- >> 1 file changed, 22 insertions(+), 7 deletions(-) > > Dan and Gerd: Can this go via one of your trees? > As it showed up as a regression in qemu-nbd, I can also consider queuing it in my NBD tree. However, I claim it counts as a bug fix, so it is fine for -rc1 even if it misses soft freeze. I'm fine whichever maintainer takes this, although I've now flagged it to go through an NBD pull request if it doesn't land elsewhere sooner.
On Tue, Mar 16, 2021 at 08:35:14AM -0500, Eric Blake wrote: > On 3/16/21 4:10 AM, Stefan Hajnoczi wrote: > > On Wed, Mar 10, 2021 at 05:30:04PM +0000, Stefan Hajnoczi wrote: > >> socket_get_fd() fails with the error "socket_get_fd: too many > >> connections" if the given listen backlog value is not 1. > >> > >> Not all callers set the backlog to 1. For example, commit > >> 582d4210eb2f2ab5baac328fe4b479cd86da1647 ("qemu-nbd: Use SOMAXCONN for > >> socket listen() backlog") uses SOMAXCONN. This will always fail with in > >> socket_get_fd(). > >> > >> This patch calls listen(2) on the fd to update the backlog value. The > >> socket may already be in the listen state. I have tested that this works > >> on Linux 5.10 and macOS Catalina. > >> > >> As a bonus this allows us to detect when the fd cannot listen. Now we'll > >> be able to catch unbound or connected fds in socket_listen(). > >> > >> Drop the num argument from socket_get_fd() since this function is also > >> called by socket_connect() where a listen backlog value does not make > >> sense. > >> > >> Fixes: e5b6353cf25c99c3f08bf51e29933352f7140e8f ("socket: Add backlog parameter to socket_listen") > >> Reported-by: Richard W.M. Jones <rjones@redhat.com> > >> Cc: Juan Quintela <quintela@redhat.com> > >> Cc: Eric Blake <eblake@redhat.com> > >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > >> --- > >> util/qemu-sockets.c | 29 ++++++++++++++++++++++------- > >> 1 file changed, 22 insertions(+), 7 deletions(-) > > > > Dan and Gerd: Can this go via one of your trees? > > > > As it showed up as a regression in qemu-nbd, I can also consider queuing > it in my NBD tree. However, I claim it counts as a bug fix, so it is > fine for -rc1 even if it misses soft freeze. > > I'm fine whichever maintainer takes this, although I've now flagged it > to go through an NBD pull request if it doesn't land elsewhere sooner. Thank you! Stefan
On Tue, Mar 16, 2021 at 08:35:14AM -0500, Eric Blake wrote: > On 3/16/21 4:10 AM, Stefan Hajnoczi wrote: > > On Wed, Mar 10, 2021 at 05:30:04PM +0000, Stefan Hajnoczi wrote: > >> socket_get_fd() fails with the error "socket_get_fd: too many > >> connections" if the given listen backlog value is not 1. > >> > >> Not all callers set the backlog to 1. For example, commit > >> 582d4210eb2f2ab5baac328fe4b479cd86da1647 ("qemu-nbd: Use SOMAXCONN for > >> socket listen() backlog") uses SOMAXCONN. This will always fail with in > >> socket_get_fd(). > >> > >> This patch calls listen(2) on the fd to update the backlog value. The > >> socket may already be in the listen state. I have tested that this works > >> on Linux 5.10 and macOS Catalina. > >> > >> As a bonus this allows us to detect when the fd cannot listen. Now we'll > >> be able to catch unbound or connected fds in socket_listen(). > >> > >> Drop the num argument from socket_get_fd() since this function is also > >> called by socket_connect() where a listen backlog value does not make > >> sense. > >> > >> Fixes: e5b6353cf25c99c3f08bf51e29933352f7140e8f ("socket: Add backlog parameter to socket_listen") > >> Reported-by: Richard W.M. Jones <rjones@redhat.com> > >> Cc: Juan Quintela <quintela@redhat.com> > >> Cc: Eric Blake <eblake@redhat.com> > >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > >> --- > >> util/qemu-sockets.c | 29 ++++++++++++++++++++++------- > >> 1 file changed, 22 insertions(+), 7 deletions(-) > > > > Dan and Gerd: Can this go via one of your trees? > > > > As it showed up as a regression in qemu-nbd, I can also consider queuing > it in my NBD tree. However, I claim it counts as a bug fix, so it is > fine for -rc1 even if it misses soft freeze. > > I'm fine whichever maintainer takes this, although I've now flagged it > to go through an NBD pull request if it doesn't land elsewhere sooner. Hi Eric, Is this patch going into QEMU 6.0? Thanks, Stefan
On Tue, Mar 16, 2021 at 08:35:14AM -0500, Eric Blake wrote: > On 3/16/21 4:10 AM, Stefan Hajnoczi wrote: > > On Wed, Mar 10, 2021 at 05:30:04PM +0000, Stefan Hajnoczi wrote: > >> socket_get_fd() fails with the error "socket_get_fd: too many > >> connections" if the given listen backlog value is not 1. > >> > >> Not all callers set the backlog to 1. For example, commit > >> 582d4210eb2f2ab5baac328fe4b479cd86da1647 ("qemu-nbd: Use SOMAXCONN for > >> socket listen() backlog") uses SOMAXCONN. This will always fail with in > >> socket_get_fd(). > >> > >> This patch calls listen(2) on the fd to update the backlog value. The > >> socket may already be in the listen state. I have tested that this works > >> on Linux 5.10 and macOS Catalina. > >> > >> As a bonus this allows us to detect when the fd cannot listen. Now we'll > >> be able to catch unbound or connected fds in socket_listen(). > >> > >> Drop the num argument from socket_get_fd() since this function is also > >> called by socket_connect() where a listen backlog value does not make > >> sense. > >> > >> Fixes: e5b6353cf25c99c3f08bf51e29933352f7140e8f ("socket: Add backlog parameter to socket_listen") > >> Reported-by: Richard W.M. Jones <rjones@redhat.com> > >> Cc: Juan Quintela <quintela@redhat.com> > >> Cc: Eric Blake <eblake@redhat.com> > >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > >> --- > >> util/qemu-sockets.c | 29 ++++++++++++++++++++++------- > >> 1 file changed, 22 insertions(+), 7 deletions(-) > > > > Dan and Gerd: Can this go via one of your trees? > > > > As it showed up as a regression in qemu-nbd, I can also consider queuing > it in my NBD tree. However, I claim it counts as a bug fix, so it is > fine for -rc1 even if it misses soft freeze. > > I'm fine whichever maintainer takes this, although I've now flagged it > to go through an NBD pull request if it doesn't land elsewhere sooner. Ping? I didn't see this land in qemu.git. Thanks, Stefan
On Tue, May 11, 2021 at 09:23:10AM +0100, Stefan Hajnoczi wrote: > On Tue, Mar 16, 2021 at 08:35:14AM -0500, Eric Blake wrote: > > On 3/16/21 4:10 AM, Stefan Hajnoczi wrote: > > > On Wed, Mar 10, 2021 at 05:30:04PM +0000, Stefan Hajnoczi wrote: > > >> socket_get_fd() fails with the error "socket_get_fd: too many > > >> connections" if the given listen backlog value is not 1. > > >> > > >> Not all callers set the backlog to 1. For example, commit > > >> 582d4210eb2f2ab5baac328fe4b479cd86da1647 ("qemu-nbd: Use SOMAXCONN for > > >> socket listen() backlog") uses SOMAXCONN. This will always fail with in > > >> socket_get_fd(). > > >> > > >> This patch calls listen(2) on the fd to update the backlog value. The > > >> socket may already be in the listen state. I have tested that this works > > >> on Linux 5.10 and macOS Catalina. > > >> > > >> As a bonus this allows us to detect when the fd cannot listen. Now we'll > > >> be able to catch unbound or connected fds in socket_listen(). > > >> > > >> Drop the num argument from socket_get_fd() since this function is also > > >> called by socket_connect() where a listen backlog value does not make > > >> sense. > > >> > > >> Fixes: e5b6353cf25c99c3f08bf51e29933352f7140e8f ("socket: Add backlog parameter to socket_listen") > > >> Reported-by: Richard W.M. Jones <rjones@redhat.com> > > >> Cc: Juan Quintela <quintela@redhat.com> > > >> Cc: Eric Blake <eblake@redhat.com> > > >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > >> --- > > >> util/qemu-sockets.c | 29 ++++++++++++++++++++++------- > > >> 1 file changed, 22 insertions(+), 7 deletions(-) > > > > > > Dan and Gerd: Can this go via one of your trees? > > > > > > > As it showed up as a regression in qemu-nbd, I can also consider queuing > > it in my NBD tree. However, I claim it counts as a bug fix, so it is > > fine for -rc1 even if it misses soft freeze. > > > > I'm fine whichever maintainer takes this, although I've now flagged it > > to go through an NBD pull request if it doesn't land elsewhere sooner. > > Ping? I didn't see this land in qemu.git. I notice this patch also fixes: https://gitlab.com/qemu-project/qemu/-/issues/218 Rich.
On Tue, May 11, 2021 at 09:23:10AM +0100, Stefan Hajnoczi wrote: > On Tue, Mar 16, 2021 at 08:35:14AM -0500, Eric Blake wrote: > > On 3/16/21 4:10 AM, Stefan Hajnoczi wrote: > > > On Wed, Mar 10, 2021 at 05:30:04PM +0000, Stefan Hajnoczi wrote: > > >> socket_get_fd() fails with the error "socket_get_fd: too many > > >> connections" if the given listen backlog value is not 1. > > >> > > >> Not all callers set the backlog to 1. For example, commit > > >> 582d4210eb2f2ab5baac328fe4b479cd86da1647 ("qemu-nbd: Use SOMAXCONN for > > >> socket listen() backlog") uses SOMAXCONN. This will always fail with in > > >> socket_get_fd(). > > >> > > >> This patch calls listen(2) on the fd to update the backlog value. The > > >> socket may already be in the listen state. I have tested that this works > > >> on Linux 5.10 and macOS Catalina. > > >> > > >> As a bonus this allows us to detect when the fd cannot listen. Now we'll > > >> be able to catch unbound or connected fds in socket_listen(). > > >> > > >> Drop the num argument from socket_get_fd() since this function is also > > >> called by socket_connect() where a listen backlog value does not make > > >> sense. > > >> > > >> Fixes: e5b6353cf25c99c3f08bf51e29933352f7140e8f ("socket: Add backlog parameter to socket_listen") > > >> Reported-by: Richard W.M. Jones <rjones@redhat.com> > > >> Cc: Juan Quintela <quintela@redhat.com> > > >> Cc: Eric Blake <eblake@redhat.com> > > >> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > >> --- > > >> util/qemu-sockets.c | 29 ++++++++++++++++++++++------- > > >> 1 file changed, 22 insertions(+), 7 deletions(-) > > > > > > Dan and Gerd: Can this go via one of your trees? > > > > > > > As it showed up as a regression in qemu-nbd, I can also consider queuing > > it in my NBD tree. However, I claim it counts as a bug fix, so it is > > fine for -rc1 even if it misses soft freeze. > > > > I'm fine whichever maintainer takes this, although I've now flagged it > > to go through an NBD pull request if it doesn't land elsewhere sooner. > > Ping? I didn't see this land in qemu.git. And a second reminder. qemu-storage-daemon is broken in the released qemu 6.0.0 so it'd be good to get this commit into the stable branch as well. Rich.
Please consider this patch for the QEMU 6.0 stable release. The bug was introduced in QEMU 6.0. Thanks, Stefan On Wed, Mar 10, 2021 at 5:54 PM Stefan Hajnoczi <stefanha@redhat.com> wrote: > > socket_get_fd() fails with the error "socket_get_fd: too many > connections" if the given listen backlog value is not 1. > > Not all callers set the backlog to 1. For example, commit > 582d4210eb2f2ab5baac328fe4b479cd86da1647 ("qemu-nbd: Use SOMAXCONN for > socket listen() backlog") uses SOMAXCONN. This will always fail with in > socket_get_fd(). > > This patch calls listen(2) on the fd to update the backlog value. The > socket may already be in the listen state. I have tested that this works > on Linux 5.10 and macOS Catalina. > > As a bonus this allows us to detect when the fd cannot listen. Now we'll > be able to catch unbound or connected fds in socket_listen(). > > Drop the num argument from socket_get_fd() since this function is also > called by socket_connect() where a listen backlog value does not make > sense. > > Fixes: e5b6353cf25c99c3f08bf51e29933352f7140e8f ("socket: Add backlog parameter to socket_listen") > Reported-by: Richard W.M. Jones <rjones@redhat.com> > Cc: Juan Quintela <quintela@redhat.com> > Cc: Eric Blake <eblake@redhat.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > util/qemu-sockets.c | 29 ++++++++++++++++++++++------- > 1 file changed, 22 insertions(+), 7 deletions(-) > > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c > index 8af0278f15..2463c49773 100644 > --- a/util/qemu-sockets.c > +++ b/util/qemu-sockets.c > @@ -1116,14 +1116,10 @@ fail: > return NULL; > } > > -static int socket_get_fd(const char *fdstr, int num, Error **errp) > +static int socket_get_fd(const char *fdstr, Error **errp) > { > Monitor *cur_mon = monitor_cur(); > int fd; > - if (num != 1) { > - error_setg_errno(errp, EINVAL, "socket_get_fd: too many connections"); > - return -1; > - } > if (cur_mon) { > fd = monitor_get_fd(cur_mon, fdstr, errp); > if (fd < 0) { > @@ -1159,7 +1155,7 @@ int socket_connect(SocketAddress *addr, Error **errp) > break; > > case SOCKET_ADDRESS_TYPE_FD: > - fd = socket_get_fd(addr->u.fd.str, 1, errp); > + fd = socket_get_fd(addr->u.fd.str, errp); > break; > > case SOCKET_ADDRESS_TYPE_VSOCK: > @@ -1187,7 +1183,26 @@ int socket_listen(SocketAddress *addr, int num, Error **errp) > break; > > case SOCKET_ADDRESS_TYPE_FD: > - fd = socket_get_fd(addr->u.fd.str, num, errp); > + fd = socket_get_fd(addr->u.fd.str, errp); > + if (fd < 0) { > + return -1; > + } > + > + /* > + * If the socket is not yet in the listen state, then transition it to > + * the listen state now. > + * > + * If it's already listening then this updates the backlog value as > + * requested. > + * > + * If this socket cannot listen because it's already in another state > + * (e.g. unbound or connected) then we'll catch the error here. > + */ > + if (listen(fd, num) != 0) { > + error_setg_errno(errp, errno, "Failed to listen on fd socket"); > + closesocket(fd); > + return -1; > + } > break; > > case SOCKET_ADDRESS_TYPE_VSOCK: > -- > 2.29.2 >
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index 8af0278f15..2463c49773 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -1116,14 +1116,10 @@ fail: return NULL; } -static int socket_get_fd(const char *fdstr, int num, Error **errp) +static int socket_get_fd(const char *fdstr, Error **errp) { Monitor *cur_mon = monitor_cur(); int fd; - if (num != 1) { - error_setg_errno(errp, EINVAL, "socket_get_fd: too many connections"); - return -1; - } if (cur_mon) { fd = monitor_get_fd(cur_mon, fdstr, errp); if (fd < 0) { @@ -1159,7 +1155,7 @@ int socket_connect(SocketAddress *addr, Error **errp) break; case SOCKET_ADDRESS_TYPE_FD: - fd = socket_get_fd(addr->u.fd.str, 1, errp); + fd = socket_get_fd(addr->u.fd.str, errp); break; case SOCKET_ADDRESS_TYPE_VSOCK: @@ -1187,7 +1183,26 @@ int socket_listen(SocketAddress *addr, int num, Error **errp) break; case SOCKET_ADDRESS_TYPE_FD: - fd = socket_get_fd(addr->u.fd.str, num, errp); + fd = socket_get_fd(addr->u.fd.str, errp); + if (fd < 0) { + return -1; + } + + /* + * If the socket is not yet in the listen state, then transition it to + * the listen state now. + * + * If it's already listening then this updates the backlog value as + * requested. + * + * If this socket cannot listen because it's already in another state + * (e.g. unbound or connected) then we'll catch the error here. + */ + if (listen(fd, num) != 0) { + error_setg_errno(errp, errno, "Failed to listen on fd socket"); + closesocket(fd); + return -1; + } break; case SOCKET_ADDRESS_TYPE_VSOCK:
socket_get_fd() fails with the error "socket_get_fd: too many connections" if the given listen backlog value is not 1. Not all callers set the backlog to 1. For example, commit 582d4210eb2f2ab5baac328fe4b479cd86da1647 ("qemu-nbd: Use SOMAXCONN for socket listen() backlog") uses SOMAXCONN. This will always fail with in socket_get_fd(). This patch calls listen(2) on the fd to update the backlog value. The socket may already be in the listen state. I have tested that this works on Linux 5.10 and macOS Catalina. As a bonus this allows us to detect when the fd cannot listen. Now we'll be able to catch unbound or connected fds in socket_listen(). Drop the num argument from socket_get_fd() since this function is also called by socket_connect() where a listen backlog value does not make sense. Fixes: e5b6353cf25c99c3f08bf51e29933352f7140e8f ("socket: Add backlog parameter to socket_listen") Reported-by: Richard W.M. Jones <rjones@redhat.com> Cc: Juan Quintela <quintela@redhat.com> Cc: Eric Blake <eblake@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- util/qemu-sockets.c | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-)