| Message ID | 20220422083639.3156978-9-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Misc cleanups | expand |
On Fri, Apr 22, 2022 at 12:36:37PM +0400, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > API available since glib 2.30. It also preserves errno. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > hw/misc/ivshmem.c | 2 +- > util/event_notifier-posix.c | 6 ++---- > util/main-loop.c | 2 +- > 3 files changed, 4 insertions(+), 6 deletions(-) There are many more places in QEMU setting O_NONBLOCK. $ git grep '\bO_NONBLOCK' | grep -i setfl hw/misc/ivshmem.c: fcntl_setfl(fd, O_NONBLOCK); /* msix/irqfd poll non block */ hw/rdma/rdma_backend.c: rc = fcntl(backend_dev->channel->fd, F_SETFL, flags | O_NONBLOCK); linux-user/syscall.c: if (fcntl(fd, F_SETFL, O_NONBLOCK | flags) == -1) { net/tap-bsd.c: fcntl(fd, F_SETFL, O_NONBLOCK); net/tap-bsd.c: fcntl(fd, F_SETFL, O_NONBLOCK); net/tap-linux.c: fcntl(fd, F_SETFL, O_NONBLOCK); net/tap-solaris.c: fcntl(fd, F_SETFL, O_NONBLOCK); tests/qtest/fuzz/virtio_net_fuzz.c: fcntl(sockfds[0], F_SETFL, O_NONBLOCK); tests/tcg/multiarch/linux-test.c: chk_error(fcntl(fds[0], F_SETFL, O_NONBLOCK)); tests/tcg/multiarch/linux-test.c: chk_error(fcntl(fds[1], F_SETFL, O_NONBLOCK)); tests/unit/test-iov.c: fcntl(sv[1], F_SETFL, O_RDWR|O_NONBLOCK); tests/unit/test-iov.c: fcntl(sv[0], F_SETFL, O_RDWR|O_NONBLOCK); util/event_notifier-posix.c: ret = fcntl_setfl(fds[0], O_NONBLOCK); util/event_notifier-posix.c: ret = fcntl_setfl(fds[1], O_NONBLOCK); util/main-loop.c: fcntl_setfl(sigfd, O_NONBLOCK); util/oslib-posix.c: f = fcntl(fd, F_SETFL, f & ~O_NONBLOCK); util/oslib-posix.c: if (fcntl(fd, F_SETFL, f | O_NONBLOCK) == -1) { With regards, Daniel
Hi On Fri, Apr 22, 2022 at 12:57 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > On Fri, Apr 22, 2022 at 12:36:37PM +0400, marcandre.lureau@redhat.com > wrote: > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > API available since glib 2.30. It also preserves errno. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > hw/misc/ivshmem.c | 2 +- > > util/event_notifier-posix.c | 6 ++---- > > util/main-loop.c | 2 +- > > 3 files changed, 4 insertions(+), 6 deletions(-) > > There are many more places in QEMU setting O_NONBLOCK. > > $ git grep '\bO_NONBLOCK' | grep -i setfl > hw/misc/ivshmem.c: fcntl_setfl(fd, O_NONBLOCK); /* msix/irqfd poll non > block */ > hw/rdma/rdma_backend.c: rc = fcntl(backend_dev->channel->fd, F_SETFL, > flags | O_NONBLOCK); > linux-user/syscall.c: if (fcntl(fd, F_SETFL, O_NONBLOCK | flags) == > -1) { > net/tap-bsd.c: fcntl(fd, F_SETFL, O_NONBLOCK); > net/tap-bsd.c: fcntl(fd, F_SETFL, O_NONBLOCK); > net/tap-linux.c: fcntl(fd, F_SETFL, O_NONBLOCK); > net/tap-solaris.c: fcntl(fd, F_SETFL, O_NONBLOCK); > tests/qtest/fuzz/virtio_net_fuzz.c: fcntl(sockfds[0], F_SETFL, > O_NONBLOCK); > tests/tcg/multiarch/linux-test.c: chk_error(fcntl(fds[0], F_SETFL, > O_NONBLOCK)); > tests/tcg/multiarch/linux-test.c: chk_error(fcntl(fds[1], F_SETFL, > O_NONBLOCK)); > tests/unit/test-iov.c: fcntl(sv[1], F_SETFL, O_RDWR|O_NONBLOCK); > tests/unit/test-iov.c: fcntl(sv[0], F_SETFL, O_RDWR|O_NONBLOCK); > util/event_notifier-posix.c: ret = fcntl_setfl(fds[0], O_NONBLOCK); > util/event_notifier-posix.c: ret = fcntl_setfl(fds[1], O_NONBLOCK); > util/main-loop.c: fcntl_setfl(sigfd, O_NONBLOCK); > util/oslib-posix.c: f = fcntl(fd, F_SETFL, f & ~O_NONBLOCK); > util/oslib-posix.c: if (fcntl(fd, F_SETFL, f | O_NONBLOCK) == -1) { > As you may have guessed, the goal was to move fcntl_setfl() to block/, as done in next patch. It looks like other callers generally do more than just setting nonblock, they need more careful review. > > 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 :| > > >
On Fri, Apr 22, 2022 at 12:36:37PM +0400, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > API available since glib 2.30. It also preserves errno. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > hw/misc/ivshmem.c | 2 +- > util/event_notifier-posix.c | 6 ++---- > util/main-loop.c | 2 +- > 3 files changed, 4 insertions(+), 6 deletions(-) > > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c > index e7c0099bdaf6..a1cd3dcc51cf 100644 > --- a/hw/misc/ivshmem.c > +++ b/hw/misc/ivshmem.c > @@ -537,7 +537,7 @@ static void process_msg_connect(IVShmemState *s, uint16_t posn, int fd, > > IVSHMEM_DPRINTF("eventfds[%d][%d] = %d\n", posn, vector, fd); > event_notifier_init_fd(&peer->eventfds[vector], fd); > - fcntl_setfl(fd, O_NONBLOCK); /* msix/irqfd poll non block */ > + g_unix_set_fd_nonblocking(fd, TRUE, NULL); /* msix/irqfd poll non block */ Does glib require us to use their non-standard TRUE, or can we merely pass true and rely on C promotion rules to make the code look nicer?
Hi On Fri, Apr 22, 2022 at 6:25 PM Eric Blake <eblake@redhat.com> wrote: > On Fri, Apr 22, 2022 at 12:36:37PM +0400, marcandre.lureau@redhat.com > wrote: > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > API available since glib 2.30. It also preserves errno. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > hw/misc/ivshmem.c | 2 +- > > util/event_notifier-posix.c | 6 ++---- > > util/main-loop.c | 2 +- > > 3 files changed, 4 insertions(+), 6 deletions(-) > > > > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c > > index e7c0099bdaf6..a1cd3dcc51cf 100644 > > --- a/hw/misc/ivshmem.c > > +++ b/hw/misc/ivshmem.c > > @@ -537,7 +537,7 @@ static void process_msg_connect(IVShmemState *s, > uint16_t posn, int fd, > > > > IVSHMEM_DPRINTF("eventfds[%d][%d] = %d\n", posn, vector, fd); > > event_notifier_init_fd(&peer->eventfds[vector], fd); > > - fcntl_setfl(fd, O_NONBLOCK); /* msix/irqfd poll non block */ > > + g_unix_set_fd_nonblocking(fd, TRUE, NULL); /* msix/irqfd poll non > block */ > > Does glib require us to use their non-standard TRUE, or can we merely > pass true and rely on C promotion rules to make the code look nicer? > > No, type promotion is fine fortunately. Will change it to "true".
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c index e7c0099bdaf6..a1cd3dcc51cf 100644 --- a/hw/misc/ivshmem.c +++ b/hw/misc/ivshmem.c @@ -537,7 +537,7 @@ static void process_msg_connect(IVShmemState *s, uint16_t posn, int fd, IVSHMEM_DPRINTF("eventfds[%d][%d] = %d\n", posn, vector, fd); event_notifier_init_fd(&peer->eventfds[vector], fd); - fcntl_setfl(fd, O_NONBLOCK); /* msix/irqfd poll non block */ + g_unix_set_fd_nonblocking(fd, TRUE, NULL); /* msix/irqfd poll non block */ if (posn == s->vm_id) { setup_interrupt(s, vector, errp); diff --git a/util/event_notifier-posix.c b/util/event_notifier-posix.c index df21c2583e1f..21d40b2f1154 100644 --- a/util/event_notifier-posix.c +++ b/util/event_notifier-posix.c @@ -52,13 +52,11 @@ int event_notifier_init(EventNotifier *e, int active) if (!g_unix_open_pipe(fds, FD_CLOEXEC, NULL)) { return -errno; } - ret = fcntl_setfl(fds[0], O_NONBLOCK); - if (ret < 0) { + if (!g_unix_set_fd_nonblocking(fds[0], TRUE, NULL)) { ret = -errno; goto fail; } - ret = fcntl_setfl(fds[1], O_NONBLOCK); - if (ret < 0) { + if (!g_unix_set_fd_nonblocking(fds[1], TRUE, NULL)) { ret = -errno; goto fail; } diff --git a/util/main-loop.c b/util/main-loop.c index b7b0ce4ca087..60ac77602bbb 100644 --- a/util/main-loop.c +++ b/util/main-loop.c @@ -114,7 +114,7 @@ static int qemu_signal_init(Error **errp) return -errno; } - fcntl_setfl(sigfd, O_NONBLOCK); + g_unix_set_fd_nonblocking(sigfd, TRUE, NULL); qemu_set_fd_handler(sigfd, sigfd_handler, NULL, (void *)(intptr_t)sigfd);