Date: Fri, 26 Sep 2014 03:03:18 +0200 From: Mateusz Guzik <mjguzik@gmail.com> To: d@delphij.net Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Xin LI <delphij@FreeBSD.org> Subject: Re: svn commit: r272144 - head/sbin/sysctl Message-ID: <20140926010317.GA21954@dft-labs.eu> In-Reply-To: <5424B11E.4030909@delphij.net> References: <201409252237.s8PMbScl041679@svn.freebsd.org> <20140925224035.GA6065@dft-labs.eu> <5424B11E.4030909@delphij.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Sep 25, 2014 at 05:19:42PM -0700, Xin Li wrote:
> - if (newval == NULL || dflag) {
> + if (newvalstr == NULL || dflag) {
> if ((kind & CTLTYPE) == CTLTYPE_NODE) {
> if (dflag) {
> i = show_var(mib, len);
> @@ -288,10 +314,22 @@ parse(const char *string, int lineno)
> (kind & CTLTYPE) == CTLTYPE_ULONG ||
> (kind & CTLTYPE) == CTLTYPE_S64 ||
> (kind & CTLTYPE) == CTLTYPE_U64) {
> - if (strlen(newval) == 0) {
> + if (strlen(newvalstr) == 0) {
> warnx("empty numeric value");
> return (1);
> }
> + } else if ((kind & CTLTYPE) != CTLTYPE_STRING) {
> + /*
> + * The only acceptable types are:
> + * CTLTYPE_INT, CTLTYPE_UINT,
> + * CTLTYPE_LONG, CTLTYPE_ULONG,
> + * CTLTYPE_S64, CTLTYPE_U64 and
> + * CTLTYPE_STRING.
> + */
> + warnx("oid '%s' is type %d,"
> + " cannot set that%s", bufp,
> + kind & CTLTYPE, line);
> + return (1);
> }
I think this should be converted to switch, +/-:
switch (kind & CTLTYPE) {
case CTLTYPE_INT:
case CTLTYPE_UINT:
case CTLTYPE_LONG:
case CTLTYPE_ULONG:
case CTLTYPE_S64:
case CTLTYPE_U64:
if (strlen(newvalstr) == 0) {
warnx("empty numeric value");
return (1);
}
break;
case CTLTYPE_STRING:
break;
default:
warnx("oid '%s' is type %d cannot set that%s",
bufp, kind & CTLTYPE, line);
return (1);
}
>
> errno = 0;
> @@ -298,91 +336,53 @@ parse(const char *string, int lineno)
>
> switch (kind & CTLTYPE) {
> default:
> - warnx("oid '%s' is type %d,"
> - " cannot set that%s", bufp,
> - kind & CTLTYPE, line);
> + /* NOTREACHED */
> return (1);
This one should be removed or should call abort() or something since it should
be impossible to get here.
> }
>
> + if (errno != 0 || endptr == newvalstr || *endptr != '\0') {
> + warnx("invalid %s '%s'%s", ctl_typename[kind & CTLTYPE],
> + newvalstr, line);
> + return (1);
> + }
> +
So one thing a big fan of is the fact that some values are still
assigned based on possibly bogus result. Some static analysis tool may
object.
But it's just a note.
In general I think it's fine.
> i = show_var(mib, len);
> if (sysctl(mib, len, 0, 0, newval, newsize) == -1) {
> if (!i && !bflag)
> @@ -665,33 +665,35 @@ S_bios_smap_xattr(size_t l2, void *p)
> #endif
>
--
Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140926010317.GA21954>