[PATCH 0/1] xnanosleep: Use pause for infinite duration.

[PATCH 0/1] xnanosleep: Use pause for infinite duration.

[Vladimir Panteleev]

Users have discovered that the "sleep" command accepts "infinity" as
its duration argument. However, specifying the "infinity" duration
does not actually cause GNU sleep to sleep for an infinite
duration. This is because the duration was being unconditionally
converted to a timespec value, and infinities are not representable as
timespecs.

Address this by using the "pause" system call (instead of "nanosleep")
when the duration is a positive infinity. This allows us to avoid
burdening the operating system with a timer which likely will never be
reached.

---

Commentary: Yes, this patch isn't too serious. There is an interesting
discussion here [1] which compares a few ways to get a process to
simply block indefinitely. I thought it was a shame that the most
elegant-looking solution, "sleep infinity", turned out to be inferior
to the other, more complicated ones.

Possibly xnanosleep is not the best way to perform this change. The
downside is that it changes the behavior of xnanosleep - it is no
longer just a nanosleep wrapper. The upside is that this the place
where the lossy conversion occurs, so the alternative would mean that
all nanosleep callers would have to check the argument.

I'm not sure if isfinite is the best way to check for infinity, as far
as the tradeoff between portability and dependencies goes. An
alternative would be "static const double infinity = 1.0 / 0.0; if
(seconds == infinity)" but I don't know how good that is in terms of
portability. I noticed that e.g. vasnprintf.c defines a private
isfinite-like function.

By the way, shouldn't xnanosleep be using the second nanosleep
parameter to track how much time there's left to sleep in case of an
interruption?

 [1]: https://stackoverflow.com/a/41655546/21501

---

Vladimir Panteleev (1):
  xnanosleep: Use pause for infinite duration.

 lib/xnanosleep.c | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

-- 
2.25.0

[PATCH] xnanosleep: Use pause for infinite duration.

[Vladimir Panteleev]

* lib/xnanosleep.c (xnanosleep): When the duration is the positive
infinity, use pause instead of nanosleep.
---
 lib/xnanosleep.c | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/lib/xnanosleep.c b/lib/xnanosleep.c
index 5774f75f3..58a2f5254 100644
--- a/lib/xnanosleep.c
+++ b/lib/xnanosleep.c
@@ -25,7 +25,9 @@
 #include <timespec.h>
 
 #include <errno.h>
+#include <math.h>
 #include <time.h>
+#include <unistd.h>
 
 /* Sleep until the time (call it WAKE_UP_TIME) specified as
    SECONDS seconds after the time this function is called.
@@ -37,21 +39,33 @@
 int
 xnanosleep (double seconds)
 {
-  struct timespec ts_sleep = dtotimespec (seconds);
-
-  for (;;)
+  if (!isfinite(seconds) && seconds > 0)
+    {
+      for (;;)
+        {
+          pause();
+          if (errno != EINTR)
+            return -1;
+        }
+    }
+  else
     {
-      /* Linux-2.6.8.1's nanosleep returns -1, but doesn't set errno
-         when resumed after being suspended.  Earlier versions would
-         set errno to EINTR.  nanosleep from linux-2.6.10, as well as
-         implementations by (all?) other vendors, doesn't return -1
-         in that case;  either it continues sleeping (if time remains)
-         or it returns zero (if the wake-up time has passed).  */
-      errno = 0;
-      if (nanosleep (&ts_sleep, NULL) == 0)
-        break;
-      if (errno != EINTR && errno != 0)
-        return -1;
+      struct timespec ts_sleep = dtotimespec (seconds);
+
+      for (;;)
+        {
+          /* Linux-2.6.8.1's nanosleep returns -1, but doesn't set errno
+             when resumed after being suspended.  Earlier versions would
+             set errno to EINTR.  nanosleep from linux-2.6.10, as well as
+             implementations by (all?) other vendors, doesn't return -1
+             in that case;  either it continues sleeping (if time remains)
+             or it returns zero (if the wake-up time has passed).  */
+          errno = 0;
+          if (nanosleep (&ts_sleep, NULL) == 0)
+            break;
+          if (errno != EINTR && errno != 0)
+            return -1;
+        }
     }
 
   return 0;
-- 
2.25.0

Re: [PATCH 0/1] xnanosleep: Use pause for infinite duration.

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 1110 bytes --]

On 2/10/20 4:26 AM, Vladimir Panteleev wrote:

> I'm not sure if isfinite is the best way to check for infinity, as far
> as the tradeoff between portability and dependencies goes.

We can use the same test that dtotimespec uses, since we want to sleep forever 
not only if the argument is infinity, but also if it exceeds the maximum 
possible timespec value. So there's no need for the isfinite portability baggage.

> By the way, shouldn't xnanosleep be using the second nanosleep
> parameter to track how much time there's left to sleep in case of an
> interruption?

I vaguely recall not doing that because some old kernels messed up when setting 
the time remaining. They're long obsolete now, so it's probably not worth 
worrying about. If I'm wrong and it is an issue it should be fixed in the 
nanosleep module anyway.

I installed the attached, which is a bit more conservative than your suggestion, 
as it doesn't assume that 'pause' exists or that it indeed sleeps forever 
(though I know of no counterexamples, there's little difficulty in relaxing 
those assumptions). Thanks for the bug report.

