Re: [PATCH 03/10] util: add qemu_set_blocking() function

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Sep 03, 2025 at 12:44:03PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> In generic code we have qio_channel_set_blocking(), which takes
> bool parameter, and qemu_file_set_blocking(), which as well takes
> bool parameter.
> 
> At lower fd-layer we have a mess of functions:
> 
> - enough direct calls to g_unix_set_fd_nonblocking()
> and several wrappers without bool parameter:
> 
> - qemu_scoket_set_nonblock(), which asserts success for posix (still,
>   in most cases we can handle the error in better way) and ignores
>   error for win32 realization
> 
> - qemu_socket_try_set_nonblock(), the best one

IMHO this method should not actually exist. The only reason it was
used in the net/ dir was to validate an FD received from the monitor
was indeed an open socket FD. net/socket.c later added a explicit
method 'net_socket_fd_check' to solve this, so we could have switched
away from try_set_nonblock to set_nonblock.

> 
> - qemu_socket_set_block(), which simply ignores an error, the worst
>   case
> 
> And all three lack errp argument, so we have to handle it after the
> call.

They lacked an errp on the expectation that this call should never
fail, if the FD is valid. While I understand the motivation of that
decision, IMHO, it is an undesirable optimization in the API design.

Not all callers can have such confidence. e.g. received the FD from
the monitor, where we really really really must never allow invalid
user supplied FD to result in an assert().

I'm inclined to agree that we should have an errp here, and any
caller that is *certain* their FD is valid could easily pass in
&error_abort.

> 
> So let's introduce a new socket-layer wrapper, and use it consistently
> in following commits.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  include/qemu/osdep.h |  1 +
>  util/oslib-posix.c   | 12 ++++++++++++
>  util/oslib-win32.c   | 18 ++++++++++++++++++
>  3 files changed, 31 insertions(+)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index be3460b32f..1b38cb7e45 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -687,6 +687,7 @@ ssize_t qemu_write_full(int fd, const void *buf, size_t count)
>      G_GNUC_WARN_UNUSED_RESULT;
>  
>  void qemu_set_cloexec(int fd);
> +bool qemu_set_blocking(int fd, bool block, Error **errp);
>  
>  /* Return a dynamically allocated directory path that is appropriate for storing
>   * local state.
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 4ff577e5de..e473938195 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -250,6 +250,18 @@ void qemu_anon_ram_free(void *ptr, size_t size)
>  #endif
>  }
>  
> +bool qemu_set_blocking(int fd, bool block, Error **errp)
> +{
> +    if (!g_unix_set_fd_nonblocking(fd, !block, NULL)) {
> +        error_setg_errno(errp, errno,
> +                         "Can't set file descriptor %d %s", fd,
> +                         block ? "blocking" : "non-blocking");
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
>  void qemu_socket_set_block(int fd)
>  {
>      g_unix_set_fd_nonblocking(fd, false, NULL);
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index b7351634ec..03044f5b59 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -177,6 +177,24 @@ static int socket_error(void)
>      }
>  }
>  
> +bool qemu_set_blocking(int fd, bool block, Error **errp)
> +{
> +    unsigned long opt = block ? 0 : 1;
> +
> +    if (block) {
> +        qemu_socket_unselect(fd, NULL);
> +    }
> +
> +    if (ioctlsocket(fd, FIONBIO, &opt) != NO_ERROR) {
> +        error_setg_errno(errp, socket_error(),
> +                         "Can't set file descriptor %d %s", fd,
> +                         block ? "blocking" : "non-blocking");
> +        return false;
> +    }
> +
> +    return true;
> +}

I don't think this is a good idea - it is giving the impression that
it works on any FD, but it only supports sockets.

With our current APIs it is clear that setting non-blocking is portable
for sockets, but via use of g_unix_set_fd_nonblocking, non-portable
for non-sockets.

The API design of this method is fine, but I think we should keep 
separate 'qemu_socket_set_blocking' and 'qemu_file_set_blocking'
methods, so that we maintain a clear awareness of the difference
for Windows.

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 :|