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 <vsement...@yandex-team.ru>
> Reviewed-by: Peter Xu <pet...@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 :|