[-- Attachment #2: 0001-xnanosleep-prefer-pause-and-get-remaining-time.patch --]
[-- Type: text/x-patch, Size: 3884 bytes --]

From f3243f20107435f620338aa15e1fdd5779d639fe Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 16 Feb 2020 12:47:52 -0800
Subject: [PATCH] xnanosleep: prefer pause, and get remaining time

Problem reported by Vladimir Panteleev in:
https://lists.gnu.org/r/bug-gnulib/2020-02/msg00052.html
* lib/xnanosleep.c: Include intprops.h, unistd.h.
(xnanosleep) [HAVE_PAUSE]: Prefer pause when sleeping infinitely.
(xnanosleep): Obtain remaining time when nanosleep is interrupted.
* m4/xnanosleep.m4 (gl_XNANOSLEEP): Check for 'pause'.
* modules/xnanosleep (Depends-on): Add intprops, unistd.
---
 ChangeLog          | 11 +++++++++++
 lib/xnanosleep.c   | 22 ++++++++++++++++++++--
 m4/xnanosleep.m4   |  4 ++--
 modules/xnanosleep |  2 ++
 4 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index beacc0926..4e3f51ba2 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2020-02-16  Paul Eggert  <eggert@cs.ucla.edu>
+
+	xnanosleep: prefer pause, and get remaining time
+	Problem reported by Vladimir Panteleev in:
+	https://lists.gnu.org/r/bug-gnulib/2020-02/msg00052.html
+	* lib/xnanosleep.c: Include intprops.h, unistd.h.
+	(xnanosleep) [HAVE_PAUSE]: Prefer pause when sleeping infinitely.
+	(xnanosleep): Obtain remaining time when nanosleep is interrupted.
+	* m4/xnanosleep.m4 (gl_XNANOSLEEP): Check for 'pause'.
+	* modules/xnanosleep (Depends-on): Add intprops, unistd.
+
 2020-02-16  Bruno Haible  <bruno@clisp.org>
 
 	lchmod: Improve cross-compilation guess.
diff --git a/lib/xnanosleep.c b/lib/xnanosleep.c
index 5774f75f3..8619c6df6 100644
--- a/lib/xnanosleep.c
+++ b/lib/xnanosleep.c
@@ -22,10 +22,12 @@
 
 #include "xnanosleep.h"
 
+#include <intprops.h>
 #include <timespec.h>
 
 #include <errno.h>
 #include <time.h>
+#include <unistd.h>
 
 /* Sleep until the time (call it WAKE_UP_TIME) specified as
    SECONDS seconds after the time this function is called.
@@ -37,6 +39,17 @@
 int
 xnanosleep (double seconds)
 {
+#if HAVE_PAUSE
+  if (1.0 + TYPE_MAXIMUM (time_t) <= seconds)
+    {
+      do
+        pause ();
+      while (errno == EINTR);
+
+      /* pause failed (!); fall back on repeated nanosleep calls.  */
+    }
+#endif
+
   struct timespec ts_sleep = dtotimespec (seconds);
 
   for (;;)
@@ -46,9 +59,14 @@ xnanosleep (double seconds)
          set errno to EINTR.  nanosleep from linux-2.6.10, as well as
          implementations by (all?) other vendors, doesn't return -1
          in that case;  either it continues sleeping (if time remains)
-         or it returns zero (if the wake-up time has passed).  */
+         or it returns zero (if the wake-up time has passed).
+
+         Gnulib's replacement nanosleep sometimes does not update
+         TS_SLEEP, and it is possible some kernels have a similar bug.
+         However, this merely causes xnanosleep to sleep longer than
+         necessary, which is not a correctness bug.  */
       errno = 0;
-      if (nanosleep (&ts_sleep, NULL) == 0)
+      if (nanosleep (&ts_sleep, &ts_sleep) == 0)
         break;
       if (errno != EINTR && errno != 0)
         return -1;
diff --git a/m4/xnanosleep.m4 b/m4/xnanosleep.m4
index 5f305f864..88454ae01 100644
--- a/m4/xnanosleep.m4
+++ b/m4/xnanosleep.m4
@@ -1,4 +1,4 @@
-#serial 5
+#serial 6
 dnl Copyright (C) 2005-2006, 2009-2020 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -8,5 +8,5 @@ dnl Written by Paul Eggert.
 
 AC_DEFUN([gl_XNANOSLEEP],
 [
-  :
+  AC_CHECK_FUNCS_ONCE([pause])
 ])
diff --git a/modules/xnanosleep b/modules/xnanosleep
index 0a2b373ea..cc9069cb2 100644
--- a/modules/xnanosleep
+++ b/modules/xnanosleep
@@ -8,8 +8,10 @@ m4/xnanosleep.m4
 
 Depends-on:
 dtotimespec
+intprops
 nanosleep
 time
+unistd
 
 configure.ac:
 gl_XNANOSLEEP
-- 
2.17.1

Re: [PATCH 0/1] xnanosleep: Use pause for infinite duration.

[Vladimir Panteleev]

On 16/02/2020 20.55, Paul Eggert wrote:
> I installed the attached, which is a bit more conservative than your
> suggestion, as it doesn't assume that 'pause' exists or that it indeed
> sleeps forever (though I know of no counterexamples, there's little
> difficulty in relaxing those assumptions). Thanks for the bug report.

Excellent, thank you!

-- 
Best regards,
 Vladimir