[illumos-Developer] webrev: 374 cron should send more useful mail

[Joshua M. Clulow]

On Tue, Oct 26, 2010 at 4:45 PM, Garrett D'Amore <garrett at nexenta.com> wrote:
> cron.c: line 1211.  No need to use strncmp since the 3rd argument is
> just strlen().  strcmp() will automatically terminate when hitting a
> null byte anyway.

The intention is to compare the beginning of the string with the
contents of ENV_SUBJECT - I'm pretty sure strcmp would compare the
entire input string.  i.e. we're seeing if the line in question is
prefixed with "MAIL_SUBJECT=", as is the case for the similar extant
blocks that check for HOME=, TZ= and SHELL=.

> line 2250: why not just use strdup().  (What does xmalloc offer?  I need
> to investigate further.)

I've changed to strdup() in my workspace now.

> line 1292, 1293: given the release at 1305, what's the point of this
> release?

The crontab processing loop processes one line at a time (starts at
~1153).  Some of those lines are variables like HOME=, or newly
MAIL_SUBJECT=, and the rest are crontab entries that describe jobs.
HOME/SHELL/TZ, once set, affect all job entries that appear after
their definition in the crontab.  I feel that MAIL_SUBJECT= should
really only affect the job entry that appears directly after it, so
I'm releasing and NULLing the variable that keeps track of it once an
entry uses the value (lines 1292-1293).  I then release again at the
bottom of readcron() because there are some cases where the code
breaks out of the loop (mostly error handling cases).

> Generally it looks ok, but I need to spend more time looking carefully
> at this "shared" memory allocation.

Yeah, cron appears somewhat more complicated that it needs to be.

> crontab.c: line 503.  Should this be wrapped with gettext()?  I've not
> looked at the other output messages for crontab.

crontab appears relatively inconsistent in this respect.  I can
certainly use gettext() -- should I also be fixing other #defined
messages that don't use gettext() at the same time?

> line 491: again, strcmp() is sufficient.

See above about strncmp.

> line 495: shouldn't you cast strncpy() to void?
> line 503: ditto fprintf().

Yes, I should!  There are a number of other places that don't have
proper void casts -- in particular directly above my addition where I
copied most of the boilerplate from in order to be consistent with the
existing code.  Should I fix those too?

Thank you for reviewing!

-- 
Joshua M. Clulow
UNIX Admin/Developer
http://blog.sysmgr.org