| Message ID | 20240326133936.125332-5-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vhost-user: support any POSIX system (tested on macOS, FreeBSD, OpenBSD) | expand |
On Tue, Mar 26, 2024 at 02:39:29PM +0100, Stefano Garzarella wrote: > In vhost-user-server we set all fd received from the other peer > in non-blocking mode. For some of them (e.g. memfd, shm_open, etc.) > if we fail, it's not really a problem, because we don't use these > fd with blocking operations, but only to map memory. > > In these cases a failure is not bad, so let's just report a warning > instead of panicking if we fail to set some fd in non-blocking mode. > > This for example occurs in macOS where setting shm_open() fd > non-blocking is failing (errno: 25). What is errno 25 on MacOS? > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > util/vhost-user-server.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c > index 3bfb1ad3ec..064999f0b7 100644 > --- a/util/vhost-user-server.c > +++ b/util/vhost-user-server.c > @@ -66,7 +66,11 @@ static void vmsg_unblock_fds(VhostUserMsg *vmsg) > { > int i; > for (i = 0; i < vmsg->fd_num; i++) { > - qemu_socket_set_nonblock(vmsg->fds[i]); > + int ret = qemu_socket_try_set_nonblock(vmsg->fds[i]); > + if (ret) { Should this be 'if (ret < 0)'? > + warn_report("Failed to set fd %d nonblock for request %d: %s", > + vmsg->fds[i], vmsg->request, strerror(-ret)); > + } This now ignores all errors even on pre-existing fds where we NEED non-blocking, rather than just the specific (expected) error we are seeing on MacOS. Should this code be a bit more precise about checking that -ret == EXXX for the expected errno value we are ignoring for the specific fds where non-blocking is not essential?
On Tue, Mar 26, 2024 at 09:40:12AM -0500, Eric Blake wrote: >On Tue, Mar 26, 2024 at 02:39:29PM +0100, Stefano Garzarella wrote: >> In vhost-user-server we set all fd received from the other peer >> in non-blocking mode. For some of them (e.g. memfd, shm_open, etc.) >> if we fail, it's not really a problem, because we don't use these >> fd with blocking operations, but only to map memory. >> >> In these cases a failure is not bad, so let's just report a warning >> instead of panicking if we fail to set some fd in non-blocking mode. >> >> This for example occurs in macOS where setting shm_open() fd >> non-blocking is failing (errno: 25). > >What is errno 25 on MacOS? It should be ENOTTY. I'll add in the commit description. > >> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> --- >> util/vhost-user-server.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c >> index 3bfb1ad3ec..064999f0b7 100644 >> --- a/util/vhost-user-server.c >> +++ b/util/vhost-user-server.c >> @@ -66,7 +66,11 @@ static void vmsg_unblock_fds(VhostUserMsg *vmsg) >> { >> int i; >> for (i = 0; i < vmsg->fd_num; i++) { >> - qemu_socket_set_nonblock(vmsg->fds[i]); >> + int ret = qemu_socket_try_set_nonblock(vmsg->fds[i]); >> + if (ret) { > >Should this be 'if (ret < 0)'? I was confused by the assert() in qemu_socket_set_nonblock(): void qemu_socket_set_nonblock(int fd) { int f; f = qemu_socket_try_set_nonblock(fd); assert(f == 0); } BTW, I see most of the code checks ret < 0, so I'll fix it. > >> + warn_report("Failed to set fd %d nonblock for request %d: %s", >> + vmsg->fds[i], vmsg->request, strerror(-ret)); >> + } > >This now ignores all errors even on pre-existing fds where we NEED >non-blocking, rather than just the specific (expected) error we are >seeing on MacOS. Should this code be a bit more precise about >checking that -ret == EXXX for the expected errno value we are >ignoring for the specific fds where non-blocking is not essential? Good point, maybe I'll just avoid calling vmsg_unblock_fds() when the message is VHOST_USER_ADD_MEM_REG or VHOST_USER_SET_MEM_TABLE. These should be the cases where carried fds are used for mmap() and so there is no need to mark them non-blocking. WDYT? Stefano
diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c index 3bfb1ad3ec..064999f0b7 100644 --- a/util/vhost-user-server.c +++ b/util/vhost-user-server.c @@ -66,7 +66,11 @@ static void vmsg_unblock_fds(VhostUserMsg *vmsg) { int i; for (i = 0; i < vmsg->fd_num; i++) { - qemu_socket_set_nonblock(vmsg->fds[i]); + int ret = qemu_socket_try_set_nonblock(vmsg->fds[i]); + if (ret) { + warn_report("Failed to set fd %d nonblock for request %d: %s", + vmsg->fds[i], vmsg->request, strerror(-ret)); + } } }
In vhost-user-server we set all fd received from the other peer in non-blocking mode. For some of them (e.g. memfd, shm_open, etc.) if we fail, it's not really a problem, because we don't use these fd with blocking operations, but only to map memory. In these cases a failure is not bad, so let's just report a warning instead of panicking if we fail to set some fd in non-blocking mode. This for example occurs in macOS where setting shm_open() fd non-blocking is failing (errno: 25). Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> --- util/vhost-user-server.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)