Re: [PATCH] Add timeout utility

[Jim Meyering]

Thanks!

Pádraig Brady <address@hidden> wrote:
> Subject: [PATCH] Add new program: timeout
...
> +/* Given an integer value *X, and a suffix character, SUFFIX_CHAR,
> +   scale *X by the multiplier implied by SUFFIX_CHAR.  SUFFIX_CHAR may
> +   be the NUL byte or `s' to denote seconds, `m' for minutes, `h' for
> +   hours, or `d' for days.  If SUFFIX_CHAR is invalid, don't modify *X
> +   and return false.  If *X would overflow, don't modify *X and return false.
> +   Otherwise return true.  */
> +
> +static bool
> +apply_suffix (unsigned int *x, char suffix_char)

You probably expect this, but I have to say it ;-)
Don't duplicate code!

apply_suffix is a tiny function, and used also by sleep.c, so let's not
duplicate it.  How about putting it (static inline) in system.h instead?

operand2sig is similar but not as lightweight, since it uses str2sig,
error, xstrdup and W* macros.  Maybe put it in its own file in src/?
Or change xstrdup to strdup, eliminate the error call, and consider
putting it in lib/gnulib.  It's probably better to go the easier
route and use a separate file in src/....[ch].

Rather than '???' (or in addition to), please mark such
comments with "FIXME".

...
> +  /* Setup handlers before fork() so that we
> +   * handle any signals caused by child, without races.  */
> +  signal (SIGALRM, cleanup);    /* our timeout.  */
> +  signal (SIGINT, cleanup);     /* Ctrl-C at terminal for example.  */
> +  signal (SIGQUIT, cleanup);    /* Ctrl-\ at terminal for example.  */
> +  signal (SIGTERM, cleanup);    /* if we're killed, stop monitored process.  
> */

Mike Frysinger's point is a good one: use signal only if absolutely
necessary.  Prefer sigaction.

Most signal-handling code in coreutils uses sigaction, and falls back on
signal ifndef SA_NOCLDSTOP.  But the ifdefs and code duplication are
pretty ugly.  If you're game, I'd like to try using *only* sigaction for
an initial test release, in order to see if any "reasonable portability
targets" remain that lack sigaction support.  Then, if there are no
build failure reports for a long enough period, we can remove the crufty
signal-using #else blocks in a bunch of programs.