On Wed, Sep 03, 2025 at 12:44:03PM +0300, Vladimir Sementsov-Ogievskiy wrote: > In generic code we have qio_channel_set_blocking(), which takes > bool parameter, and qemu_file_set_blocking(), which as well takes > bool parameter. > > At lower fd-layer we have a mess of functions: > > - enough direct calls to g_unix_set_fd_nonblocking() > and several wrappers without bool parameter: > > - qemu_scoket_set_nonblock(), which asserts success for posix (still, > in most cases we can handle the error in better way) and ignores > error for win32 realization > > - qemu_socket_try_set_nonblock(), the best one
IMHO this method should not actually exist. The only reason it was used in the net/ dir was to validate an FD received from the monitor was indeed an open socket FD. net/socket.c later added a explicit method 'net_socket_fd_check' to solve this, so we could have switched away from try_set_nonblock to set_nonblock. > > - qemu_socket_set_block(), which simply ignores an error, the worst > case > > And all three lack errp argument, so we have to handle it after the > call. They lacked an errp on the expectation that this call should never fail, if the FD is valid. While I understand the motivation of that decision, IMHO, it is an undesirable optimization in the API design. Not all callers can have such confidence. e.g. received the FD from the monitor, where we really really really must never allow invalid user supplied FD to result in an assert(). I'm inclined to agree that we should have an errp here, and any caller that is *certain* their FD is valid could easily pass in &error_abort. > > So let's introduce a new socket-layer wrapper, and use it consistently > in following commits. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <[email protected]> > --- > include/qemu/osdep.h | 1 + > util/oslib-posix.c | 12 ++++++++++++ > util/oslib-win32.c | 18 ++++++++++++++++++ > 3 files changed, 31 insertions(+) > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > index be3460b32f..1b38cb7e45 100644 > --- a/include/qemu/osdep.h > +++ b/include/qemu/osdep.h > @@ -687,6 +687,7 @@ ssize_t qemu_write_full(int fd, const void *buf, size_t > count) > G_GNUC_WARN_UNUSED_RESULT; > > void qemu_set_cloexec(int fd); > +bool qemu_set_blocking(int fd, bool block, Error **errp); > > /* Return a dynamically allocated directory path that is appropriate for > storing > * local state. > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > index 4ff577e5de..e473938195 100644 > --- a/util/oslib-posix.c > +++ b/util/oslib-posix.c > @@ -250,6 +250,18 @@ void qemu_anon_ram_free(void *ptr, size_t size) > #endif > } > > +bool qemu_set_blocking(int fd, bool block, Error **errp) > +{ > + if (!g_unix_set_fd_nonblocking(fd, !block, NULL)) { > + error_setg_errno(errp, errno, > + "Can't set file descriptor %d %s", fd, > + block ? "blocking" : "non-blocking"); > + return false; > + } > + > + return true; > +} > + > void qemu_socket_set_block(int fd) > { > g_unix_set_fd_nonblocking(fd, false, NULL); > diff --git a/util/oslib-win32.c b/util/oslib-win32.c > index b7351634ec..03044f5b59 100644 > --- a/util/oslib-win32.c > +++ b/util/oslib-win32.c > @@ -177,6 +177,24 @@ static int socket_error(void) > } > } > > +bool qemu_set_blocking(int fd, bool block, Error **errp) > +{ > + unsigned long opt = block ? 0 : 1; > + > + if (block) { > + qemu_socket_unselect(fd, NULL); > + } > + > + if (ioctlsocket(fd, FIONBIO, &opt) != NO_ERROR) { > + error_setg_errno(errp, socket_error(), > + "Can't set file descriptor %d %s", fd, > + block ? "blocking" : "non-blocking"); > + return false; > + } > + > + return true; > +} I don't think this is a good idea - it is giving the impression that it works on any FD, but it only supports sockets. With our current APIs it is clear that setting non-blocking is portable for sockets, but via use of g_unix_set_fd_nonblocking, non-portable for non-sockets. The API design of this method is fine, but I think we should keep separate 'qemu_socket_set_blocking' and 'qemu_file_set_blocking' methods, so that we maintain a clear awareness of the difference for Windows. 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 :|
