Re: [PATCH] Add timeout utility

[Pádraig Brady]

Jim Meyering wrote:
> 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!

of course.

> 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?

sleep's apply_suffic() works on floats where as timeout's work on ints.
I'll see if I can merge them somewhat.
Hmm I could allow float input to timeout (1.5d for example),
and then round this to the nearest second later?

> 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].

I intended but forgot to add the comment:
/* FIXME: where will we refactor this to? */
I'll add a new src/ file so.

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

righto

>> +  /* 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.


Ok, I'll assume that sigaction is available.

thanks,
Pádraig.