From owner-svn-src-all@freebsd.org Fri Oct 23 05:21:05 2015 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id C5FB4A1630E; Fri, 23 Oct 2015 05:21:05 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail104.syd.optusnet.com.au (mail104.syd.optusnet.com.au [211.29.132.246]) by mx1.freebsd.org (Postfix) with ESMTP id 8A6981ED6; Fri, 23 Oct 2015 05:21:05 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c211-30-166-197.carlnfd1.nsw.optusnet.com.au (c211-30-166-197.carlnfd1.nsw.optusnet.com.au [211.30.166.197]) by mail104.syd.optusnet.com.au (Postfix) with ESMTPS id D3B9A422887; Fri, 23 Oct 2015 16:21:01 +1100 (AEDT) Date: Fri, 23 Oct 2015 16:21:00 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: "Conrad E. Meyer" cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r289773 - in head: sbin/sysctl sys/kern sys/sys In-Reply-To: <201510222303.t9MN37D2093845@repo.freebsd.org> Message-ID: <20151023145159.W937@besplex.bde.org> References: <201510222303.t9MN37D2093845@repo.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=cK4dyQqN c=1 sm=1 tr=0 a=KA6XNC2GZCFrdESI5ZmdjQ==:117 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=kj9zAlcOel0A:10 a=fprmH2DYPCcJMjTXhHkA:9 a=XyxJXbozhXr6ND9E:21 a=CbopKwBvR2axtTK3:21 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.20 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, 23 Oct 2015 05:21:05 -0000 On Thu, 22 Oct 2015, Conrad E. Meyer wrote: > Log: > Sysctl: Add common support for U8, U16 types > > Sponsored by: EMC / Isilon Storage Division This uses similar low-quality code to other sysctls, and more. > Modified: head/sbin/sysctl/sysctl.c > ============================================================================== > --- head/sbin/sysctl/sysctl.c Thu Oct 22 22:29:25 2015 (r289772) > +++ head/sbin/sysctl/sysctl.c Thu Oct 22 23:03:06 2015 (r289773) > @@ -96,6 +96,8 @@ static int ctl_size[CTLTYPE+1] = { > [CTLTYPE_ULONG] = sizeof(u_long), > [CTLTYPE_S64] = sizeof(int64_t), > [CTLTYPE_U64] = sizeof(uint64_t), > + [CTLTYPE_U8] = sizeof(uint8_t), > + [CTLTYPE_U16] = sizeof(uint16_t), > }; Unsorting. The table was sorted on size, then alphabetically. Alphabetical ordering also happens to give ordering on rank. (This ordering is the inverse of the ordering for declarations.) > > static const char *ctl_typename[CTLTYPE+1] = { > @@ -105,6 +107,8 @@ static const char *ctl_typename[CTLTYPE+ > [CTLTYPE_ULONG] = "unsigned long", > [CTLTYPE_S64] = "int64_t", > [CTLTYPE_U64] = "uint64_t", > + [CTLTYPE_U8] = "uint8_t", > + [CTLTYPE_U16] = "uint16_t", > }; Consistent unsorting. > > static void > @@ -221,6 +225,8 @@ parse(const char *string, int lineno) > int len, i, j; > const void *newval; > const char *newvalstr = NULL; > + uint8_t u8val; > + uint16_t u16val; > int intval; > unsigned int uintval; > long longval; Locally consistent, but backwards sorting. The *val variables were sorted on rank, and the addition matches this, but style(9) requires inverse sorting on rank. Other local variables are randomly ordered. > @@ -322,6 +328,8 @@ parse(const char *string, int lineno) > } > > switch (kind & CTLTYPE) { > + case CTLTYPE_U8: > + case CTLTYPE_U16: > case CTLTYPE_INT: > case CTLTYPE_UINT: > case CTLTYPE_LONG: Consistent sorting. The types are sorted on rank. Style(9) requires sorting case stylements alpahbetically, but it is good to break that here and sort on rank or inverse rank to match other orderings. > @@ -345,6 +353,16 @@ parse(const char *string, int lineno) > errno = 0; > > switch (kind & CTLTYPE) { > + case CTLTYPE_U8: > + u8val = (uint8_t)strtoul(newvalstr, &endptr, 0); > + newval = &u8val; > + newsize = sizeof(u8val); > + break; Bug: blind truncation gives overflow before overflow can be checked for. The overflow checking done by the subsequent errno check only checks for representability of the return value. Style bug: bogus cast to make the overflow more explicit and/or prevent the compiler from detecting the bug and misleading the reader into thinking that overflow is intentionally allowed. The cast usually has no other effect (the behaviour on conversion is only implementation-defined in the overflow case. Perhaps it can be different with the cast). The other assignments are mostly similarly broken: - CTLTYPE_INT: similar bogus cast to int - CTLTYPE_UINT: unsimilar bogus cast to int. The correct cast to u_int would have no effect, but casting to int gratuitously gives implementation defined behaviour even for representable values like UINT_MAX - CTLTYPE_LONG, CTLTYPE_ULONG: correct (null conversion with no cast) - CTLTYPE_S64: no cast, though overflow can occur if int64_t < intmax_t - CTLTYPE_U64: no cast. Correct since the type is unsigned - CTLTYPE_IMAX, CTLTYPE_UIMAX: unsupported. > + case CTLTYPE_U16: > + u16val = (uint16_t)strtoul(newvalstr, &endptr, 0); > + newval = &u16val; > + newsize = sizeof(u16val); > + break; As a bug, but another style bug from the cast (ling too long). > case CTLTYPE_INT: > if (strncmp(fmt, "IK", 2) == 0) > intval = strIKtoi(newvalstr, &endptr, fmt); The error handling is somewhat obfuscated by having general code after the switch statement to avoid almost repeating it for each case. It could be simplified by always assigning to uintmax_t uimaxval. Then do general error handling. Then do specific error handling (range checking for each case). The error handling also has a silly check for empty values before the main check. The general error handling finds empty values and much more as a special case of disallowing values with no digits (e.g., nonempty leading whitespace and/or sign before the empty value). So all the silly check does is give a special error message for empty values. Not worth it. Style bug: you forgot to add the new types to the silly check. > free(oval); > return (0); > > + case CTLTYPE_U8: > + case CTLTYPE_U16: > case CTLTYPE_INT: > case CTLTYPE_UINT: > case CTLTYPE_LONG: > @@ -902,6 +922,14 @@ show_var(int *oid, int nlen) > sep1 = ""; > while (len >= intlen) { > switch (kind & CTLTYPE) { > + case CTLTYPE_U8: > + umv = *(uint8_t *)p; > + mv = *(int8_t *)p; > + break; > + case CTLTYPE_U16: > + umv = *(uint16_t *)p; > + mv = *(int16_t *)p; > + break; CTLTYPE_I8 and CTLTYPE_I16 are not supported (nonexistent), so the assignments to mv are never used. They probably prevent the compiler warning about the missing support by detecting the bug as "mv possibly used uninitialized". > case CTLTYPE_INT: > case CTLTYPE_UINT: > umv = *(u_int *)p; > > Modified: head/sys/kern/kern_sysctl.c > ============================================================================== > --- head/sys/kern/kern_sysctl.c Thu Oct 22 22:29:25 2015 (r289772) > +++ head/sys/kern/kern_sysctl.c Thu Oct 22 23:03:06 2015 (r289773) > @@ -1128,6 +1128,70 @@ static SYSCTL_NODE(_sysctl, 5, oiddescr, > */ > > /* > + * Handle an int8_t, signed or unsigned. > + * Two cases: > + * a variable: point arg1 at it. > + * a constant: pass it in arg2. > + */ Only 1 case. Signed is not supported. The style of the comment matches the bad style of others. It is n-tuplicated ad nauseum. It has fancy formatting, but no indent protection. > + > +int > +sysctl_handle_8(SYSCTL_HANDLER_ARGS) > +{ > + int8_t tmpout; > + int error = 0; Style bugs copied. > + > + /* > + * Attempt to get a coherent snapshot by making a copy of the data. > + */ Bogus comment on probably unnecessary code. sysctl_int() might need to copy since int might not be atomic, although it is on all supported arches. But surely byte accesses are atomic? > + if (arg1) > + tmpout = *(int8_t *)arg1; > + else > + tmpout = arg2; Bogus cast. Only uint8_t is supported. This matches bugs in sysctl_handle_int(), except there both types are supported so the type pun saves space. sysctl(8) is more careful about making the types match exactly. > + error = SYSCTL_OUT(req, &tmpout, sizeof(tmpout)); > + > + if (error || !req->newptr) > + return (error); > + > + if (!arg1) > + error = EPERM; > + else > + error = SYSCTL_IN(req, arg1, sizeof(tmpout)); > + return (error); > +} This duplcates mounds of style bugs again: - blank lines to separate related code (error handling) - boolean checks on non-boolean > + > +/* > + * Handle an int16_t, signed or unsigned. > + * Two cases: > + * a variable: point arg1 at it. > + * a constant: pass it in arg2. > + */ > + > +int > +sysctl_handle_16(SYSCTL_HANDLER_ARGS) > +{ > + int16_t tmpout; > + int error = 0; > + > + /* > + * Attempt to get a coherent snapshot by making a copy of the data. > + */ > + if (arg1) > + tmpout = *(int16_t *)arg1; > + else > + tmpout = arg2; > + error = SYSCTL_OUT(req, &tmpout, sizeof(tmpout)); > + > + if (error || !req->newptr) > + return (error); > + > + if (!arg1) > + error = EPERM; > + else > + error = SYSCTL_IN(req, arg1, sizeof(tmpout)); > + return (error); > +} Similarly, except the copy might be needed even if int is atomic. > + > +/* > * Handle an int, signed or unsigned. > * Two cases: > * a variable: point arg1 at it. > > Modified: head/sys/sys/sysctl.h > ============================================================================== > --- head/sys/sys/sysctl.h Thu Oct 22 22:29:25 2015 (r289772) > +++ head/sys/sys/sysctl.h Thu Oct 22 23:03:06 2015 (r289773) > @@ -73,6 +73,8 @@ struct ctlname { > #define CTLTYPE_LONG 7 /* name describes a long */ > #define CTLTYPE_ULONG 8 /* name describes an unsigned long */ > #define CTLTYPE_U64 9 /* name describes an unsigned 64-bit number */ > +#define CTLTYPE_U8 0xa /* name describes an unsigned 8-bit number */ > +#define CTLTYPE_U16 0xb /* name describes an unsigned 16-bit number */ Style bug: spelling ids in hex. This should at least reserve values for the signed types. ISTR that the 4.4BSD version had null support for unsigned types. Then some hacks gave support for some unsigned types. S64 was spelled QUAD, and U64 was missing for a long time, but IIRC you could use a hack to print it though not input it. Indeed, the format was determined by the format string and not by CTLTYPE*, so you could print U64 using format "QU" to type pun a quad_t to uquad_t for printing. This was eventually cleaned up to remove the hacks. > @@ -319,6 +323,46 @@ TAILQ_HEAD(sysctl_ctx_list, sysctl_ctx_e > __arg, len, sysctl_handle_string, "A", __DESCR(descr)); \ > }) > > +/* Oid for an unsigned 8-bit int. If ptr is NULL, val is returned. */ > +#define SYSCTL_NULL_U8_PTR ((unsigned *)NULL) > +#define SYSCTL_U8(parent, nbr, name, access, ptr, val, descr) \ > + SYSCTL_OID(parent, nbr, name, \ > + CTLTYPE_U8 | CTLFLAG_MPSAFE | (access), \ > + ptr, val, sysctl_handle_8, "CU", descr); \ "CU" looks like never-used garbage. "QU" is still in the initializer for U64, (and the macro is still named with UQUAD instead of U64), but sysctl(8) no longer uses even the "Q" part. I think only old sysctl binaries benefit from having "QU". But the new "C" format is not supported by anything. Old sysctl binaries would be confused by even the size. > ... > +/* Oid for an unsigned 16-bit int. If ptr is NULL, val is returned. */ > +#define SYSCTL_NULL_U16_PTR ((unsigned *)NULL) > +#define SYSCTL_U16(parent, nbr, name, access, ptr, val, descr) \ > + SYSCTL_OID(parent, nbr, name, \ > + CTLTYPE_U16 | CTLFLAG_MPSAFE | (access), \ > + ptr, val, sysctl_handle_16, "SU", descr); \ > + CTASSERT((((access) & CTLTYPE) == 0 || \ > + ((access) & SYSCTL_CT_ASSERT_MASK) == CTLTYPE_U16) && \ > + sizeof(uint16_t) == sizeof(*(ptr))) "S" for uint16_t is even less supported and a larger misspelling than "C" for uint8_t. POSIX now requires chars to be 8 bits and uint8_t to be u_char, but nothing requries uint16_t to be u_short. Spelling the name of uint16_t with an "S" is like the old mistake spelling the name of uint64_t with a "Q". Bruce