Re: timeout.c

[Bob Proulx]

Robert Millan [ackstorm] wrote:
> [ Please keep address@hidden on CC ]
> 
> Since the program is small, I decided to rewrite it from scratch.  I think
> it would be nice to have it in GNU coreutils.  Would you please consider it?

I am undecided.  It really would be up to the maintainers to decide.
This utility would need a pretty significant effort to add help and
localization and tests and all of the infrastructure in order to get
it up to the level needed for the coreutils project.  It would
probably be better to start off smaller and allow it to grow and
mature.  That would be easier than trying to do it all at once.  So I
am leaning toward saying that it would be better waiting.  I don't
feel strongly about it though.

But I have some comments on the code regardless.

> /*
>  *  timeout.c -- run commands with a specified timeout
>  *  Copyright (C) 2007 Robert Millan <address@hidden>
>  *
>  *  GRUB is free software; you can redistribute it and/or modify

Grub?

>  *  it under the terms of the GNU General Public License as published by
>  *  the Free Software Foundation; either version 3 of the License, or
>  *  (at your option) any later version.
>  *
>  *  This program is distributed in the hope that it will be useful,
>  *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>  *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>  *  GNU General Public License for more details.
>  *
>  *  You should have received a copy of the GNU General Public License
>  *  along with GRUB; if not, write to the Free Software

Grub!

>  *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
>  */
> 
> #include <getopt.h>
> #include <signal.h>
> #include <stdio.h>
> #include <stdlib.h>
> 
> #include <sys/wait.h>
> 
> #define PROGRAM_NAME          "timeout"
> #define PROGRAM_VERSION               "0.0.1"
> 
> int pid;
> int signum = SIGTERM;
> int timeout_status = 1;

Please comment the use of the timeout_status variable.

> static struct option options[] =
>   {
>     {"signal", required_argument, 0, 's'}, 

Please trim whitespace from the file.  There is a hanging space on
this line.

>     {"status", required_argument, 0, 't'},
>     {"help", no_argument, 0, 'h'},
>     {"version", no_argument, 0, 'v'},
>     {0, 0, 0, 0}
>   };
> 
> static void
> usage (int status)
> {
>   if (status)

I think this use of usage with an argument to make it do first one
thing and then another thing is pernicious.  This routine really needs
to be split into dedicated functions.

>     fprintf (stderr, "Try ``" PROGRAM_NAME " --help'' for more 
> information.\n");

There are two many sets of quotes there.  They are doubled.

>   else
>     printf ("\
> Usage: " PROGRAM_NAME " [OPTION]... NUMBER COMMAND ARGUMENTS\n\
> Runs COMMAND with ARGUMENTS, and kills it after NUMBER seconds.\n\
> \n\
>   -s, --signal SIGNUM use SIGNUM to kill our child\n\

That reads strangely in english.  I think this would read better.

  -s, --signal=SIGNUM   use SIGNUM to kill the child process\n\

>   -t, --status NUMBER after killing our child, exit with NUMBER\n\

Huh?  What?  Huh?  This does not match the code.

>   -h, --help          display this message and exit\n\
>   -v, --version               print version information and exit\n\
> \n");
> 
>   exit (status);
> }
> 
> void
> handler (int dummy)
> {
>   kill (signum, pid);
>   exit (timeout_status);
> }

I think two signal catchers need to be registered.  One for sigalarm
and one for other signals.  This way if the timeout watcher is killed
with a signal it will be passed through to the child process.

> main (int argc, char **argv)

Missing return type of main.

> {
>   int timeout;
>   char *tail;
> 
>   /* Check for options.  */
>   while (1)
>     {
>       int c = getopt_long (argc, argv, "hs:t:v", options, 0);
> 
>       if (c == -1)
>         break;
> 
>       switch (c)
>         {
>           case 'h':

I would not burn '-h' as a help option.  Using '--help' is sufficient
and would leave '-h' available.

>             usage (0);
>           case 'v':
>             printf (PROGRAM_NAME " " PROGRAM_VERSION "\n");
>             exit (0);
>             break;
>           case 's':
>             signum = strtol (optarg, &tail, 10);
>             if (tail[0] != '\0')
>               usage (1);
>             break;
>           case 't':
>             timeout_status = strtol (optarg, &tail, 10);
>             if (tail[0] != '\0')
>               usage (1);
>             break;
>           default:
>             usage (1);
>         }
>     }
> 
>   /* Ditch getopt args -- no longer needed */
>   argv += optind;
> 
>   if (argv[0] == NULL)
>     usage (1);
> 
>   timeout = strtol (argv[0], &tail, 10);
>   if (tail[0] != '\0')
>     usage (1);
> 
>   /* Ditch timeout -- no longer needed */
>   argv++;
> 
>   signal (SIGALRM, handler);

Signals should be set with sigaction() these days.

>   pid = fork ();
> 
>   switch (pid)
>     {
>       case -1:
>         perror ("fork");
>         exit (1);
>       case 0:
>         execvp (argv[0], argv);
>         perror ("execvp");
>         exit (1);
>       default:
>         {
>           int child_status;
>           alarm (timeout);
>           while (waitpid (pid, &child_status, WNOHANG) == 0);

This loops at cpu speed waiting for the child process.  Of course that
is not acceptable and is a complete showstopper.

>           exit (child_status);
>         }
>     }
> }