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

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