GNU bug report logs - #9101
timeout should use setitimer if available

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 9101 in the body.
You can then email your comments to 9101 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: bug-coreutils <at> gnu.org
Subject: timeout should use setitimer if available
Date: Sat, 16 Jul 2011 12:28:40 -0700

setitimer has nanosecond resolution, which is better than the
one-second resolution that 'alarm' has.  timeout should use
setitimer if available, to take advantage of this.  On 64-bit
hosts, this has the additional advantage of increasing the
upper bound for timeouts from 2**31 seconds to 2**63 seconds
(about 68 years to about 292 billion years, which should be
long enough for most practical purposes :-).

Message #8 received at 9101 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 9101 <at> debbugs.gnu.org
Subject: Re: bug#9101: timeout should use setitimer if available
Date: Sun, 17 Jul 2011 11:59:00 +0200

Paul Eggert wrote:
> setitimer has nanosecond resolution, which is better than the
> one-second resolution that 'alarm' has.  timeout should use
> setitimer if available, to take advantage of this.  On 64-bit
> hosts, this has the additional advantage of increasing the
> upper bound for timeouts from 2**31 seconds to 2**63 seconds
> (about 68 years to about 292 billion years, which should be
> long enough for most practical purposes :-).

I like the idea of supporting a sub-second timeout interval, but it
probably deserves a warning in the documentation.  Even a command like
"timeout 3 sleep 1" will timeout on a system under heavy load.

Message #11 received at 9101 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 9101 <at> debbugs.gnu.org
Subject: Re: bug#9101: timeout should use setitimer if available
Date: Mon, 18 Jul 2011 11:01:06 +0100

[Message part 1 (text/plain, inline)]

On 16/07/11 20:28, Paul Eggert wrote:
> setitimer has nanosecond resolution, which is better than the
> one-second resolution that 'alarm' has.  timeout should use
> setitimer if available, to take advantage of this.  On 64-bit
> hosts, this has the additional advantage of increasing the
> upper bound for timeouts from 2**31 seconds to 2**63 seconds
> (about 68 years to about 292 billion years, which should be
> long enough for most practical purposes :-).

I'll apply this soon.

cheers,
Pádraig.

[timeout-float.diff (text/x-patch, attachment)]

Message #14 received at 9101 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 9101 <at> debbugs.gnu.org
Subject: Re: bug#9101: timeout should use setitimer if available
Date: Mon, 18 Jul 2011 09:44:08 -0700

On 07/18/11 03:01, Pádraig Brady wrote:
> I'll apply this soon.

Thanks for doing that. Some comments:

I see that my bug report was incoherent, as it was talking about
nanosecond resolution and setitimer, when I meant to be writing about
timer_settime (which *does* have nanosecond resolution).  Sorry about
that.

POSIX says that setitimer is obsolescent, and that applications should use
timer_settime.  How about if we add a gnulib module for timer_settime
and have 'timeout' use that instead?  (This could be a separate patch,
done later.)

   +  struct timespec ts = dtotimespec (duration);
   +  tv.tv_sec = MIN (ts.tv_sec, LONG_MAX); /* Assuming tv_sec is positive.  */

That second statement should be removed.  time_t is not necessarily
the same as 'long', and setitimer should work with values longer than
LONG_MAX, if time_t is wider than 'long'.  And parse_duration when
combined with dtotimespec guarantees that tv_sec is nonnegative here
(it might be zero), so that comment needs to be removed too.

   +      if (tv.tv_sec != LONG_MAX)

This LONG_MAX should be TYPE_MAXIMUM (time_t).

Of course all of this rounding-and-checking-for-overflow business
goes away once we go to timer_settime....

Message #17 received at 9101 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 9101 <at> debbugs.gnu.org
Subject: Re: bug#9101: timeout should use setitimer if available
Date: Mon, 18 Jul 2011 23:17:25 +0100

