On Tue, Jun 12, 2018 at 09:13:05PM +0200, Job Snijders wrote:
> On Tue, Jun 12, 2018 at 09:54:47AM -0600, Theo de Raadt wrote:
> > I would prefer if the -q and -n descriptions were in a table. I dislike
> > the ancient style of describing such things inline (harder to spot).
> > And it really falls down when there are multiple ones. How do you
> > feel about that jmc?
>
> Agreed, I changed it a bit to improve readability.
>
morning.
i agree a small list for the options is probably clearer. but not in
the way your diff does it. the format of the page is very odd anyway.
barring a rewrite, i'd just keep it as it is, but add a standard text
along the lines of:
Commands may be modified as follows:
.Bl -tag width Ds
.It Li %
...
.It Fl n Ar command
...
something like that?
i don;t want to see it split up into these small mini sections, but i
understand the need. the page might benefit from some sort of reworking,
but i'd save that for another diff.
jmc
> > Also, do -qn and -nq work? How about -nnn. Not saying those make a
> > lot of sense, but once getopt syntax is borrowed it should probably be
> > honoured.
>
> I redid this piece a little bit, and opted to go a bit stricter to leave
> as much freedom as possible for future extensions.
>
> OK:
> -n command
> -n -q command
> -q -n command
> -q command
> command
>
> Not OK:
> -nn command
> -qn command
> -q -q command
> -n -n -q command
>
> My thinking is by being strict now, we make it possible to add arguments
> to options in the future. If we allow "-nn" or "-nq" now, we won't be
> able to allow "[email protected]" in the future. Or maybe we'll want
> "-v" to mean something different than "-vv". I don't know, so prefer to
> be less forgiving.
>
> Kind regards,
>
> Job
>
>
> diff --git usr.sbin/cron/crontab.5 usr.sbin/cron/crontab.5
> index 9c2e651980a..d9330698fd3 100644
> --- usr.sbin/cron/crontab.5
> +++ usr.sbin/cron/crontab.5
> @@ -193,15 +193,29 @@ will be changed into newline characters, and all data
> after the first
> .Ql %
> will be sent to the command as standard input.
> -If the
> +.Ss Options
> +The
> .Ar command
> -field begins with
> -.Ql -q ,
> -execution will not be logged.
> +field can begin with one or more options.
> +.Bl -tag -width Ds
> +.It Fl n
> +No mail is send after a successful run.
> +The execution output will only be mailed if the command exits with a non-zero
> +exit code.
> +The
> +.Ql -n
> +option is an attempt to cure potentially copious volumes of mail coming from
> +.Xr cron 8 .
> +.It Fl q
> +Execution will not be logged.
> +.El
> +.Pp
> Use whitespace to separate
> -.Ql -q
> -from the command.
> +.Ql -n ,
> +.Ql -q ,
> +and the command.
> .Pp
> +.Ss Execution
> Commands are executed by
> .Xr cron 8
> when the
> @@ -329,6 +343,9 @@ Ranges may include
> .It
> Months or days of the week can be specified by name.
> .It
> +Mailing after a successful run can be suppressed with
> +.Ql -n .
> +.It
> Logging can be suppressed with
> .Ql -q .
> .It
> diff --git usr.sbin/cron/do_command.c usr.sbin/cron/do_command.c
> index 6a4022fcc9a..4fbca61d170 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 <[email protected]>
> *
> * 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;
> + 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;
> + 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;
> + 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,50 @@ 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) {
> + kill(mailpid, SIGKILL);
> + (void)fclose(mail);
> + 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..cba6e805dc0 100644
> --- usr.sbin/cron/entry.c
> +++ usr.sbin/cron/entry.c
> @@ -338,14 +338,32 @@ 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':
> + /* only allow the user to set this option once */
> + if ((e->flags & MAIL_WHEN_ERR) == MAIL_WHEN_ERR) {
> + ecode = e_option;
> + goto eof;
> + }
> + e->flags |= MAIL_WHEN_ERR;
> + break;
> case 'q':
> + /* only allow the user to set this option once */
> + if ((e->flags & DONT_LOG) == DONT_LOG) {
> + ecode = e_option;
> + goto eof;
> + }
> e->flags |= DONT_LOG;
> - Skip_Nonblanks(ch, file)
> break;
> default:
> ecode = e_option;
> goto eof;
> }
> + ch = get_char(file);
> + if (ch!='\t' && ch!=' ') {
> + 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
>
