From owner-svn-src-all@freebsd.org Fri Mar 9 09:50:37 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 3842DF3DB2B; Fri, 9 Mar 2018 09:50:37 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id 7E8D96DCDA; Fri, 9 Mar 2018 09:50:35 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail108.syd.optusnet.com.au (Postfix) with ESMTPS id A12871A2D65; Fri, 9 Mar 2018 20:50:33 +1100 (AEDT) Date: Fri, 9 Mar 2018 20:50:32 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Conrad Meyer cc: Bruce Evans , Mark Johnston , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r330663 - head/sys/kern In-Reply-To: Message-ID: <20180309194900.F2122@besplex.bde.org> References: <201803081704.w28H4aQx052056@repo.freebsd.org> <20180309150402.X950@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=FNpr/6gs c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=EVY10kfv5RHgi_n90qYA:9 a=fUBJKX1s9cisX-TV:21 a=W0ZdzM6i1VXaRkF-:21 a=SskPc2CMMsG2m25c:21 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 09 Mar 2018 09:50:37 -0000 On Thu, 8 Mar 2018, Conrad Meyer wrote: > On Thu, Mar 8, 2018 at 8:42 PM, Bruce Evans 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