Date: Sat, 8 Sep 2012 02:02:05 +0300 From: Aleksandr Rybalko <ray@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, Adrian Chadd <adrian.chadd@gmail.com>, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: [PATCH]Re: svn commit: r240119 - head/sys/kern 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <brde@optusnet.com.au> 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 <sys/lock.h>
> > #include <sys/malloc.h>
> > #include <sys/mutex.h>
> > -#include <sys/systm.h>
> > #include <sys/sysctl.h>
> > +#include <sys/systm.h>
>
> 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 <sys/bus.h>
>
> Sorting this correctly woruld be an unrelated fix.
>
Yeah, kind of second <sys/types.h> 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 <sys/param.h>
+#include <sys/systm.h>
+#include <sys/bus.h>
#include <sys/lock.h>
#include <sys/malloc.h>
#include <sys/mutex.h>
#include <sys/sysctl.h>
-#include <sys/systm.h>
-#include <sys/bus.h>
/*
* 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 <ray@freebsd.org>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120908020205.fd7f0c7f.ray>