Libguestfs - Aug 2019 - Re: [PATCH nbdkit] freebsd: In nbdkit_nanosleep, fallback to calling nanosleep(2).

Rather than failing to compile on platforms which lack POLLRDHUP such
as FreeBSD, simply fallback to the old method of sleeping.

This leaves the porting suggestions as a comment in case someone wants
to implement a better solution for particular platforms.
---
 server/public.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/server/public.c b/server/public.c
index a992df1..630de9b 100644
--- a/server/public.c
+++ b/server/public.c
@@ -304,6 +304,16 @@ nbdkit_realpath (const char *path)
 int
 nbdkit_nanosleep (unsigned sec, unsigned nsec)
 {
+  struct timespec ts;
+
+  if (sec >= INT_MAX - nsec / 1000000000) {
+    nbdkit_error ("sleep request is too long");
+    errno = EINVAL;
+    return -1;
+  }
+  ts.tv_sec = sec + nsec / 1000000000;
+  ts.tv_nsec = nsec % 1000000000;
+
 #if defined HAVE_PPOLL && defined POLLRDHUP
   /* End the sleep early if any of these happen:
    * - nbdkit has received a signal to shut down the server
@@ -311,7 +321,6 @@ nbdkit_nanosleep (unsigned sec, unsigned nsec)
    *   NBD_CMD_DISC or a problem with the connection
    * - the input socket detects POLLRDHUP/POLLHUP/POLLERR
    */
-  struct timespec ts;
   struct connection *conn = threadlocal_get_conn ();
   struct pollfd fds[] = {
     [0].fd = quit_fd,
@@ -323,14 +332,6 @@ nbdkit_nanosleep (unsigned sec, unsigned nsec)
   };
   sigset_t all;
 
-  if (sec >= INT_MAX - nsec / 1000000000) {
-    nbdkit_error ("sleep request is too long");
-    errno = EINVAL;
-    return -1;
-  }
-  ts.tv_sec = sec + nsec / 1000000000;
-  ts.tv_nsec = nsec % 1000000000;
-
   /* Block all signals to this thread during the poll, so we don't
    * have to worry about EINTR
    */
@@ -354,9 +355,13 @@ nbdkit_nanosleep (unsigned sec, unsigned nsec)
   nbdkit_error ("aborting sleep to shut down");
   errno = ESHUTDOWN;
   return -1;
+
 #else
-# error "Please port this to your platform"
-  /* Porting ideas, in order of preference:
+  /* The fallback path simply calls ordinary nanosleep, and will
+   * cause long delays on server shutdown.
+   *
+   * If however you want to port this to your platform, then
+   * porting ideas, in order of preference:
    * - POSIX requires pselect; it's a bit clunkier to set up than poll,
    *   but the same ability to atomically mask all signals and operate
    *   on struct timespec makes it similar to the preferred ppoll interface
@@ -364,8 +369,13 @@ nbdkit_nanosleep (unsigned sec, unsigned nsec)
    *   a recalculation of the timeout to still reach the end time (masking
    *   signals in that case is not safe, as it is a non-atomic race)
    */
-  nbdkit_error ("nbdkit_nanosleep not yet ported to systems without
ppoll");
-  errno = ENOSYS;
-  return -1;
+  int r;
+
+  r = nanosleep (&ts, NULL);
+  if (r == -1 && errno != EINTR && errno != EAGAIN) {
+    nbdkit_error ("nanosleep: %m");
+    return -1;
+  }
+  return 0;
 #endif
 }
-- 
2.22.0

On 8/28/19 11:14 AM, Richard W.M. Jones wrote:
> Rather than failing to compile on platforms which lack POLLRDHUP such
> as FreeBSD, simply fallback to the old method of sleeping.
> 
> This leaves the porting suggestions as a comment in case someone wants
> to implement a better solution for particular platforms.
> ---
>  server/public.c | 38 ++++++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 14 deletions(-)
This means 'make check' will fail on FreeBSD; do we have a way to
introspect whether a platform lacks POLLRDHUP this is the case, to skip
rather than fail tests/test-shutdown.sh on those platforms?

At any rate, the fallback is reasonable, even if underwhelming in
performance, so ACK.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Wed, Aug 28, 2019 at 12:02:44PM -0500, Eric Blake
wrote:
> On 8/28/19 11:14 AM, Richard W.M. Jones wrote:
> > Rather than failing to compile on platforms which lack POLLRDHUP such
> > as FreeBSD, simply fallback to the old method of sleeping.
> > 
> > This leaves the porting suggestions as a comment in case someone wants
> > to implement a better solution for particular platforms.
> > ---
> >  server/public.c | 38 ++++++++++++++++++++++++--------------
> >  1 file changed, 24 insertions(+), 14 deletions(-)
> 
> This means 'make check' will fail on FreeBSD; do we have a way to
> introspect whether a platform lacks POLLRDHUP this is the case, to skip
> rather than fail tests/test-shutdown.sh on those platforms?
> 
> At any rate, the fallback is reasonable, even if underwhelming in
> performance, so ACK.
Thanks.  I'm going to push this.

The current situation on FreeBSD (11.1-RELEASE) is that a lot of tests
fail including this one.  I'm currently (and very slowly) upgrading to
12.0-RELEASE which may have a newer qemu which should fix most of the
failures, and if there's an easy way to fix this test for FreeBSD then
I'll do that later.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v