Re: [PATCH v2 4/8] util: drop qemu_socket_set_nonblock()

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

On Thu, Sep 11, 2025 at 12:20:02PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Use common qemu_set_blocking() instead.
> 
> Note that pre-patch the behavior of Win32 and Linux realizations
> are inconsistent: we ignore failure for Win32, and assert success
> for Linux.
> 
> How do we convert the callers?

> 3. io/channel-socket.c: here we convert both old calls to
> qemu_socket_set_nonblock() and qemu_socket_set_block() to
> one new call. Pre-patch we assert success for Linux in
> qemu_socket_set_nonblock(), and ignore all other errors here.
> Still, all callers pass errp=NULL to qio_channel_set_blocking(),
> so after patch we ignore all errors. Switching from assertion
> to ignoring may be not very good, but still acceptable, keeping
> in mind that all callers of qio_channel_set_blocking() do
> explicitly ignore the error.

This is a bit questionable. IMHO the reason why nearly all
callers pass errp=NULL is laziness based on the assumption
that the code actually asserts, and no one thinking to check
the win impl which didn't asset.

IOW, I think we need a prior patch that sets the 'errp'
in all these callers, either to &error_abort, or to
propage the error if practical.

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> ---
>  contrib/ivshmem-server/ivshmem-server.c |  6 +++++-
>  hw/hyperv/syndbg.c                      |  4 +++-
>  hw/virtio/vhost-user.c                  |  5 ++++-
>  include/qemu/sockets.h                  |  1 -
>  io/channel-socket.c                     |  7 +++----
>  net/dgram.c                             | 16 +++++++++++++---
>  net/l2tpv3.c                            |  5 +++--
>  net/socket.c                            | 20 ++++++++++++++++----
>  qga/channel-posix.c                     |  7 ++++++-
>  tests/unit/socket-helpers.c             |  5 ++++-
>  tests/unit/test-crypto-tlssession.c     |  8 ++++----
>  util/oslib-posix.c                      |  7 -------
>  util/oslib-win32.c                      |  5 -----
>  util/vhost-user-server.c                |  4 ++--
>  14 files changed, 63 insertions(+), 37 deletions(-)
> 
> diff --git a/contrib/ivshmem-server/ivshmem-server.c b/contrib/ivshmem-server/ivshmem-server.c
> index 2f3c7320a6..a65943d0b8 100644
> --- a/contrib/ivshmem-server/ivshmem-server.c
> +++ b/contrib/ivshmem-server/ivshmem-server.c
> @@ -146,9 +146,13 @@ ivshmem_server_handle_new_conn(IvshmemServer *server)
>          return -1;
>      }
>  
> -    qemu_socket_set_nonblock(newfd);
>      IVSHMEM_SERVER_DEBUG(server, "accept()=%d\n", newfd);
>  
> +    if (!qemu_set_blocking(newfd, false, NULL)) {

&error_warn here so we diagnose the reason we're returning -1
to the caller ?

> +        close(newfd);
> +        return -1;
> +    }
> +
>      /* allocate new structure for this peer */
>      peer = g_malloc0(sizeof(*peer));
>      peer->sock_fd = newfd;

> diff --git a/tests/unit/socket-helpers.c b/tests/unit/socket-helpers.c
> index 37db24f72a..1b7e283f24 100644
> --- a/tests/unit/socket-helpers.c
> +++ b/tests/unit/socket-helpers.c
> @@ -88,7 +88,10 @@ static int socket_can_bind_connect(const char *hostname, int family)
>          goto cleanup;
>      }
>  
> -    qemu_socket_set_nonblock(cfd);
> +    if (!qemu_set_blocking(cfd, false, NULL)) {

&error_abort here.

> +        goto cleanup;
> +    }
> +
>      if (connect(cfd, (struct sockaddr *)&ss, sslen) < 0) {
>          if (errno == EINPROGRESS) {
>              check_soerr = true;
 
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 :|