svn commit: r330663 - head/sys/kern

[Bruce Evans]

On Thu, 8 Mar 2018, Conrad Meyer wrote:

> On Thu, Mar 8, 2018 at 8:42 PM, Bruce Evans <brde at optusnet.com.au> wrote:
>> On Thu, 8 Mar 2018, Mark Johnston wrote:
>>> ...
>>> +++ head/sys/kern/kern_shutdown.c       Thu Mar  8 17:04:36 2018
>>> (r330663)
>>> @@ -1115,6 +1115,12 @@ dump_check_bounds(struct dumperinfo *di, off_t
>>> offset,
>>>
>>>         if (length != 0 && (offset < di->mediaoffset ||
>>>             offset - di->mediaoffset + length > di->mediasize)) {
>>> +               if (di->kdcomp != NULL && offset >= di->mediaoffset) {
>>> +                       printf(
>>> +                   "Compressed dump failed to fit in device
>>> boundaries.\n");
>>> +                       return (E2BIG);
>>> +               }
>>> +
>>
>> Style bug: extra blank line.  Even the outer if statements in this function
>> are missing this error.
>
> No.  Style(9) does not and should not require the complete absence of
> blank lines.  Committers are free to use their best judgement on how
> to separate blocks of code.  I think Mark's decision is eminently
> reasonable.

style(9) does require this.  This is hard to see since the style guide
has been mangled by converting it into a man page, with markup in non
C-code and lots of blank lines to separate the markup.  See
/usr/src/admin/style/style in 4.4BSD or /usr/src/share/misc/style in
NetBSD for non-mangled versions.  These are pure C code which give
many rules implicitly by example.  All versions give 2 or 3 rules for
leaving a blank line between groups of header files.  These rules are
somewhat needed since leaving blank lines is so unusual.

It is also the default for indent(1) (indent -sob).  indent removes far too
many blank lines by default.  I like to have optional blank lines only
before blocks of code headed by a comment (with the blank line before the
comment).  But is more common in KNF code to not have a blank line before
a block comment (since the "/*" for the first line of the comment is an
adequate separator).

Blank lines might be used to separate sections of code, but the above
blank line is a very large style bug since it separates related code,
while the style nearby is to not separate even unrelated code.

kern_shutdown.c has very few extra blank lines.  indent only wants to
remove 2.  I see a few more:

- the include of sys/signalvar.h is separated from all other includes
   by a blank line (and more so by not sorting it into the sys includes
   section)
- similarly for the include of machine/stdarg.h.  This has an attached
   comment about K&R support that wasn't needed even when K&R was supported.
- boot() has an extra blank line between initializing waittime() and
   calling sync().  These are unrelated, but the code is so simple (just
   2 statements) that splitting it up is not useful.  Elsewhere, boot()
   uses lots of little sections headed by comments with a block line before
   the comments except for some block comments.
- panic() has a blank line before a va_start() but no blank line after the
   matching va_end(), then blank line after the next statement.  The second
   blank line is correct for matching the first one since the next statement
   is related (it completes printing a message).
- kproc_shutdown() has a bogus blank line after a return statement, and a
   nonsense blank line to separate checking of 'error' from setting it.
- kthread() copies the bad style of kproc_shutdown() perfectly.

Dumper code had no extra blank lines.  This is easy enough since it is small
(less than 10% of the file).

>>>                 printf("Attempt to write outside dump device
>>> boundaries.\n"
>>>             "offset(%jd), mediaoffset(%jd), length(%ju),
>>> mediasize(%jd).\n",
>>>                     (intmax_t)offset, (intmax_t)di->mediaoffset,
>> ...
>>
>> Other style bugs:
>> - capitalization of error messages
>> - termination of error messages with a period
>
> Why do you think these are style bugs?

Because they are.  Error messages are conventionally not capitalized or
terminated with a period.  You snipped my partial explanation by example
that err(3) almost enforces this in userland by not supporting it.
Userland messages look like "foo: bar: Bad style", where 'foo' is the program
name and it would be unUnix-like to capitalize it; 'bar' might be a function
name and it would be Unix-like to capitalize that too, or it might be an
English word and then it is unclear whose style rules apply (IIRC,
capitalization after a colon only depends on style); and "Bad style" is
strerror(ESTYLE) -- for some reason, strings returned by strerror() are
always capitalized so they match some style rules for colons, and then using
different style rules for 'bar' is either a bug or a feature.

There is not much enforcement of this in the kernel.  panic() is a bit
like err().  Using device_printf() gives a uniform style for the first
word in device driver messages.   It would be unUnix-line to capitalize
device names, so this gives uncapitalized messages if the style rule
used for capitalization after colons is to not capitalize.

Bruce