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

[Garrett D'Amore]

On Tue, 2010-10-26 at 17:20 +1100, Joshua M. Clulow wrote:
> 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=.

Ah, right.  It would be strlen()+1 to include the terminating zero.  On
that note, your design make sense.

> 
> > 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).

Ok, thanks for explaining this.  (I probably had 3 or 4 beers too many
in me when I conducted the review last night. :-)

> 
> > 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?

Its not a bad idea.  Perhaps file a bug on the issue?

> 
> > line 491: again, strcmp() is sufficient.
> 
> See above about strncmp.

Noted.

> 
> > 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?

Yes please.  Have a try at making this code pass lint too... I'm
assuming it isn't currently linted, but since you're here anyway....

> 
> Thank you for reviewing!
> 

Sure.

	- Garrett