Page MenuHomeFreeBSD

cron: add log suppression and mail suppression for successful runs
ClosedPublic

Authored by freebsd_t.lastninja.net on Apr 25 2019, 5:16 AM.

Details

Summary

I would like to see FreeBSD provide the crontab extensions found in OpenBSD
that implement -n to suppress mail on successful run and -q to suppress
logging of command execution.

The -q option appears decades old, but -n is relatively new. The
original proposal by Job Snijder can be found here [1], and gives very
convincing reasons for inclusion in base.

This patch is a nearly identical port of OpenBSD cron for -q and -n
features. It is written to follow existing conventions and style of the
existing codebase. I'm also not good at writing manpages but gave it my best
shot.

[1]: https://marc.info/?l=openbsd-tech&m=152874866117948&w=2

PR: 237538

Test Plan

I tested each of the following crontab entries in isolation:

# should both log and send email
* * * * * date

# should log but not send email (check to preserve original behaviour)
* * * * * date >/dev/null

# should only send email, but won't show up in log
* * * * * -q date

# should not send email
* * * * * -n date

# should not send email or log
* * * * * -n -q date

# should send email because of ping failure
* * * * * -n -q ping -c 1 5.5.5.5

I also tested to ensure the command options work using the
/etc/crontab format which requires user to be specified.

Also, I tested the empty command entry, e.g.:

* * * * *

This will log but never send an email.

However, because of the way the parsing works, an empty command
is not supported in conjunction with command options. So:

* * * * * -q

will not parse as valid crontab syntax.

I suspect this is the same behaviour as OpenBSD crontab, and
frankly makes sense - since an empty command entry only purpose
is to provide a signal in the cron logs.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

freebsd_t.lastninja.net edited the test plan for this revision. (Show Details)Apr 25 2019, 5:17 AM
freebsd_t.lastninja.net edited the test plan for this revision. (Show Details)Apr 25 2019, 5:20 AM
freebsd_t.lastninja.net edited the summary of this revision. (Show Details)Apr 25 2019, 5:24 AM
freebsd_t.lastninja.net edited the summary of this revision. (Show Details)
  • crontab: add -n to not mail if run is successful
  • crontab: add -n to not mail if run is successful
freebsd_t.lastninja.net edited the summary of this revision. (Show Details)Apr 25 2019, 2:10 PM
freebsd_t.lastninja.net edited the summary of this revision. (Show Details)
freebsd_t.lastninja.net set the repository for this revision to rS FreeBSD src repository.
pi added a subscriber: pi.Apr 25 2019, 6:54 PM
bcr added a subscriber: bcr.Apr 26 2019, 9:37 AM

Can you run "mandoc -Tlint" and textproc/igor over the man page? Thanks!

  • crontab: add -q option to suppress logging
  • crontab: add -n to not mail if run is successful
In D20046#431547, @bcr wrote:

Can you run "mandoc -Tlint" and textproc/igor over the man page? Thanks!

Hi I have run both commands and rectified all warnings introduced by my change.

Thanks.

bcr accepted this revision.Apr 26 2019, 1:01 PM

OK, thanks. Approved for the manpages part.

This revision is now accepted and ready to land.Apr 26 2019, 1:01 PM
freebsd_t.lastninja.net edited the test plan for this revision. (Show Details)Apr 27 2019, 11:55 AM

Sorry for the long delay... I think just these really minor style nits.

usr.sbin/cron/cron/do_command.c
99 ↗(On Diff #56699)

Kill this blank line

539 ↗(On Diff #56699)

Please move this comment to its own spacing after beginning and before end of comment markers

543 ↗(On Diff #56699)

I'd lean towards moving the comment to the line before it, but the style here is all fairly broken (already, before this change) with respect to style(9)

usr.sbin/cron/lib/entry.c
440 ↗(On Diff #56699)

Space after switch keyword

danfe added a subscriber: danfe.Jul 11 2019, 3:07 AM
danfe added inline comments.
usr.sbin/cron/cron/do_command.c
101 ↗(On Diff #56699)

Since you're changing these lines anyway, would it make sense to drop the register specifier?

546 ↗(On Diff #56699)

This comment violates our style(9).

553 ↗(On Diff #56699)

This one as well.

559 ↗(On Diff #56699)

I'm not a native speaker, but shouldn't executed be followed by an adverb, not adjective?

kevans added inline comments.Jul 11 2019, 3:43 AM
usr.sbin/cron/cron/do_command.c
546 ↗(On Diff #56699)

It matches the style of the rest of the file, which is more important and allowed by style(9) last time I read it.

553 ↗(On Diff #56699)

Ditto above comment

559 ↗(On Diff #56699)

Indeed, successfully