svn commit: r252672 - head/sbin/nvmecontrol

[Bruce Evans]

On Tue, 9 Jul 2013, Jim Harris wrote:

> On Sat, Jul 6, 2013 at 7:26 PM, Bruce Evans <brde at optusnet.com.au> wrote:
>
>> On Sat, 6 Jul 2013, Jilles Tjoelker wrote:
>>
>>  On Sat, Jul 06, 2013 at 08:42:49PM +0200, Pawel Jakub Dawidek wrote:
>[>]* ...
>>>  Bruce, until sysexits(3) doesn't explicitly say it shouldn't be used,
>>>> please stop calling this a bug, because you are just confusing people.
>>>> At this point sysexits(3) actually even suggests it is blessed by
>>>> style(9). This is how it starts:
>>>
>>>          According to style(9), it is not a good practice to call exit(3)
>>>>         with arbitrary values to indicate a failure condition when
>>>>         ending a program. Instead, the pre-defined exit codes from
>>>>         sysexits should be used, so the caller of the process can get a
>>>>         rough estimation about the failure class without looking up the
>>>>         source code.
>>>
>> This is just another bug in sysexits(3).  This is not according to
>> style(9), since style(9) was fixed to not say that after I complained
>> previously :-).  It has never been normal practice to use sysexits(3),
>> but someone who likes it added recommendations to use it to style(9) when
>> they added the man pages for sysexits(3).  Before that, it was
>> so rarely used that it had no man page.
>
> To add to the areas of confusion already stated in this thread, err(3)
> explicitly recommends using sysexits(3) and uses it in all of the examples.

Hmm, that isn't in err(3) in the old version of FreeBSD that I use.

> I decided to use 0/1 instead of sysexits since it seems most appropriate
> based on the discussion here and other examples in sbin.  I incorporated
> these changes as well as addressing some of Bruce's other feedback in
> r253109.

Thanks.

The examples in err(3) always were bad.  They currently are:

% EXAMPLES
%      Display the current errno information string and exit:
% 
%            if ((p = malloc(size)) == NULL)
%                    err(EX_OSERR, NULL);

This has no string at all.  Use of err() and EX_OSERR are wrong too,
or at least a style bug and gratuitously unportable.  In Standard C,
malloc() is not specified to set errno.  In POSIX it is specified to
set errno to ENOMEM iff it fails.  The POSIX setting of errno just
allows sloppy code like the above to work -- ENOMEM will be translated
to "Cannot allocate memory" by err().

Normal style is to not depend on this and print the more specific
message "malloc failed" using errx().  Note that err() is not
sophisticated enough to print a good message following "malloc failed".
'err(1, "malloc failed")' would print "foo: malloc failed: Cannot
allocate memory\n").  This is missing a " (duh)" after the redundant
"Cannot allocate memory".

In $(find /usr/src/*bin -name *.c), there are 136 lines matching
"malloc f":
- 17 of these lines use precisely warnx("malloc failed").
- 49 use precisely errx(1, "malloc failed").
All these have normal style.
- 13 mire use errx(), with assorted bugs and style bugs:
   - 2 have a bogus newline at the end of the string
   - 4 say "failure" instead of "failed"
   - 1 says "malloc for continent submenu"
   - 1 says "malloc for submenu"
   - 1 says "kern.consile malloc failed"
   - 3 use EX_OSERR
   - 1 uses unusual whitespace
- 10 use printf or fprintf.  Most of these print the normal message
   "malloc failed" and not much more.  The only obvious stle bug is
   that 4 of these terminate the message with a "."
Most of the remaining 47 have style bugs:
- 4 use precisely err(1, "malloc failed")
- 6 in ar use precisely bsdar_errc(bsdar, EX_SOFTWARE, errno, "malloc failed")
- 2 in ar use "malloc failed" on a line by itself, presumably because the
   verboseness is large enough to defeat my simple regexp
- 3 in gzip use maybe_error("malloc failed")
- 1 uses precisely warn(1, "malloc failed")
- 13 use syslog(LOG_ERR, ...).  Mostly with "malloc failed" annotated
   inconsistently.  bsnmpd uses strerror(errno) a lot. It should use %m
   unless that is too unportable.  It mostly formats this using ": %s",
   but it sometimes uses " - %s" and sometimes doesn't print the errno.
   - 5 of the 13 don't print the redundant errno, so don't have any obvious
     style bugs
- 2 are in comments, with the bad grammar "If the malloc fail".
- 1 is in a comment, with the less bad grammar "malloc fail"
- 3 use precisely fatal(EX_UNAVAIL, "malloc faield"
- 3 more use strerror(errno) to add nothing
- 2 more seem to use syslog via a utility function (no errno, so OK)
- 3 use yp_error("malloc failed" or similar
- the remaining 4 use "[...] malloc [...] failed" to add something.

The example used to be worse.   It said just err(1, NULL).  No hint about
the source of the error.

%            if ((fd = open(file_name, O_RDONLY, 0)) == -1)
%                    err(EX_NOINPUT, "%s", file_name);

Error messages for open failure are more varied than for malloc failure.
I like to put at least the syscall or function name in all error messages
when not trying hard to write a very detailed but concise message.

% 
%      Display an error message and exit:
% 
%            if (tm.tm_hour < START_TIME)
%                    errx(EX_DATAERR, "too early, wait until %s",
%                        start_time_string);

Might be precise enough in context.

The message has a grammar error (comma splice).

Timing errors are unlikely to be related to data errors.  EX_TEMPFAIL
would be better, but I prefer 1 with a better message of course.

% 
%      Warn of an error:
% 
%            if ((fd = open(raw_device, O_RDONLY, 0)) == -1)
%                    warnx("%s: %s: trying the block device",
%                        raw_device, strerror(errno));

This is really an example of how to print the errno when you want it
but don't want it in the standard position given by warn().

%            if ((fd = open(block_device, O_RDONLY, 0)) == -1)
%                    err(EX_OSFILE, "%s", block_device);

This is not a warning, but a fatal error.

Both examples could be improved by giving the function name, possibly
by using "opening" instead of "trying".

Bruce