On 18/07/11 17:44, Paul Eggert wrote:
> On 07/18/11 03:01, Pádraig Brady wrote:
>> I'll apply this soon.
> 
> Thanks for doing that. Some comments:
> 
> I see that my bug report was incoherent, as it was talking about
> nanosecond resolution and setitimer, when I meant to be writing about
> timer_settime (which *does* have nanosecond resolution).  Sorry about
> that.
> 
> POSIX says that setitimer is obsolescent, and that applications should use
> timer_settime.  How about if we add a gnulib module for timer_settime
> and have 'timeout' use that instead?  (This could be a separate patch,
> done later.)

Hmm. We don't need the extra resolution or functionality of timer_settime().
So I prefer the simplier setitimer() interface TBH.
Currently, setitimer should be more portable too, I guess.
No rush with timer_settime() I think.

> 
>    +  struct timespec ts = dtotimespec (duration);
>    +  tv.tv_sec = MIN (ts.tv_sec, LONG_MAX); /* Assuming tv_sec is positive.  */
> 
> That second statement should be removed.  time_t is not necessarily
> the same as 'long', and setitimer should work with values longer than
> LONG_MAX, if time_t is wider than 'long'. 

Bah, I was confused by the linux setitimer man page
which explicitly stated 'long' was used for setitimer.
Also the value on freebsd is truncated to 100000000.

> And parse_duration when
> combined with dtotimespec guarantees that tv_sec is nonnegative here
> (it might be zero), so that comment needs to be removed too.

Well the comment was confirming that for the purpose of that calculation :)

> 
>    +      if (tv.tv_sec != LONG_MAX)
> 
> This LONG_MAX should be TYPE_MAXIMUM (time_t).

Could it ever be 'long', and that being different to time_t?
I suppose we could use _GL_INT_MAXIMUM (tv.tv_sec),
but I'll assume time_t is always used for the moment.
I'll use the following:

static void
settimeout (double duration)
{
  struct timeval tv;
  struct timespec ts = dtotimespec (duration);
  tv.tv_usec = (ts.tv_nsec + 999) / 1000;
  if (tv.tv_usec == 1000 * 1000)
    {
      if (tv.tv_sec != TYPE_MAXIMUM (time_t))
        {
          tv.tv_sec++;
          tv.tv_usec = 0;
        }
      else
        tv.tv_usec--;
    }
  struct itimerval it = { {0, 0}, tv };
  if (setitimer (ITIMER_REAL, &it, NULL) == -1)
    error (EXIT_FAILURE, errno, _("error setting timer"));
}

> Of course all of this rounding-and-checking-for-overflow business
> goes away once we go to timer_settime....

Talking about overflow, for my own reference
I noticed that there was some info lost
in the double -> 64bit conversion:

$ strace -e setitimer src/timeout 9223372036852775425 sleep 0
setitimer(ITIMER_REAL, {it_interval={0, 0}, it_value={9223372036852775936, 0}}, NULL) = 0

Not a practical issue for `timeout` at least.

I'm marking this bug done now.

thanks for the review,
Pádraig.

Message #20 received at 9101 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 9101 <at> debbugs.gnu.org
Subject: Re: bug#9101: timeout should use setitimer if available
Date: Mon, 18 Jul 2011 16:05:44 -0700

On 07/18/11 15:17, Pádraig Brady wrote:
> I noticed that there was some info lost
> in the double -> 64bit conversion:
> 
> $ strace -e setitimer src/timeout 9223372036852775425 sleep 0
> setitimer(ITIMER_REAL, {it_interval={0, 0}, it_value={9223372036852775936, 0}}, NULL) = 0

The info is lost in converting string to double.
IEEE-754 double can represent integers exactly up through
2**52; then it starts losing information.  We could
work around this on x86 by going to long double, but...

> Not a practical issue for `timeout` at least.

Exactly.

>>    +      if (tv.tv_sec != LONG_MAX)
>> > 
>> > This LONG_MAX should be TYPE_MAXIMUM (time_t).
> Could it ever be 'long', and that being different to time_t?

Not on any host I know of.  Historically tv_sec has always
been of type time_t, and POSIX requires it; I don't expect
this to change.

> We don't need the extra resolution

This is true in a practical sense, but it'd be nice to do it
"right", if only for the future machines that can really
do sub-microsecond timing accurately.  Obviously this is low
priority though.

