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(-)

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));
+        }
     }
 }
 
-- 
2.44.0

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?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

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