From owner-svn-src-all@FreeBSD.ORG Fri Sep 7 23:02:18 2012 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id B26991065670; Fri, 7 Sep 2012 23:02:18 +0000 (UTC) (envelope-from ray@freebsd.org) Received: from smtp.dlink.ua (smtp.dlink.ua [193.138.187.146]) by mx1.freebsd.org (Postfix) with ESMTP id 0804E8FC19; Fri, 7 Sep 2012 23:02:17 +0000 (UTC) Received: from rnote.ddteam.net (88-243-133-95.pool.ukrtel.net [95.133.243.88]) (Authenticated sender: ray) by smtp.dlink.ua (Postfix) with ESMTPSA id EF2BDC493D; Sat, 8 Sep 2012 02:02:10 +0300 (EEST) Date: Sat, 8 Sep 2012 02:02:05 +0300 From: Aleksandr Rybalko To: Bruce Evans Message-Id: <20120908020205.fd7f0c7f.ray@freebsd.org> In-Reply-To: <20120905135841.D1053@besplex.bde.org> References: <201209042316.q84NGtuA035023@svn.freebsd.org> <20120905135841.D1053@besplex.bde.org> Organization: FreeBSD.ORG X-Mailer: Sylpheed 3.1.2 (GTK+ 2.24.5; amd64-portbld-freebsd9.0) X-Operating-System: FreeBSD Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: svn-src-head@freebsd.org, Adrian Chadd , svn-src-all@freebsd.org, src-committers@freebsd.org Subject: [PATCH]Re: svn commit: r240119 - head/sys/kern X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 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, 07 Sep 2012 23:02:18 -0000 Hi Bruce! Did not absorb all details of style(9) yet. But I'm working on that :) On Wed, 5 Sep 2012 14:18:40 +1000 (EST) Bruce Evans wrote: > On Tue, 4 Sep 2012, Aleksandr Rybalko wrote: > > > Log: > > Style fixes. > > > > Suggested by: mdf > > Approved by: adrian (menthor) > > The following style bugs remain. (The density of style bugs is low > enough for them to be easy to fix.) > > > Modified: head/sys/kern/subr_hints.c > > ============================================================================== > > --- head/sys/kern/subr_hints.c Tue Sep 4 23:13:24 > > 2012 (r240118) +++ head/sys/kern/subr_hints.c Tue > > Sep 4 23:16:55 2012 (r240119) @@ -31,8 +31,8 @@ __FBSDID > > ("$FreeBSD$"); > > #include > > #include > > #include > > -#include > > #include > > +#include > > Sorting this correctly would be an unrelated fix (it is a prerequisite > for most headers, since almost any header may use KASSERT()). Yeah, now I found right place for it. > > > #include > > Sorting this correctly woruld be an unrelated fix. > Yeah, kind of second for kernel. > > > > /* > > @@ -52,9 +52,9 @@ static char *hintp; > > Sorting and indenting the static variables would be an unrelated fix. I swear, I was not touch hintp. Same with checkmethod and use_kenv. :) > > > static int > > sysctl_hintmode(SYSCTL_HANDLER_ARGS) > > A bug in svn diff is visible. The variable declaration is worse than > useless as a header for this block of code. > Did not get it. About which variable you saying? > > > { > > - int error, i, from_kenv, value, eqidx; > > const char *cp; > > char *line, *eq; > > + int eqidx, error, from_kenv, i, value; > > > > from_kenv = 0; > > cp = kern_envp; > > @@ -62,7 +62,7 @@ sysctl_hintmode(SYSCTL_HANDLER_ARGS) > > > > /* Fetch candidate for new hintmode value */ > > Comments (except possibly ones at the right of code) should be real > sentences. This one is missing a ".", unlike all older comments > (not at the right of code) in this file. Fixed. > > > error = sysctl_handle_int(oidp, &value, 0, req); > > - if (error || !req->newptr) > > + if (error || req->newptr == NULL) > > return (error); > > > > if (value != 2) > > This still has a boolean test for the non-boolean `error'. Now the > older code sets a bad example in all cases where `error' is tested. Fixed. > > > @@ -73,8 +73,11 @@ sysctl_hintmode(SYSCTL_HANDLER_ARGS) > > switch (hintmode) { > > case 0: > > if (dynamic_kenv) { > > - /* Already here */ > > - hintmode = value; /* XXX: Need we switch > > or not ? */ > > + /* > > + * Already here. But assign hintmode to 2, > > to not > > + * check it in the future. > > + */ > > Sentence breaks should be 2 spaces, as in all older comments in this > file, starting as usual with the copyright. But outside of the > copyright, the style bug of single-space sentence breaks was avoided > in this file mostly by using the larger style bug of using a new line > for most new sentences. Already start delimiting sentences with double space. When folks last time arguing about it, I found power to read only about 10 first mails. :) > > > + hintmode = 2; > > return (0); > > } > > from_kenv = 1; > > @@ -98,7 +101,7 @@ sysctl_hintmode(SYSCTL_HANDLER_ARGS) > > continue; > > } > > eq = strchr(cp, '='); > > - if (!eq) > > + if (eq == NULL) > > /* Bad hint value */ > > continue; > > eqidx = eq - cp; > > Bruce Thank you very much Bruce! I am understand how much important is code style, but still on the way to that point. Patch: --------------------------------------------------------------------- Index: subr_hints.c =================================================================== --- subr_hints.c (revision 240161) +++ subr_hints.c (working copy) @@ -28,12 +28,12 @@ __FBSDID("$FreeBSD$"); #include +#include +#include #include #include #include #include -#include -#include /* * Access functions for device resources. @@ -45,7 +45,7 @@ static char *hintp; /* * Define kern.hintmode sysctl, which only accept value 2, that cause to - * switch from Static KENV mode to Dynamic KENV. So systems that have hints + * switch from Static KENV mode to Dynamic KENV. So systems that have hints * compiled into kernel will be able to see/modify KENV (and hints too). */ @@ -60,21 +60,20 @@ sysctl_hintmode(SYSCTL_HANDLER_ARGS) cp = kern_envp; value = hintmode; - /* Fetch candidate for new hintmode value */ + /* Fetch candidate for new hintmode value. */ error = sysctl_handle_int(oidp, &value, 0, req); - if (error || req->newptr == NULL) + if (error != 0 || req->newptr == NULL) return (error); - if (value != 2) - /* Only accept swithing to hintmode 2 */ + if (value != 2) /* Only accept swithing to hintmode 2 */ return (EINVAL); - /* Migrate from static to dynamic hints */ + /* Migrate from static to dynamic hints. */ switch (hintmode) { case 0: if (dynamic_kenv) { /* - * Already here. But assign hintmode to 2, to not + * Already here. But assign hintmode to 2, to not * check it in the future. */ hintmode = 2; @@ -87,22 +86,21 @@ sysctl_hintmode(SYSCTL_HANDLER_ARGS) cp = static_hints; break; case 2: - /* Nothing to do, hintmode already 2 */ + /* Nothing to do, if hintmode already 2. */ return (0); } - while (cp) { + while (cp != '\0') { i = strlen(cp); if (i == 0) break; if (from_kenv) { if (strncmp(cp, "hint.", 5) != 0) - /* kenv can have not only hints */ + /* kenv can have not only hints. */ continue; } eq = strchr(cp, '='); - if (eq == NULL) - /* Bad hint value */ + if (eq == NULL) /* Bad hint value */ continue; eqidx = eq - cp; --------------------------------------------------------------------- WBW -- Aleksandr Rybalko