From owner-svn-src-head@FreeBSD.ORG Sun Dec 8 09:36:20 2013 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 4C4E25C4; Sun, 8 Dec 2013 09:36:20 +0000 (UTC) Received: from mail-pd0-x22b.google.com (mail-pd0-x22b.google.com [IPv6:2607:f8b0:400e:c02::22b]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 0C8A21691; Sun, 8 Dec 2013 09:36:20 +0000 (UTC) Received: by mail-pd0-f171.google.com with SMTP id z10so3423837pdj.16 for ; Sun, 08 Dec 2013 01:36:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-type:content-transfer-encoding; bh=UatvS/2IR+fy6XLXHua3Zid/YipF2AWCdKyzQ26WgMU=; b=fEs5kv4iLGXHI1yQ4+BXO4LK7IzN6w2ewwll7RykFBPzndQvRi6nUsNN3D+gIH8QTb zj7Xn1NznJ2Pjj38+J86qs/l4BEa7ZrMdXEP2KXcALaw3rr/0WLmTUHWXwnA5iNZeLsK LEvN2FwY0FWnPADwwb5C7rhmJKFpwu4BQHCiEa6L+EPNL5D3qeQfJfumboeoAyWZ1d77 yf95uPhI//yCVAfXn1qBSiaMP97Z5kgK6k7fSnwB/Vdgh3fYMybOvmKqSIOYmNG/FXM+ bIjbCECcGrOUU0l5n0lgQH6vaDrRUkrhPu+xjmRw+GZ5eULOCTWKQ/HX4MIwaKuG3caw xW9Q== X-Received: by 10.66.221.199 with SMTP id qg7mr14488223pac.13.1386495379607; Sun, 08 Dec 2013 01:36:19 -0800 (PST) Received: from zhabar.gateway.2wire.net (76-253-2-5.lightspeed.sntcca.sbcglobal.net. [76.253.2.5]) by mx.google.com with ESMTPSA id gh3sm9840028pbb.2.2013.12.08.01.36.18 for (version=SSLv3 cipher=RC4-SHA bits=128/128); Sun, 08 Dec 2013 01:36:19 -0800 (PST) Date: Sun, 8 Dec 2013 01:36:14 -0800 From: Justin Hibbits To: Bruce Evans Subject: Re: svn commit: r259080 - in head/sys: dev/iicbus geom/cache geom/journal Message-ID: <20131208013614.6dc63ef9@zhabar.gateway.2wire.net> In-Reply-To: <20131208123144.U883@besplex.bde.org> References: <201312071955.rB7JtYNH001792@svn.freebsd.org> <20131208123144.U883@besplex.bde.org> X-Mailer: Claws Mail 3.9.2 (GTK+ 2.24.19; powerpc64-portbld-freebsd11.0) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: svn-src-head@freebsd.org, Justin Hibbits , src-committers@freebsd.org, svn-src-all@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 08 Dec 2013 09:36:20 -0000 On Sun, 8 Dec 2013 13:53:30 +1100 (EST) Bruce Evans wrote: > On Sat, 7 Dec 2013, Justin Hibbits wrote: > > > Log: > > Fix some integer signs. These unsigned integers should all be > > signed. > > > > Found by: clang (powerpc64) > > Unsigned integers are indeed used excessively. They should only be > used if values larger than INT_MAX need to be represented, but values > larger than UINT_MAX need not be represented. This is a very small > target (just 1 more bit). > > > Modified: head/sys/dev/iicbus/max6690.c > > ============================================================================== > > --- head/sys/dev/iicbus/max6690.c Sat Dec 7 19:39:38 > > 2013 (r259079) +++ head/sys/dev/iicbus/max6690.c Sat > > Dec 7 19:55:34 2013 (r259080) @@ -362,7 +362,7 @@ > > max6690_sensor_sysctl(SYSCTL_HANDLER_ARG struct max6690_softc *sc; > > struct max6690_sensor *sens; > > int error; > > - unsigned int temp; > > + int temp; > > > > dev = arg1; > > sc = device_get_softc(dev); > > > > This fixes one style bug (non-use of the BSD spelling u_int) but adds > another (int declarations not grouped together). > > > Modified: head/sys/geom/cache/g_cache.c > > ============================================================================== > > --- head/sys/geom/cache/g_cache.c Sat Dec 7 19:39:38 > > 2013 (r259079) +++ head/sys/geom/cache/g_cache.c Sat > > Dec 7 19:55:34 2013 (r259080) @@ -67,7 +67,7 @@ static > > u_int g_cache_used_hi = 20; static int > > sysctl_handle_pct(SYSCTL_HANDLER_ARGS) > > { > > - u_int val = *(u_int *)arg1; > > + int val; > > int error; > > > > error = sysctl_handle_int(oidp, &val, 0, req); > > > > This adds the larger non-style bug that val is used uninitialized. > Compilers should warn about this. This results in stack garbage > being copied out instead of the "old" value. > > sysctl_handle_u_int() doesn't exist, so sysctl_handle_int() os often > abused to handle u_int's and worse (sometimes "opaque" types, where > it usually works accidentally if the type is 32 bits, and sometimes > even works accidentally if the type is larger). clang seems to be > complaining about this abuse. Why does it only complain here? Ah, > maybe it is not smart enough to complain about this, but is just > complaining about the (val < 0) range check. > > This adds 2 (lexical) style bugs: > - the int declarations are not grouped together > > This misses fixing nearby bogus unsigned variables. The cast for > these is more bogus than before. Indeed, the whole function is > horrible, and this change is sort of backwards since it gives a sign > mismatch with the underlying variables: > > % static u_int g_cache_used_lo = 5; > % static u_int g_cache_used_hi = 20; > > Example of excessive use of unsigned types. These variables only need > to handle percentages between 0 and 100. > > % static int > > Style bug (missing blank line); > > % sysctl_handle_pct(SYSCTL_HANDLER_ARGS) > % { > % u_int val = *(u_int *)arg1; > > This is the old version. It has another style bug (initialization in > declaration). The initialization wouldn't have been lost so easily > if it had been in the correct place. > > But the type was correct, since it is bug-for-bug constent with the > underlying variables. > > % int error; > % > % error = sysctl_handle_int(oidp, &val, 0, req); > > sysctl_handle_int() was abused to handle the u_int (arg1 points to > one of the above 2 variables). > > % if (error || !req->newptr) > % return (error); > > 2 style bugs (2 boolean comparisions of non-booleans). Strict KNF > doesn't even use the ! operator to test booleansl; it uses an > explicit " == 0" test for this, though it sometimes doesn't write out > "!= 0" for the reverse test. > > % if (val < 0 || val > 100) > % return (EINVAL); > > The first test in this is redundant, This is apparently what clang > was complaining about. The warning for that can be fixed by removing > the test. However, I don't like this fix. It depends on the type > being unsigned. The code becomes more fragile, and would break if > the type were changed to the correct one (signed). > > % if ((arg1 == &g_cache_used_lo && val > g_cache_used_hi) || > % (arg1 == &g_cache_used_hi && g_cache_used_lo > val)) > % return (EINVAL); > % *(u_int *)arg1 = val; > > There are only 2 variables handled by this function, so we use arg1 > to set the correct one. However, the previous statement has decided > which of the variables is being set, and we could use that to set it. > Or better, convert arg1 from void * to u_int * up front, and use that > throughout instead of several casts of arg1. > > % return (0); > % } > % SYSCTL_PROC(_kern_geom_cache, OID_AUTO, used_lo, > CTLTYPE_UINT|CTLFLAG_RW, > > Style bugs: > - missing blank line. This is intentional for grouping the function > with its variables, as above, but still ugly. > - missing spaces around binary operator '|'. This bug is too common > for this operator, and for CTLFLAG*. But the spaces are even more > needed for '|' than for '+' or '*', since '|' looks more like an > alphanumeric character. > > % &g_cache_used_lo, 0, sysctl_handle_pct, "IU", ""); > % SYSCTL_PROC(_kern_geom_cache, OID_AUTO, used_hi, > CTLTYPE_UINT|CTLFLAG_RW, % &g_cache_used_hi, 0, > sysctl_handle_pct, "IU", ""); > > Style bugs: > - non-KNF continuation indent. This bug is too common for SYSCTL*(), > although someone once did a fairly global sweep to fix it for them. > - no description. > > This sysctl is over-engineered. A simple SYSCTL_UINT() is enough. > There are many more-critical sysctls that don't have any range > checking. The fix is not to bloat the kernel by adding range checking > to them all. With several thousand sysctls, the range checking code > could easily be over-engineered enough to double the size of an > already massively bloated kernel. OTOH, there may still be runtime > errors from sloppy caclulations using the percentage variables. The > calculations are of the form (x * pct / 100) and not the more robust > but fuzzier (x / 100 * pct). pct will normally be promoted to the > type of x in these expressions (often to 64 bits, and then overflow > is far off). Overflow is only affected by pct being unsigned if x is > int or shorter. > > > Modified: head/sys/geom/journal/g_journal.c > > ============================================================================== > > --- head/sys/geom/journal/g_journal.c Sat Dec 7 19:39:38 > > 2013 (r259079) +++ head/sys/geom/journal/g_journal.c > > Sat Dec 7 19:55:34 2013 (r259080) @@ -168,7 +168,7 @@ > > SYSCTL_UINT(_kern_geom_journal_cache, OI static int > > g_journal_cache_switch_sysctl(SYSCTL_HANDLER_ARGS) > > { > > - u_int cswitch; > > + int cswitch; > > int error; > > > > cswitch = g_journal_cache_switch; > > > > 2 style bugs (int variables not grouped; int variables not sorted). > > This sysctl is missing most of the style bugs discussed above: > - the transfer variable cswitch is not initialized in its declaration. > Its initialization didn't get deleted. > - the comparisons of the non-booleans are non-boolean. > - the percentage variable is used in the robust way (x / 100 * pct). > - the SYSCTL()s use KNF continuation indentation > - there are spaces around the binary operator '|' > - the SYSCTL()s have descriptions. > > This leaves mainly type errors (excessive use of unsigned variables). > There is a different inconsistency: the u_int g_journal_cache_switch > has formatting data "I" instead of "IU". Both will work due to the > range checking and assumptions that the machine is not exotic, but > there is no reason to not to use a consistent format. > > Another common style bug/API design error visible here is using > sysctl_handle_int() on generic variables. You have to know that the > variable type is int to use it or u_int, int32_t or uint32_t to abuse > it. The width of the variable is in the sysctl data and is usually > correct. sysctl_handle_opaque() is usable on generic variables but > doesn't work so well (it prevents type checking and has more locking > overheads). When sysctl_handle_int() is used, the width in the sysctl > data should be used to detect size mismatches. It is used, but there > are bugs in the error handling for this (something like ignoring size > mismatches for short i/o's in only 1 direction). > > Bruce I just reverted these two changes in r259096. Rather than fix what I did and change the rest of the files, I'm leaving them for another day, and reverted my changes so they're at least back to the working order from before. - Justin