Message #23 received at 9101 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 9101 <at> debbugs.gnu.org
Subject: Re: bug#9101: timeout should use setitimer if available
Date: Tue, 19 Jul 2011 12:00:23 +0100

On 18/07/11 23:17, Pádraig Brady wrote:
> On 18/07/11 17:44, Paul Eggert wrote:
>> On 07/18/11 03:01, Pádraig Brady wrote:
>>> I'll apply this soon.
>>
>> Thanks for doing that. Some comments:
>>
>> I see that my bug report was incoherent, as it was talking about
>> nanosecond resolution and setitimer, when I meant to be writing about
>> timer_settime (which *does* have nanosecond resolution).  Sorry about
>> that.
>>
>> POSIX says that setitimer is obsolescent, and that applications should use
>> timer_settime.  How about if we add a gnulib module for timer_settime
>> and have 'timeout' use that instead?  (This could be a separate patch,
>> done later.)
> 
> Hmm. We don't need the extra resolution or functionality of timer_settime().
> So I prefer the simplier setitimer() interface TBH.
> Currently, setitimer should be more portable too, I guess.
> No rush with timer_settime() I think.

Note the main current platform missing timer_settimer is darwin.

Using timer_settimer isn't that onerous actually.
The gnulib check could be lumped into clock_time, like:

diff --git a/m4/clock_time.m4 b/m4/clock_time.m4
index 3c08512..344d330 100644
--- a/m4/clock_time.m4
+++ b/m4/clock_time.m4
@@ -4,7 +4,7 @@ dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
 dnl with or without modifications, as long as this notice is preserved.

-# Check for clock_gettime and clock_settime, and set LIB_CLOCK_GETTIME.
+# Check for clock_[gs]ettime and timer_settime, and set LIB_CLOCK_GETTIME.
 # For a program named, say foo, you should add a line like the following
 # in the corresponding Makefile.am file:
 # foo_LDADD = $(LDADD) $(LIB_CLOCK_GETTIME)
@@ -26,6 +26,6 @@ AC_DEFUN([gl_CLOCK_TIME],
     AC_SEARCH_LIBS([clock_gettime], [rt posix4],
                    [test "$ac_cv_search_clock_gettime" = "none required" ||
                     LIB_CLOCK_GETTIME=$ac_cv_search_clock_gettime])
-    AC_CHECK_FUNCS([clock_gettime clock_settime])
+    AC_CHECK_FUNCS([clock_gettime clock_settime timer_settime])
   LIBS=$gl_saved_libs
 ])


And coreutils could use it like:

diff --git a/src/Makefile.am b/src/Makefile.am
index ef0e7a4..76cee0d 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -329,6 +329,7 @@ date_LDADD += $(LIB_CLOCK_GETTIME)
 ginstall_LDADD += $(LIB_CLOCK_GETTIME)
 ls_LDADD += $(LIB_CLOCK_GETTIME)
 pr_LDADD += $(LIB_CLOCK_GETTIME)
+timeout_LDADD += $(LIB_CLOCK_GETTIME)
 touch_LDADD += $(LIB_CLOCK_GETTIME)

 # for gethrxtime
