Date: Fri, 23 Oct 2015 16:21:00 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: "Conrad E. Meyer" <cem@freebsd.org> 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 Message-ID: <20151023145159.W937@besplex.bde.org> In-Reply-To: <201510222303.t9MN37D2093845@repo.freebsd.org> References: <201510222303.t9MN37D2093845@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20151023145159.W937>