svn commit: r268745 - in head/usr.bin: . timeout

[Konstantin Belousov]

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 = 0;
> +static sig_atomic_t sig_term = 0;
> +static sig_atomic_t sig_alrm = 0;
> +static sig_atomic_t sig_ign = 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 = strtod(duration, &end);
> +	if (ret == 0 && end == duration)
> +		errx(EXIT_FAILURE, "invalid duration");
> +
> +	if (end == NULL || *end == '\0')
> +		return (ret);
> +
> +	if (end != NULL && *(end + 1) != '\0')
> +		errx(EX_USAGE, "invalid duration");
> +
> +	switch (*end) {
> +	case 's':
> +		break;
> +	case 'm':
> +		ret *= 60;
> +		break;
> +	case 'h':
> +		ret *= 60 * 60;
> +		break;
> +	case 'd':
> +		ret *= 60 * 60 * 24;
> +		break;
> +	default:
> +		errx(EX_USAGE, "invalid duration");
> +	}
> +	
> +	if (ret < 0 || ret >= 100000000UL)
> +		errx(EX_USAGE, "invalid duration");
> +
> +	return (ret);
> +}
> +
> +static int
> +parse_signal(const char *str)
> +{
> +	int sig, i;
> +	const char *err;
> +
> +	sig = strtonum(str, 0, sys_nsig, &err);
> +
> +	if (err == NULL)
> +		return (sig);
> +	if (strncasecmp(str, "SIG", 3) == 0)
> +		str += 3;
> +
> +	for (i = 1; i < sys_nsig; i++) {
> +		if (strcasecmp(str, sys_signame[i]) == 0)
> +			return (i);
> +	}
> +	
> +	errx(EX_USAGE, "invalid signal");
> +}
> +
> +static void
> +sig_handler(int signo)
> +{
> +	if (sig_ign != 0 && signo == sig_ign) {
> +		sig_ign = 0;
> +		return;
> +	}
> +
> +	switch(signo) {
> +	case 0:
> +	case SIGINT:
> +	case SIGHUP:
> +	case SIGQUIT:
> +	case SIGTERM:
> +		sig_term = signo;
> +		break;
> +	case SIGCHLD:
> +		sig_chld = 1;
> +		break;
> +	case SIGALRM:
> +		sig_alrm = 1;
> +		break;
> +	}
> +}
> +
> +static void
> +set_interval(double iv)
> +{
> +	struct itimerval tim;
> +
> +	memset(&tim, 0, sizeof(tim));
> +	tim.it_value.tv_sec = (time_t)iv;
> +	iv -= (time_t)iv;
> +	tim.it_value.tv_usec = (suseconds_t)(iv * 1000000UL);
> +
> +	if (setitimer(ITIMER_REAL, &tim, NULL) == -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 = SIGTERM;
> +	int killedwith;
> +	pid_t pgid, pid, cpid;
> +	double first_kill;
> +	double second_kill;
> +	bool timedout = false;
> +	bool do_second_kill = false;
> +	struct sigaction signals;
> +	int signums[] = {
> +		-1,
> +		SIGTERM,
> +		SIGINT,
> +		SIGHUP,
> +		SIGCHLD,
> +		SIGALRM,
> +		SIGQUIT,
> +	};
> +
> +	foreground = preserve = 0;
> +	second_kill = 0;
> +	cpid = -1;
> +
> +	struct option longopts[] = {
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 = getopt_long(argc, argv, "+k:s:h", longopts, NULL)) != -1) {
> +		switch (ch) {
> +			case 'k':
> +				do_second_kill = true;
> +				second_kill = parse_duration(optarg);
> +				break;
> +			case 's':
> +				killsig = parse_signal(optarg);
> +				break;
> +			case 0:
> +				break;
> +			case 'h':
> +			default:
> +				usage();
> +				break;
> +		}
> +	}
> +
> +	argc -= optind;
> +	argv += optind;
> +
> +	if (argc < 2)
> +		usage();
> +
> +	first_kill = parse_duration(argv[0]);
> +	argc--;
> +	argv++;
> +
> +	if (!foreground) {
> +		pgid = setpgid(0,0);
> +
> +		if (pgid == -1)
> +			err(EX_OSERR, "setpgid()");
> +	}
> +
> +	memset(&signals, 0, sizeof(signals));
> +	sigemptyset(&signals.sa_mask);
> +
> +	if (killsig != SIGKILL && killsig != SIGSTOP)
> +		signums[0] = killsig;
> +
> +	for (i = 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 = sig_handler;
> +	signals.sa_flags = SA_RESTART;
> +
> +	for (i = 0; i < sizeof(signums) / sizeof(signums[0]); i ++)
> +		if (signums[i] != -1 && signums[i] != 0 &&
> +		    sigaction(signums[i], &signals, NULL) == -1)
> +			err(EX_OSERR, "sigaction()");
> +
> +	signal(SIGTTIN, SIG_IGN);
> +	signal(SIGTTOU, SIG_IGN);
> +
> +	pid = fork();
> +	if (pid == -1)
> +		err(EX_OSERR, "fork()");
> +	else if (pid == 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 = execvp(argv[0], argv);
> +		if (error == -1)
> +			err(EX_UNAVAILABLE, "exec()");
> +	}
> +
> +	if (sigprocmask(SIG_BLOCK, &signals.sa_mask, NULL) == -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 = killsig;
> +		sigemptyset(&signals.sa_mask);
> +		sigsuspend(&signals.sa_mask);
> +
> +		if (sig_chld) {
> +			sig_chld = 0;
> +			while (((cpid = wait(&status)) < 0) && errno != EINTR)
> +				continue;
Should the test be errno == EINTR instead ?

> +
> +			if (cpid == pid) {
> +				pstat = status;
> +				break;
> +			}
> +		} else if (sig_alrm) {
> +			sig_alrm = 0;
> +
> +			timedout = true;
> +			if (!foreground)
> +				killpg(pgid, killsig);
> +			else
> +				kill(pid, killsig);
> +
> +			if (do_second_kill) {
> +				set_interval(second_kill);
> +				second_kill = 0;
> +				sig_ign = killsig;
> +				killsig = 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 = 0;
> +				sig_ign = killsig;
> +				killsig = SIGKILL;
> +			} else
> +				break;
> +		}
> +	}
> +
> +	while (cpid != pid  && wait(&pstat) == -1) {
> +		if (errno != EINTR)
There, the test looks correct.

> +			err(EX_OSERR, "waitpid()");
> +	}
> +
> +	if (WEXITSTATUS(pstat))
> +		pstat = WEXITSTATUS(pstat);
> +	else if(WIFSIGNALED(pstat))
> +		pstat = 128 + WTERMSIG(pstat);
> +
> +	if (timedout && !preserve)
> +			pstat = EXIT_TIMEOUT;
> +
> +	return (pstat);
> +}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20140716/c8fc5337/attachment.sig>