This is an oft-requested feature. The diff looks good, I've made
some minor comments in-line.
- todd
On Mon, 11 Jun 2018 20:23:11 -0000, Job Snijders wrote:
> diff --git usr.sbin/cron/crontab.5 usr.sbin/cron/crontab.5
> index 9c2e651980a..700010faadf 100644
> --- usr.sbin/cron/crontab.5
> +++ usr.sbin/cron/crontab.5
> @@ -193,14 +193,27 @@ will be changed into newline characters, and all data
> after the first
> .Ql %
> will be sent to the command as standard input.
> +.Pp
> +If the
> +.Ar command
> +field begins with
> +.Ql -n ,
> +the execution output will only be mailed if the command exits with a non-zer
> o
> +exit code.
> +No mail is sent after a successful run.
> +The
> +.Ql -n
> +option is an attempt to cure potentially copious volumes of mail coming from
> +.Xr cron 8 .
> If the
> .Ar command
> field begins with
> .Ql -q ,
> execution will not be logged.
> Use whitespace to separate
> -.Ql -q
> -from the command.
> +.Ql -n ,
> +.Ql -q ,
> +and the command.
> .Pp
> Commands are executed by
> .Xr cron 8
> diff --git usr.sbin/cron/do_command.c usr.sbin/cron/do_command.c
> index 6a4022fcc9a..5d60bff6421 100644
> --- usr.sbin/cron/do_command.c
> +++ usr.sbin/cron/do_command.c
> @@ -3,6 +3,7 @@
> /* Copyright 1988,1990,1993,1994 by Paul Vixie
> * Copyright (c) 2004 by Internet Systems Consortium, Inc. ("ISC")
> * Copyright (c) 1997,2000 by Internet Software Consortium, Inc.
> + * Copyright (c) 2018 Job Snijders <j...@openbsd.org>
> *
> * Permission to use, copy, modify, and distribute this software for any
> * purpose with or without fee is hereby granted, provided that the above
> @@ -80,7 +81,6 @@ child_process(entry *e, user *u)
> char **p, *input_data, *usernm;
> auth_session_t *as;
> login_cap_t *lc;
> - int children = 0;
> extern char **environ;
>
> /* mark ourselves as different to PS command watchers */
> @@ -146,7 +146,9 @@ child_process(entry *e, user *u)
>
> /* fork again, this time so we can exec the user's command.
> */
> - switch (fork()) {
> +
> + pid_t jobpid = -1;
No need to initialize jobpid, it is assigned directly after it is
declared.
> + switch (jobpid = fork()) {
> case -1:
> syslog(LOG_ERR, "(CRON) CAN'T FORK (%m)");
> _exit(EXIT_FAILURE);
> @@ -260,8 +262,6 @@ child_process(entry *e, user *u)
> break;
> }
>
> - children++;
> -
> /* middle process, child of original cron, parent of process running
> * the user's command.
> */
> @@ -283,7 +283,8 @@ child_process(entry *e, user *u)
> * we would block here. thus we must fork again.
> */
>
> - if (*input_data && fork() == 0) {
> + pid_t stdinjob = -1;
> + if (*input_data && (stdinjob = fork()) == 0) {
> FILE *out = fdopen(stdin_pipe[WRITE_PIPE], "w");
> int need_newline = FALSE;
> int escaped = FALSE;
> @@ -331,8 +332,6 @@ child_process(entry *e, user *u)
> */
> close(stdin_pipe[WRITE_PIPE]);
>
> - children++;
> -
> /*
> * read output from the grandchild. it's stderr has been redirected to
> * it's stdout, which has been redirected to our pipe. if there is any
> @@ -342,15 +341,17 @@ child_process(entry *e, user *u)
>
> (void) signal(SIGPIPE, SIG_IGN);
> in = fdopen(stdout_pipe[READ_PIPE], "r");
> +
> + char *mailto;
> + FILE *mail = NULL;
> + int status = 0;
> + pid_t mailpid = -1;
> + size_t bytes = 1;
> +
> if (in != NULL) {
> int ch = getc(in);
>
> if (ch != EOF) {
> - FILE *mail = NULL;
> - char *mailto;
> - size_t bytes = 1;
> - int status = 0;
> - pid_t mailpid;
>
> /* get name of recipient. this is MAILTO if set to a
> * valid local username; USER otherwise.
> @@ -415,30 +416,6 @@ child_process(entry *e, user *u)
> fputc(ch, mail);
> }
>
> - /* only close pipe if we opened it -- i.e., we're
> - * mailing...
> - */
> -
> - if (mail) {
> - /* Note: the pclose will probably see
> - * the termination of the grandchild
> - * in addition to the mail process, since
> - * it (the grandchild) is likely to exit
> - * after closing its stdout.
> - */
> - status = cron_pclose(mail, mailpid);
> - }
> -
> - /* if there was output and we could not mail it,
> - * log the facts so the poor user can figure out
> - * what's going on.
> - */
> - if (mail && status) {
> - syslog(LOG_NOTICE, "(%s) MAIL (mailed %zu byte"
> - "%s of output but got status 0x%04x)", usernm,
> - bytes, (bytes == 1) ? "" : "s", status);
> - }
> -
> } /*if data from grandchild*/
>
> fclose(in); /* also closes stdout_pipe[READ_PIPE] */
> @@ -446,20 +423,51 @@ child_process(entry *e, user *u)
>
> /* wait for children to die.
> */
> - for (; children > 0; children--) {
> - int waiter;
> - pid_t pid;
> -
> - while ((pid = wait(&waiter)) < 0 && errno == EINTR)
> + int waiter;
> + if (jobpid > 0) {
> + ;
> + while (waitpid(jobpid, &waiter, 0) < 0 && errno == EINTR)
> ;
> - if (pid < 0) {
> - break;
> +
> + /* If everything went well, and -n was set, _and_ we have mail,
> + * we won't be mailing... so shoot the messenger!
> + */
> + if (WIFEXITED(waiter) && WEXITSTATUS(waiter) == 0
> + && (e->flags & MAIL_WHEN_ERR) == MAIL_WHEN_ERR
> + && mail) {
> + fflush(mail);
> + kill(mailpid, SIGKILL);
> + (void)fclose(mail);
There's no need for fflush(mail) before you kill the mailer.
> + mail = NULL;
> }
> - /*
> - if (WIFSIGNALED(waiter) && WCOREDUMP(waiter))
> - Debug(DPROC, (", dumped core"))
> +
> + /* only close pipe if we opened it -- i.e., we're
> + * mailing...
> */
> + if (mail) {
> + /*
> + * Note: the pclose will probably see the termination
> + * of the grandchild in addition to the mail process,
> + * since it (the grandchild) is likely to exit after
> + * closing its stdout.
> + */
> + status = cron_pclose(mail, mailpid);
> + }
> +
> + /* if there was output and we could not mail it,
> + * log the facts so the poor user can figure out
> + * what's going on.
> + */
> + if (mail && status) {
> + syslog(LOG_NOTICE, "(%s) MAIL (mailed %zu byte"
> + "%s of output but got status 0x%04x)", usernm,
> + bytes, (bytes == 1) ? "" : "s", status);
> + }
> }
> +
> + if (stdinjob > 0)
> + while (waitpid(stdinjob, &waiter, 0) < 0 && errno == EINTR)
> + ;
> }
>
> int
> diff --git usr.sbin/cron/entry.c usr.sbin/cron/entry.c
> index 761f01d3d6f..a9d358a2068 100644
> --- usr.sbin/cron/entry.c
> +++ usr.sbin/cron/entry.c
> @@ -338,6 +338,10 @@ load_entry(FILE *file, void (*error_func)(const char *),
> struct passwd *pw,
> ch = get_char(file);
> while (ch == '-') {
> switch (ch = get_char(file)) {
> + case 'n':
> + e->flags |= MAIL_WHEN_ERR;
> + Skip_Nonblanks(ch, file)
> + break;
> case 'q':
> e->flags |= DONT_LOG;
> Skip_Nonblanks(ch, file)
> @@ -346,6 +350,7 @@ load_entry(FILE *file, void (*error_func)(const char *),
> struct passwd *pw,
> ecode = e_option;
> goto eof;
> }
> +
> Skip_Blanks(ch, file)
> if (ch == EOF || ch == '\n') {
> ecode = e_cmd;
> diff --git usr.sbin/cron/structs.h usr.sbin/cron/structs.h
> index af0d75c3b55..597f65d6614 100644
> --- usr.sbin/cron/structs.h
> +++ usr.sbin/cron/structs.h
> @@ -38,6 +38,7 @@ typedef struct _entry {
> #define DOW_STAR 0x08
> #define WHEN_REBOOT 0x10
> #define DONT_LOG 0x20
> +#define MAIL_WHEN_ERR 0x40
> } entry;
>
> /* the crontab database will be a list of the