diff --git a/src/timeout.c b/src/timeout.c
index bfca548..8b5ff8d 100644
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -105,7 +105,15 @@ static struct option const long_options[] =
 static void
 settimeout (double duration)
 {
-#if HAVE_SETITIMER
+#if HAVE_TIMER_SETTIME
+  struct timespec ts = dtotimespec (duration);
+  struct itimerspec its = { {0, 0}, ts };
+  timer_t timerid;
+  if (timer_create (CLOCK_REALTIME, NULL, &timerid) == -1)
+    error (EXIT_FAILURE, errno, _("error in timer_create"));
+  if (timer_settime (timerid, 0, &its, NULL) == -1)
+    error (EXIT_FAILURE, errno, _("error in timer_settime"));
+#elif HAVE_SETITIMER
   struct timeval tv;
   struct timespec ts = dtotimespec (duration);
   tv.tv_usec = (ts.tv_nsec + 999) / 1000;

We could remove the setitimer stuff altogether and
just support 1 second resolution on darwin et. al.
That's by far the most common use case anyway.

cheers,
Pádraig.

Message #26 received at 9101 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 9101 <at> debbugs.gnu.org
Subject: Re: bug#9101: timeout should use setitimer if available
Date: Tue, 19 Jul 2011 11:45:59 -0700

On 07/19/11 04:00, Pádraig Brady wrote:

> +  if (timer_create (CLOCK_REALTIME, NULL, &timerid) == -1)
> +    error (EXIT_FAILURE, errno, _("error in timer_create"));
> +  if (timer_settime (timerid, 0, &its, NULL) == -1)
> +    error (EXIT_FAILURE, errno, _("error in timer_settime"));

A minor point: the usual (more-conservative, and often-faster) style
in coreutils is to write "foo (...) != 0" rather than "foo (...) ==
-1" for system calls that return 0 or -1.

> We could remove the setitimer stuff altogether and
> just support 1 second resolution on darwin et. al.
> That's by far the most common use case anyway.

That sounds good to me.

Another thought, to make the code more bulletproof, is to fall back on
alarm if timer_create and timer_settime fail due to ENOTSUP or EAGAIN
or whatever.

Something like this (untested) code:

/* Start the timeout after which we'll receive a SIGALRM.
   Round DURATION up to the next representable value.
   Treat out-of-range values as if they were maximal,
   as that's more useful in practice than reporting an error.
   '0' means don't timeout.  */
static void
settimeout (double duration)
{
#if HAVE_TIMER_SETTIME
  /* Prefer timer_settime if it works, as it's higher-resolution.  */
  struct timespec ts = dtotimespec (duration);
  struct itimerspec its = { {0, 0}, ts };
  timer_t timerid;
  if (timer_create (CLOCK_REALTIME, NULL, &timerid) == 0)
    {
      if (timer_settime (timerid, 0, &its, NULL) == 0)
	return;
      timer_delete (timerid);
    }
#endif

  if (UINT_MAX <= duration)
    alarm (UINT_MAX);
  else
    {
      unsigned int duration_floor = duration;
      alarm (duration_floor + (duration_floor < duration));
    }
}



> The gnulib check could be lumped into clock_time, like:

Yes, that would work, but the clock-time module probably should stay
decoupled from timer_settime.  How about the following (untested)
patch instead?  The idea is to append "timeout_LDADD +=
$(LIB_TIMER_SETTIME)" to src/Makefile.am.

diff --git a/m4/jm-macros.m4 b/m4/jm-macros.m4
index 9bb6fa4..c32cbd3 100644
--- a/m4/jm-macros.m4
+++ b/m4/jm-macros.m4
@@ -43,6 +43,11 @@ AC_DEFUN([coreutils_MACROS],
 
   # used by ls
   AC_REQUIRE([gl_CLOCK_TIME])
+  coreutils_lib_pattern='-l\(.*\)'
+  coreutils_lib_rt=`
+    expr "X$ac_cv_search_clock_gettime" : "X$coreutils_lib_pattern"
+  `
+
   # used by shred
   AC_CHECK_FUNCS_ONCE([directio])
 
@@ -97,13 +102,24 @@ AC_DEFUN([coreutils_MACROS],
   # for dd.c and shred.c
   coreutils_saved_libs=$LIBS
     LIB_FDATASYNC=
-    AC_SEARCH_LIBS([fdatasync], [rt posix4],
+    AC_SEARCH_LIBS([fdatasync], [$coreutils_lib_rt],
                    [test "$ac_cv_search_fdatasync" = "none required" ||
                     LIB_FDATASYNC=$ac_cv_search_fdatasync])
     AC_SUBST([LIB_FDATASYNC])
     AC_CHECK_FUNCS([fdatasync])
   LIBS=$coreutils_saved_libs
 
+  # used by timeout.c
+  AC_SUBST([LIB_TIMER_SETTIME])
+  coreutils_saved_libs=$LIBS
+    LIB_TIMER_SETTIME=
+    AC_SEARCH_LIBS([timer_settime], [$coreutils_lib_rt],
+                   [test "$ac_cv_search_timer_settime" = "none required" ||
+                    LIB_TIMER_SETTIME=$ac_cv_search_timer_settime])
+    AC_SUBST([LIB_TIMER_SETTIME])
+    AC_CHECK_FUNCS([timer_settim])
+  LIBS=$coreutils_saved_libs
+
   # Check whether libcap is usable -- for ls --color support
   LIB_CAP=
   AC_ARG_ENABLE([libcap],

Message #29 received at 9101 <at> debbugs.gnu.org (full text, mbox):

From: SciFi <sci-fi <at> hush.ai>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: Paul Eggert <eggert <at> cs.ucla.edu>, 9101 <at> debbugs.gnu.org
Subject: Re: bug#9101: timeout should use setitimer if available
Date: Tue, 19 Jul 2011 14:01:34 -0500

[Message part 1 (text/plain, inline)]

Hi,

On Jul 19, 2011, at 06:00, Pádraig Brady wrote:
> 
> We could remove the setitimer stuff altogether and
> just support 1 second resolution on darwin et. al.
> That's by far the most common use case anyway.

If I may, since I am an OSX user [1] [2] ,
I cast my "vote" here.  ;)

[1] I am planning to use the 'timeout' CLI as part of a
    simple PVR app in conjunction with HDHomeRun® tuners.
    1s-resolution is plenty, if I can also trust the
    age-old 'at'/'cron' mechanisms, too.  ;)

[2] OT: I am also planning to "jump ship", and have
    posted a big write-up on why I've decided this on
    the pan-users list (my current main project):
    <https://lists.nongnu.org/archive/html/pan-users/2011-07/msg00043.html>
    I'd like to invite anyone to comment in that thread.

[PGP.sig (application/pgp-signature, inline)]

Message #32 received at 9101 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: bug-gnulib <bug-gnulib <at> gnu.org>, 9101 <at> debbugs.gnu.org
Subject: Re: bug#9101: timeout should use setitimer if available
Date: Tue, 19 Jul 2011 23:24:48 +0100

[Message part 1 (text/plain, inline)]

On 19/07/11 19:45, Paul Eggert wrote:
> On 07/19/11 04:00, Pádraig Brady wrote:
> 
>> +  if (timer_create (CLOCK_REALTIME, NULL, &timerid) == -1)
>> +    error (EXIT_FAILURE, errno, _("error in timer_create"));
>> +  if (timer_settime (timerid, 0, &its, NULL) == -1)
>> +    error (EXIT_FAILURE, errno, _("error in timer_settime"));
> 
> A minor point: the usual (more-conservative, and often-faster) style
> in coreutils is to write "foo (...) != 0" rather than "foo (...) ==
> -1" for system calls that return 0 or -1.
> 
>> We could remove the setitimer stuff altogether and
>> just support 1 second resolution on darwin et. al.
>> That's by far the most common use case anyway.
> 

OK I'll do that, encompassing your fallback suggestion.

> 
>> The gnulib check could be lumped into clock_time, like:
> 
> Yes, that would work, but the clock-time module probably should stay
> decoupled from timer_settime.  How about the following (untested)
> patch instead?  The idea is to append "timeout_LDADD +=
> $(LIB_TIMER_SETTIME)" to src/Makefile.am.
> 
> diff --git a/m4/jm-macros.m4 b/m4/jm-macros.m4

I'd rather not add this into jm-macros.
How about just adding a module to gnulib
where others might find it useful too?
Proposed module attached.

cheers,
Pádraig.

[timer-time.diff (text/x-patch, attachment)]

Message #35 received at 9101 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: bug-gnulib <bug-gnulib <at> gnu.org>, 9101 <at> debbugs.gnu.org
Subject: Re: bug#9101: timeout should use setitimer if available
Date: Tue, 19 Jul 2011 15:46:39 -0700

On 07/19/11 15:24, Pádraig Brady wrote:
> How about just adding a module to gnulib
> where others might find it useful too?

Yes, that's better, thanks.  Could you please add
Jim and me to the maintainer list?

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.