svn commit: r272144 - head/sbin/sysctl

[Mateusz Guzik]

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>