Date: Wed, 16 Jul 2014 14:31:28 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Baptiste Daroussin <bapt@FreeBSD.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r268745 - in head/usr.bin: . timeout Message-ID: <20140716113128.GV93733@kib.kiev.ua> In-Reply-To: <201407160955.s6G9taro084054@svn.freebsd.org> References: <201407160955.s6G9taro084054@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--LhGscpP45fjEbabV
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Wed, Jul 16, 2014 at 09:55:36AM +0000, Baptiste Daroussin wrote:
> +#include <sys/types.h>
> +#include <sys/time.h>
> +#include <sys/wait.h>
> +#include <signal.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sysexits.h>
> +#include <unistd.h>
> +#include <getopt.h>
> +#include <err.h>
> +#include <spawn.h>
Is this header needed ?
The headers are unordered.
> +#include <errno.h>
> +#include <stdbool.h>
> +
> +#define EXIT_TIMEOUT 124
> +
> +static sig_atomic_t sig_chld =3D 0;
> +static sig_atomic_t sig_term =3D 0;
> +static sig_atomic_t sig_alrm =3D 0;
> +static sig_atomic_t sig_ign =3D 0;
> +
> +static void
> +usage(void)
> +{
There should be empty line after '{' if no local vars are declared.
> + fprintf(stderr, "Usage: %s [--signal sig | -s sig] [--preserve-status]"
> + " [--kill-after time | -k time] [--foreground] <duration> <command>"
> + " <arg ...>\n", getprogname());
> +
> + exit(EX_USAGE);
> +}
> +
> +static double
> +parse_duration(const char *duration)
> +{
> + double ret;
> + char *end;
> +
> + ret =3D strtod(duration, &end);
> + if (ret =3D=3D 0 && end =3D=3D duration)
> + errx(EXIT_FAILURE, "invalid duration");
> +
> + if (end =3D=3D NULL || *end =3D=3D '\0')
> + return (ret);
> +
> + if (end !=3D NULL && *(end + 1) !=3D '\0')
> + errx(EX_USAGE, "invalid duration");
> +
> + switch (*end) {
> + case 's':
> + break;
> + case 'm':
> + ret *=3D 60;
> + break;
> + case 'h':
> + ret *=3D 60 * 60;
> + break;
> + case 'd':
> + ret *=3D 60 * 60 * 24;
> + break;
> + default:
> + errx(EX_USAGE, "invalid duration");
> + }
> +=09
> + if (ret < 0 || ret >=3D 100000000UL)
> + errx(EX_USAGE, "invalid duration");
> +
> + return (ret);
> +}
> +
> +static int
> +parse_signal(const char *str)
> +{
> + int sig, i;
> + const char *err;
> +
> + sig =3D strtonum(str, 0, sys_nsig, &err);
> +
> + if (err =3D=3D NULL)
> + return (sig);
> + if (strncasecmp(str, "SIG", 3) =3D=3D 0)
> + str +=3D 3;
> +
> + for (i =3D 1; i < sys_nsig; i++) {
> + if (strcasecmp(str, sys_signame[i]) =3D=3D 0)
> + return (i);
> + }
> +=09
> + errx(EX_USAGE, "invalid signal");
> +}
> +
> +static void
> +sig_handler(int signo)
> +{
> + if (sig_ign !=3D 0 && signo =3D=3D sig_ign) {
> + sig_ign =3D 0;
> + return;
> + }
> +
> + switch(signo) {
> + case 0:
> + case SIGINT:
> + case SIGHUP:
> + case SIGQUIT:
> + case SIGTERM:
> + sig_term =3D signo;
> + break;
> + case SIGCHLD:
> + sig_chld =3D 1;
> + break;
> + case SIGALRM:
> + sig_alrm =3D 1;
> + break;
> + }
> +}
> +
> +static void
> +set_interval(double iv)
> +{
> + struct itimerval tim;
> +
> + memset(&tim, 0, sizeof(tim));
> + tim.it_value.tv_sec =3D (time_t)iv;
> + iv -=3D (time_t)iv;
> + tim.it_value.tv_usec =3D (suseconds_t)(iv * 1000000UL);
> +
> + if (setitimer(ITIMER_REAL, &tim, NULL) =3D=3D -1)
> + err(EX_OSERR, "setitimer()");
> +}
> +
> +int
> +main(int argc, char **argv)
> +{
> + int ch;
> + unsigned long i;
> + int foreground, preserve;
> + int error, pstat, status;
> + int killsig =3D SIGTERM;
> + int killedwith;
> + pid_t pgid, pid, cpid;
> + double first_kill;
> + double second_kill;
> + bool timedout =3D false;
> + bool do_second_kill =3D false;
> + struct sigaction signals;
> + int signums[] =3D {
> + -1,
> + SIGTERM,
> + SIGINT,
> + SIGHUP,
> + SIGCHLD,
> + SIGALRM,
> + SIGQUIT,
> + };
> +
> + foreground =3D preserve =3D 0;
> + second_kill =3D 0;
> + cpid =3D -1;
> +
> + struct option longopts[] =3D {
This should be static const.
> + { "preserve-status", no_argument, &preserve, 1 },
> + { "foreground", no_argument, &foreground, 1 },
> + { "kill-after", required_argument, NULL, 'k'},
> + { "signal", required_argument, NULL, 's'},
> + { "help", no_argument, NULL, 'h'},
> + { NULL, 0, NULL, 0 }
> + };
> +
> + while ((ch =3D getopt_long(argc, argv, "+k:s:h", longopts, NULL)) !=3D =
-1) {
> + switch (ch) {
> + case 'k':
> + do_second_kill =3D true;
> + second_kill =3D parse_duration(optarg);
> + break;
> + case 's':
> + killsig =3D parse_signal(optarg);
> + break;
> + case 0:
> + break;
> + case 'h':
> + default:
> + usage();
> + break;
> + }
> + }
> +
> + argc -=3D optind;
> + argv +=3D optind;
> +
> + if (argc < 2)
> + usage();
> +
> + first_kill =3D parse_duration(argv[0]);
> + argc--;
> + argv++;
> +
> + if (!foreground) {
> + pgid =3D setpgid(0,0);
> +
> + if (pgid =3D=3D -1)
> + err(EX_OSERR, "setpgid()");
> + }
> +
> + memset(&signals, 0, sizeof(signals));
> + sigemptyset(&signals.sa_mask);
> +
> + if (killsig !=3D SIGKILL && killsig !=3D SIGSTOP)
> + signums[0] =3D killsig;
> +
> + for (i =3D 0; i < sizeof(signums) / sizeof(signums[0]); i ++)
> + sigaddset(&signals.sa_mask, signums[i]);
Calling sigaddset(..., -1); relies both on the quality of implementation,
and on absense of the implementation extensions, which could define
unexpected semantic for the undefined call.
> +
> + signals.sa_handler =3D sig_handler;
> + signals.sa_flags =3D SA_RESTART;
> +
> + for (i =3D 0; i < sizeof(signums) / sizeof(signums[0]); i ++)
> + if (signums[i] !=3D -1 && signums[i] !=3D 0 &&
> + sigaction(signums[i], &signals, NULL) =3D=3D -1)
> + err(EX_OSERR, "sigaction()");
> +
> + signal(SIGTTIN, SIG_IGN);
> + signal(SIGTTOU, SIG_IGN);
> +
> + pid =3D fork();
> + if (pid =3D=3D -1)
> + err(EX_OSERR, "fork()");
> + else if (pid =3D=3D 0) {
> + /* child process */
> + signal(SIGTTIN, SIG_DFL);
> + signal(SIGTTOU, SIG_DFL);
Shouldn't you preserve the signal disposition for all intercepted signals
and restore it for the child ?
> +
> + error =3D execvp(argv[0], argv);
> + if (error =3D=3D -1)
> + err(EX_UNAVAILABLE, "exec()");
> + }
> +
> + if (sigprocmask(SIG_BLOCK, &signals.sa_mask, NULL) =3D=3D -1)
> + err(EX_OSERR, "sigprocmask()");
> +
> + /* parent continues here */
> + set_interval(first_kill);
> +
> + sigemptyset(&signals.sa_mask);
This statement is not needed, you repeat it inside for() loop below.
> +
> + for (;;) {
> + killedwith =3D killsig;
> + sigemptyset(&signals.sa_mask);
> + sigsuspend(&signals.sa_mask);
> +
> + if (sig_chld) {
> + sig_chld =3D 0;
> + while (((cpid =3D wait(&status)) < 0) && errno !=3D EINTR)
> + continue;
Should the test be errno =3D=3D EINTR instead ?
> +
> + if (cpid =3D=3D pid) {
> + pstat =3D status;
> + break;
> + }
> + } else if (sig_alrm) {
> + sig_alrm =3D 0;
> +
> + timedout =3D true;
> + if (!foreground)
> + killpg(pgid, killsig);
> + else
> + kill(pid, killsig);
> +
> + if (do_second_kill) {
> + set_interval(second_kill);
> + second_kill =3D 0;
> + sig_ign =3D killsig;
> + killsig =3D SIGKILL;
> + } else
> + break;
> +
> + } else if (sig_term) {
> + if (!foreground)
> + killpg(pgid, killsig);
> + else
> + kill(pid, sig_term);
> +
> + if (do_second_kill) {
> + set_interval(second_kill);
> + second_kill =3D 0;
> + sig_ign =3D killsig;
> + killsig =3D SIGKILL;
> + } else
> + break;
> + }
> + }
> +
> + while (cpid !=3D pid && wait(&pstat) =3D=3D -1) {
> + if (errno !=3D EINTR)
There, the test looks correct.
> + err(EX_OSERR, "waitpid()");
> + }
> +
> + if (WEXITSTATUS(pstat))
> + pstat =3D WEXITSTATUS(pstat);
> + else if(WIFSIGNALED(pstat))
> + pstat =3D 128 + WTERMSIG(pstat);
> +
> + if (timedout && !preserve)
> + pstat =3D EXIT_TIMEOUT;
> +
> + return (pstat);
> +}
--LhGscpP45fjEbabV
Content-Type: application/pgp-signature
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iQIcBAEBAgAGBQJTxmKPAAoJEJDCuSvBvK1BQvIP/RH+KuzDEEd53symNv5dELw8
Mne4wGbjm7rEDV6KXBcHo1cf/dhDk9xWJnlskd99A6ECmwcoGXBigwpVMJJoUE4C
wbz3KhEkmXoK1QiV+7S1XfFHsT1ltHZsglcL/senHN7+sP2L6hC7q00m+OBS1oOa
rOFcIxnnkd/k05fLe8x/b7MQVZ39xCyaHb4gKIwa7mK9QYwFxxHbksl8WM57Q/Ey
5HjtI1vdu8mJWH5AOKgu4rlm97n0v6DQxGOFtiMCM54dJJ12Os/P57FDepMaq6h6
ZULU7wf+zhZGPZ3OXpJGSJt7KZeVViyzNO2dHLD17MvG3i7msarH4VxMXZkgk4av
eIVLDzNFF4BLe9DKmdd6GQ/qyjtD7F5pU6MBxZuhT1LH3hOwFpbBbn8oL+JGlywU
UMb9QGC/wbbpanVC0ZzgLpJtp7eMF9MiFEkItVIJ88OeEyfWBoYy3ZgCCbPKziGg
rcA7JzMWI3TH2uNvlq9NY6zWq+I+pKI0LlRU/dGL8FOaGlSq+djNclSSL9XfEnSd
VbC/kM1KTiAiO7hvr087UFWX2Uu+iHraD2ZmOC3vE/kKJROQBAPuCOGH+O2vPtBs
mTOFmIpSlw+dkhP7JyWIv7TiacTfkI6fePcoK9BtVOEa57o2A/qzH4FPxZpAzJdT
61MkhlIPbie/pOAH1gWJ
=X3yl
-----END PGP SIGNATURE-----
--LhGscpP45fjEbabV--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140716113128.GV93733>