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 > > > > > <[email protected]> > > > > > --- > > > > > 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 :|
