Re: [patch] crontab(5) add -n option to suppress mail when the run was successful
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 "-n...@instituut.net" 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
> *
> * 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)
>*/
>
Re: [patch] crontab(5) add -n option to suppress mail when the run was successful
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. > > > 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 "-n...@instituut.net" 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. Well, if it was actual getopt parsing it would work in either case due to ":" handling. Shrug.
Re: [patch] crontab(5) add -n option to suppress mail when the run was successful
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.
> 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 "-n...@instituut.net" 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
*
* 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.
Re: [patch] crontab(5) add -n option to suppress mail when the run was successful
Todd C. Miller wrote: > > --- 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 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? 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.
Re: [patch] crontab(5) add -n option to suppress mail when the run was successful
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 -, 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
> *
> * 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
> -
Re: [patch] crontab(5) add -n option to suppress mail when the run was successful
On Mon, 11 Jun 2018 20:23:11 + Job Snijders wrote: > > With the above example crontab(5) entry you'll only receive a mail > from cron(8) if the cp(1) encountered some kind of error. Good one Job. Related: https://marc.info/?l=openbsd-tech=142075623225995 Cheers, -- Craig Skinner | http://linkd.in/yGqkv7
