Re: [PATCH v5 06/13] util: drop qemu_socket_set_nonblock()

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

On Tue, Sep 16, 2025 at 04:13:55PM +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?
> 
> 1. Most of callers call qemu_socket_set_nonblock() on a
> freshly created socket fd, in conditions when we may simply
> report an error. Seems correct switching to error handling
> both for Windows (pre-patch error is ignored) and Linux
> (pre-patch we assert success). Anyway, we normally don't
> expect errors in these cases.
> 
> Still in tests let's use &error_abort for simplicity.
> 
> What are exclusions?
> 
> 2. hw/virtio/vhost-user.c - we are inside #ifdef CONFIG_LINUX,
> so no damage in switching to error handling from assertion.
> 
> 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.
> So, for Windows switch is a bit dangerous: we may get
> new errors or crashes(when error_abort is passed) in
> cases where we have silently ignored the error before
> (was it correct in all such cases, if they were?) Still,
> there is no other way to stricter API than take
> this risk.
> 
> 4. util/vhost-user-server - compiled only for Linux (see
> util/meson.build), so we are safe, switching from assertion to
> &error_abort.
> 
> Note: In qga/channel-posix.c we use g_warning(), where g_printerr()
> would actually be a better choice. Still let's for now follow
> common style of qga, where g_warning() is commonly used to print
> such messages, and no call to g_printerr(). Converting everything
> to use g_printerr() should better be another series.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  contrib/ivshmem-server/ivshmem-server.c |  9 ++++++++-
>  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             |  4 +++-
>  tests/unit/test-crypto-tlssession.c     |  8 ++++----
>  util/oslib-posix.c                      |  7 -------
>  util/oslib-win32.c                      |  5 -----
>  util/vhost-user-server.c                |  6 ++++--
>  14 files changed, 67 insertions(+), 37 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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