From owner-svn-src-head@FreeBSD.ORG Sun Dec 8 02:54:05 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 84B50611; Sun, 8 Dec 2013 02:54:05 +0000 (UTC) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id 312B71AC2; Sun, 8 Dec 2013 02:54:04 +0000 (UTC) Received: from c122-106-156-23.carlnfd1.nsw.optusnet.com.au (c122-106-156-23.carlnfd1.nsw.optusnet.com.au [122.106.156.23]) by mail108.syd.optusnet.com.au (Postfix) with ESMTPS id EB8141A5541; Sun, 8 Dec 2013 13:53:55 +1100 (EST) Date: Sun, 8 Dec 2013 13:53:30 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Justin Hibbits Subject: Re: svn commit: r259080 - in head/sys: dev/iicbus geom/cache geom/journal In-Reply-To: <201312071955.rB7JtYNH001792@svn.freebsd.org> Message-ID: <20131208123144.U883@besplex.bde.org> References: <201312071955.rB7JtYNH001792@svn.freebsd.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.1 cv=YYGEuWhf c=1 sm=1 tr=0 a=ebeQFi2P/qHVC0Yw9JDJ4g==:117 a=PO7r1zJSAAAA:8 a=3HaF-T8NxwoA:10 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=ssdoF7uQumEA:10 a=-y4LA6a5D9EZghm1e9kA:9 a=CjuIK1q_8ugA:10 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@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 02:54:05 -0000 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