Re: [PATCH 07/10] use qemu_set_blocking instead of g_unix_set_fd_nonblocking

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

On Tue, Sep 09, 2025 at 12:50:00PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 09.09.25 12:15, Daniel P. Berrangé wrote:
> > On Tue, Sep 09, 2025 at 12:11:19PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > On 09.09.25 12:00, Daniel P. Berrangé wrote:
> > > > On Wed, Sep 03, 2025 at 12:44:07PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > > > Instead of open-coded g_unix_set_fd_nonblocking() calls, use
> > > > > QEMU wrapper qemu_socket_set_nonblock().
> > > > > 
> > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > > > > ---
> > > > >    chardev/char-fd.c                  |  4 ++--
> > > > >    chardev/char-pty.c                 |  3 +--
> > > > >    chardev/char-serial.c              |  3 +--
> > > > >    chardev/char-stdio.c               |  3 +--
> > > > >    hw/input/virtio-input-host.c       |  3 +--
> > > > >    hw/misc/ivshmem-flat.c             |  4 +++-
> > > > >    hw/misc/ivshmem-pci.c              |  8 +++++++-
> > > > >    hw/virtio/vhost-vsock.c            |  8 ++------
> > > > >    io/channel-command.c               |  9 ++++++---
> > > > >    io/channel-file.c                  |  3 +--
> > > > >    net/tap-bsd.c                      | 12 ++++++++++--
> > > > >    net/tap-linux.c                    |  8 +++++++-
> > > > >    net/tap-solaris.c                  |  7 ++++++-
> > > > >    net/tap.c                          | 21 ++++++---------------
> > > > >    qga/commands-posix.c               |  3 +--
> > > > >    tests/qtest/fuzz/virtio_net_fuzz.c |  2 +-
> > > > >    tests/qtest/vhost-user-test.c      |  3 +--
> > > > >    tests/unit/test-iov.c              |  5 +++--
> > > > >    ui/input-linux.c                   |  3 +--
> > > > >    util/event_notifier-posix.c        |  5 +++--
> > > > >    util/main-loop.c                   |  6 +++++-
> > > > >    21 files changed, 69 insertions(+), 54 deletions(-)
> > > > 
> > > > > diff --git a/io/channel-command.c b/io/channel-command.c
> > > > > index 8966dd3a2b..8ae9a026b3 100644
> > > > > --- a/io/channel-command.c
> > > > > +++ b/io/channel-command.c
> > > > > @@ -277,9 +277,12 @@ static int qio_channel_command_set_blocking(QIOChannel *ioc,
> > > > >        cioc->blocking = enabled;
> > > > >    #else
> > > > > -    if ((cioc->writefd >= 0 && !g_unix_set_fd_nonblocking(cioc->writefd, !enabled, NULL)) ||
> > > > > -        (cioc->readfd >= 0 && !g_unix_set_fd_nonblocking(cioc->readfd, !enabled, NULL))) {
> > > > > -        error_setg_errno(errp, errno, "Failed to set FD nonblocking");
> > > > > +    if (cioc->writefd >= 0 &&
> > > > > +        !qemu_set_blocking(cioc->writefd, enabled, errp)) {
> > > > > +        return -1;
> > > > > +    }
> > > > > +    if (cioc->readfd >= 0 &&
> > > > > +        !qemu_set_blocking(cioc->readfd, enabled, errp)) {
> > > > >            return -1;
> > > > >        }
> > > > >    #endif
> > > > > diff --git a/io/channel-file.c b/io/channel-file.c
> > > > > index ca3f180cc2..5cef75a67c 100644
> > > > > --- a/io/channel-file.c
> > > > > +++ b/io/channel-file.c
> > > > > @@ -223,8 +223,7 @@ static int qio_channel_file_set_blocking(QIOChannel *ioc,
> > > > >    #else
> > > > >        QIOChannelFile *fioc = QIO_CHANNEL_FILE(ioc);
> > > > > -    if (!g_unix_set_fd_nonblocking(fioc->fd, !enabled, NULL)) {
> > > > > -        error_setg_errno(errp, errno, "Failed to set FD nonblocking");
> > > > > +    if (!qemu_set_blocking(fioc->fd, enabled, errp)) {
> > > > >            return -1;
> > > > >        }
> > > > >        return 0;
> > > > 
> > > > This is wrong for Windows. fioc->fd is not a socket, but this is passing
> > > > it to an API whose impl assume it is receiving a socket.
> > > > 
> > > 
> > > But what is changed with the patch? g_unix_set_fd_nonblocking(fioc->fd, ..) is wrong for Windows as well.
> > 
> > Actually I missed the #ifdef. This code is in an #else branch that excludes
> > compilation on Wnidows - the Windows brach just raise an error.
> > 
> > > And making separate qemu_set_blocking() and qemu_socket_set_blocking(), which do the same
> > > thing, doesn't make sense..
> > > 
> > > Hmm. But we can define qemu_set_blocking() only for Linux, keeping qemu_socket_set_blocking() the generic
> > > function. Still, nothing prevents using qemu_socket_set_blocking() on non-sockets..
> > 
> > We just relying on the name of the function alerting the developer / reviewer.
> > If you're dealing with a FIFO pipe FD you are (hopefully) going to realize
> > that qemu_socket_XXX is not a function to be used.
> > 
> 
> Understand. But having both qemu_XXX() and qemu_socket_XXX(), I'd be confused, why not just use qemu_XXX() everywhere.
> 
> So, for final API, either we have one function qemu_set_blocking() (as the series propose), and lose this alert,
> 
> or, we should have something like:
> 
> qemu_socket_set_blocking() - generic, for calling on sockets, Windows or Linux
> 
> qemu_unix_set_blocking() - defined only for Linux
> 
> What I dislike with the latter:
> 
> 1. For Linux functions are duplicates actually. So, we have a defined only for Linux duplicate of generic function
> 
> 2. Nothing prevents using qemu_socket_set_blocking() on any fds in Linux except name (and it will succeed!!)

I looked at what we do in libvirt, and there we just have a single
function virSetBlocking() that simply does ioctrlsocket() on Windows.
I can't say we've had any significant trouble with that, though our
Windows scope & usage is much less than QEMU's.

A key difference though is that callers are expected to check the
return value for failure.


With the current QEMU design where we do NOT expect callers to check
for failure, I think it is important that the function naming make
clear the intended use case, as we won't get runtime diagnosis of
incorrect calls.

So with this in mind, I'm actually OK with your proposed design for
a general  qemu_set_blocking, as that has an Error **errp parameter
encouraging failure checking by callers.

The ioctlsocket method will also return a clear error if passed
something that is not a SOCKET, but a generic file backed HANDLE.

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