Re: RFC: sysctl -f filename

From: Jilles Tjoelker <jilles_at_stack.nl>
Date: Sun, 2 Dec 2012 14:00:22 +0100
On Sun, Dec 02, 2012 at 01:50:48AM +0900, Hiroki Sato wrote:
>  I would like comments about the attached patch for sysctl(8) to add a
>  new option "-f filename".  It supports reading of a file with
>  key=value lines.

>  As you probably know, we already have /etc/sysctl.conf and it is
>  processed by rc.d/sysctl shell script in a line-by-line basis.  The
>  problem I want to fix is a confusing syntax of /etc/sysctl.conf.  The
>  file supports a typical configuration file syntax but problematic in
>  some cases.  For example:

>   kern.coredump=1

>  works well in /etc/sysctl.conf, but

>   kern.coredump="1"

>  does not work.  Similarly, it is difficult to use whitespaces and "#"
>  in the value:

>   OK: kern.domainname=domain\ name\ with\ spaces
>   NG: kern.domainname="domain name with spaces"
>   NG: kern.domainname=domain\ name\ including\ #\ character
>   NG: kern.domainname=domain\ name\ including\ \#\ character

>  The attached patch solves them, and in addition it displays an error
>  message with a line number if there is something wrong in the file
>  like this:

>   % cat -n /etc/sysctl.conf
>   ...
>   10  kern.coredump=1
>   11  kern.coredump2=1
>   ...

>   % /etc/rc.d/sysctl start
>   sysctl: kern.coredump at line 10: Operation not permitted
>   sysctl: unknown oid 'kern.coredump2' at line 11

>   # /etc/rc.d/sysctl start
>   kern.coredump: 1 -> 1
>   sysctl: unknown oid 'kern.coredump2' at line 11

>  Any comments are welcome.

The idea is OK but the format of the file is messy. Further comments
inline.

> Index: sbin/sysctl/sysctl.8
> ===================================================================
> --- sbin/sysctl/sysctl.8	(revision 243756)
> +++ sbin/sysctl/sysctl.8	(working copy)
> _at__at_ -28,7 +28,7 _at__at_
>  .\"	From: _at_(#)sysctl.8	8.1 (Berkeley) 6/6/93
>  .\" $FreeBSD$
>  .\"
> -.Dd January 17, 2011
> +.Dd November 22, 2012
>  .Dt SYSCTL 8
>  .Os
>  .Sh NAME
> _at__at_ -37,6 +37,7 _at__at_
>  .Sh SYNOPSIS
>  .Nm
>  .Op Fl bdehiNnoqx
> +.Op Fl f Ar filename
>  .Ar name Ns Op = Ns Ar value
>  .Ar ...
>  .Nm
> _at__at_ -80,6 +81,11 _at__at_
>  or
>  .Fl n
>  is specified, or a variable is being set.
> +.It Fl f Ar filename
> +Specify a file which contains a pair of name and value in each line.
> +.Nm
> +reads and processes the specified file first and then processes the name
> +and value pairs in the command line argument.

The file format is not described.

>  .It Fl h
>  Format output for human, rather than machine, readability.
>  .It Fl i
> Index: sbin/sysctl/sysctl.c
> ===================================================================
> --- sbin/sysctl/sysctl.c	(revision 243756)
> +++ sbin/sysctl/sysctl.c	(working copy)
> _at__at_ -56,13 +56,16 _at__at_
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> +#include <sysexits.h>
>  #include <unistd.h>
> 
> +static const char *conffile;
>  static int	aflag, bflag, dflag, eflag, hflag, iflag;
> -static int	Nflag, nflag, oflag, qflag, xflag, warncount;
> +static int	Nflag, nflag, oflag, qflag, xflag;
> 
>  static int	oidfmt(int *, int, char *, u_int *);
> -static void	parse(char *);
> +static int	parsefile(const char *);
> +static int	parse(char *, int);
>  static int	show_var(int *, int);
>  static int	sysctl_all(int *oid, int len);
>  static int	name2oid(char *, int *);
> _at__at_ -74,7 +77,7 _at__at_
>  {
> 
>  	(void)fprintf(stderr, "%s\n%s\n",
> -	    "usage: sysctl [-bdehiNnoqx] name[=value] ...",
> +	    "usage: sysctl [-bdehiNnoqx] [-f filename] name[=value] ...",
>  	    "       sysctl [-bdehNnoqx] -a");
>  	exit(1);
>  }
> _at__at_ -83,12 +86,13 _at__at_
>  main(int argc, char **argv)
>  {
>  	int ch;
> +	int warncount = 0;
> 
>  	setlocale(LC_NUMERIC, "");
>  	setbuf(stdout,0);
>  	setbuf(stderr,0);
> 
> -	while ((ch = getopt(argc, argv, "AabdehiNnoqwxX")) != -1) {
> +	while ((ch = getopt(argc, argv, "Aabdef:hiNnoqwxX")) != -1) {
>  		switch (ch) {
>  		case 'A':
>  			/* compatibility */
> _at__at_ -106,6 +110,9 _at__at_
>  		case 'e':
>  			eflag = 1;
>  			break;
> +		case 'f':
> +			conffile = strdup(optarg);
> +			break;
>  		case 'h':
>  			hflag = 1;
>  			break;
> _at__at_ -146,13 +153,15 _at__at_
>  		usage();
>  	if (aflag && argc == 0)
>  		exit(sysctl_all(0, 0));
> -	if (argc == 0)
> +	if (argc == 0 && conffile == NULL)
>  		usage();
> -
>  	warncount = 0;
> +	if (conffile != NULL)
> +		warncount += parsefile(conffile);
>  	while (argc-- > 0)
> -		parse(*argv++);
> -	exit(warncount);
> +		warncount += parse(*argv++, 0);
> +
> +	return (warncount);
>  }
> 
>  /*
> _at__at_ -160,8 +169,8 _at__at_
>   * Lookup and print out the MIB entry if it exists.
>   * Set a new value if requested.
>   */
> -static void
> -parse(char *string)
> +static int
> +parse(char *string, int lineno)
>  {
>  	int len, i, j;
>  	void *newval = 0;
> _at__at_ -173,17 +182,30 _at__at_
>  	int64_t i64val;
>  	uint64_t u64val;
>  	int mib[CTL_MAXNAME];
> -	char *cp, *bufp, buf[BUFSIZ], *endptr, fmt[BUFSIZ];
> +	char *cp, *bufp, buf[BUFSIZ], *endptr, fmt[BUFSIZ], line[BUFSIZ];
>  	u_int kind;
> 
> +	memset(line, 0, sizeof(line));
> +	if (lineno)
> +		snprintf(line, sizeof(line), " at line %d", lineno);

BUFSIZ seems a bit bogus here. For 'line', 30 will be sufficient.

>  	bufp = buf;
> -	if (snprintf(buf, BUFSIZ, "%s", string) >= BUFSIZ)
> -		errx(1, "oid too long: '%s'", string);
> +	if (snprintf(buf, BUFSIZ, "%s", string) >= BUFSIZ) {
> +		warn("oid too long: '%s'%s", string, line);
> +		return (1);
> +	}

This is unrelated to your patch, but this should probably be a strdup()
instead to avoid an arbitrary length limit.

>  	if ((cp = strchr(string, '=')) != NULL) {
>  		*strchr(buf, '=') = '\0';
>  		*cp++ = '\0';
>  		while (isspace(*cp))
>  			cp++;
> +		/* Strip a pair of " or ' if any. */
> +		switch (*cp) {
> +		case '\"':
> +		case '\'':
> +			if (cp[strlen(cp) - 1] == *cp)
> +				cp[strlen(cp) - 1] = '\0';
> +			cp++;
> +		}

What if I want to set a value which starts and ends with quotes? There
is no backslash escaping so weird-looking quotes may have to be added.

Furthermore, I think a value passed on the command line should remain
literal.

>  		newval = cp;
>  		newsize = strlen(cp);
>  	}
> _at__at_ -191,15 +213,20 _at__at_
> 
>  	if (len < 0) {
>  		if (iflag)
> -			return;
> -		if (qflag)
> +			return (1);
> +		if (qflag)
>  			exit(1);
>  		else
> -			errx(1, "unknown oid '%s'", bufp);
> +			errx(1, "unknown oid '%s'%s", bufp, line);
>  	}
> 
> -	if (oidfmt(mib, len, fmt, &kind))
> -		err(1, "couldn't find format of oid '%s'", bufp);
> +	if (oidfmt(mib, len, fmt, &kind)) {
> +		warn("couldn't find format of oid '%s'%s", bufp, line);
> +		if (!iflag)
> +			exit(1);
> +		else
> +			return (1);
> +	}
> 
>  	if (newval == NULL || dflag) {
>  		if ((kind & CTLTYPE) == CTLTYPE_NODE) {
> _at__at_ -215,16 +242,20 _at__at_
>  				putchar('\n');
>  		}
>  	} else {
> -		if ((kind & CTLTYPE) == CTLTYPE_NODE)
> -			errx(1, "oid '%s' isn't a leaf node", bufp);
> +		if ((kind & CTLTYPE) == CTLTYPE_NODE) {
> +			warn("oid '%s' isn't a leaf node%s", bufp, line);
> +			return (1);
> +		}
> 
>  		if (!(kind & CTLFLAG_WR)) {
>  			if (kind & CTLFLAG_TUN) {
> -				warnx("oid '%s' is a read only tunable", bufp);
> -				errx(1, "Tunable values are set in /boot/loader.conf");
> -			} else {
> -				errx(1, "oid '%s' is read only", bufp);
> -			}
> +				warnx("oid '%s' is a read only tunable%s",
> +				    bufp, line);
> +				warn("Tunable values are set in /boot/loader.conf");
> +			} else
> +				warn("oid '%s' is read only%s", bufp, line);
> +
> +			return (1);
>  		}
> 
>  		if ((kind & CTLTYPE) == CTLTYPE_INT ||
> _at__at_ -233,22 +264,28 _at__at_
>  		    (kind & CTLTYPE) == CTLTYPE_ULONG ||
>  		    (kind & CTLTYPE) == CTLTYPE_S64 ||
>  		    (kind & CTLTYPE) == CTLTYPE_U64) {
> -			if (strlen(newval) == 0)
> -				errx(1, "empty numeric value");
> +			if (strlen(newval) == 0) {
> +				warn("empty numeric value%s", line);
> +				return (1);
> +			}
>  		}
> 
>  		switch (kind & CTLTYPE) {
>  			case CTLTYPE_INT:
>  				if (strcmp(fmt, "IK") == 0) {
> -					if (!set_IK(newval, &intval))
> -						errx(1, "invalid value '%s'",
> -						    (char *)newval);
> +					if (!set_IK(newval, &intval)) {
> +						warn("invalid value '%s'%s",
> +						    (char *)newval, line);
> +						return (1);
> +					}
>   				} else {
>  					intval = (int)strtol(newval, &endptr,
>  					    0);
> -					if (endptr == newval || *endptr != '\0')
> -						errx(1, "invalid integer '%s'",
> -						    (char *)newval);
> +					if (endptr == newval || *endptr != '\0') {
> +						warn("invalid integer '%s'%s",
> +						    (char *)newval, line);
> +						return (1);
> +					}
>  				}
>  				newval = &intval;
>  				newsize = sizeof(intval);
> _at__at_ -256,16 +293,16 _at__at_
>  			case CTLTYPE_UINT:
>  				uintval = (int) strtoul(newval, &endptr, 0);
>  				if (endptr == newval || *endptr != '\0')
> -					errx(1, "invalid unsigned integer '%s'",
> -					    (char *)newval);
> +					errx(1, "invalid unsigned integer "
> +					    "'%s'%s", (char *)newval, line);
>  				newval = &uintval;
>  				newsize = sizeof(uintval);
>  				break;
>  			case CTLTYPE_LONG:
>  				longval = strtol(newval, &endptr, 0);
>  				if (endptr == newval || *endptr != '\0')
> -					errx(1, "invalid long integer '%s'",
> -					    (char *)newval);
> +					errx(1, "invalid long integer '%s'%s",
> +					    (char *)newval, line);
>  				newval = &longval;
>  				newsize = sizeof(longval);
>  				break;
> _at__at_ -273,7 +310,7 _at__at_
>  				ulongval = strtoul(newval, &endptr, 0);
>  				if (endptr == newval || *endptr != '\0')
>  					errx(1, "invalid unsigned long integer"
> -					    " '%s'", (char *)newval);
> +					    " '%s'%s", (char *)newval, line);
>  				newval = &ulongval;
>  				newsize = sizeof(ulongval);
>  				break;
> _at__at_ -282,16 +319,16 _at__at_
>  			case CTLTYPE_S64:
>  				i64val = strtoimax(newval, &endptr, 0);
>  				if (endptr == newval || *endptr != '\0')
> -					errx(1, "invalid int64_t '%s'",
> -					    (char *)newval);
> +					errx(1, "invalid int64_t '%s'%s",
> +					    (char *)newval, line);
>  				newval = &i64val;
>  				newsize = sizeof(i64val);
>  				break;
>  			case CTLTYPE_U64:
>  				u64val = strtoumax(newval, &endptr, 0);
>  				if (endptr == newval || *endptr != '\0')
> -					errx(1, "invalid uint64_t '%s'",
> -					    (char *)newval);
> +					errx(1, "invalid uint64_t '%s'%s",
> +					    (char *)newval, line);
>  				newval = &u64val;
>  				newsize = sizeof(u64val);
>  				break;
> _at__at_ -299,8 +336,8 _at__at_
>  				/* FALLTHROUGH */
>  			default:
>  				errx(1, "oid '%s' is type %d,"
> -					" cannot set that", bufp,
> -					kind & CTLTYPE);
> +					" cannot set that%s", bufp,
> +					kind & CTLTYPE, line);
>  		}
> 
>  		i = show_var(mib, len);
> _at__at_ -309,18 +346,20 _at__at_
>  				putchar('\n');
>  			switch (errno) {
>  			case EOPNOTSUPP:
> -				errx(1, "%s: value is not available",
> -					string);
> +				warn("%s: value is not available%s", string,
> +				    line);
> +				return (1);
>  			case ENOTDIR:
> -				errx(1, "%s: specification is incomplete",
> -					string);
> +				warn("%s: specification is incomplete%s",
> +				    string, line);
> +				return (1);
>  			case ENOMEM:
> -				errx(1, "%s: type is unknown to this program",
> -					string);
> +				warn("%s: type is unknown to this program%s",
> +				    string, line);
> +				return (1);
>  			default:
> -				warn("%s", string);
> -				warncount++;
> -				return;
> +				warn("%s%s", string, line);
> +				return (1);
>  			}
>  		}
>  		if (!bflag)
> _at__at_ -332,8 +371,46 _at__at_
>  			putchar('\n');
>  		nflag = i;
>  	}
> +
> +	return (0);
>  }
> 
> +static int
> +parsefile(const char *filename)
> +{
> +	FILE *file;
> +	char line[BUFSIZ], *p;
> +	int warncount = 0, lineno = 0;
> +
> +	file = fopen(filename, "r");
> +	if (file == NULL)
> +		err(EX_NOINPUT, "%s", filename);
> +	while (fgets(line, sizeof(line), file) != NULL) {
> +		lineno++;
> +		p = line;
> +		/* Replace the first # with \0. */
> +		while((p = strchr(p, '#')) != NULL) {

sh(1) only recognizes # specially at the beginning of a word but perhaps
you want to do it differently.

> +			if (p == line || *(p-1) != '\\') {

This '\\' thing looks a bit strange. There is no backslash removal later
on.

> +				*p = '\0';
> +				break;
> +			}
> +			p++;
> +		}
> +		p = line;
> +		while (isspace((int)p[strlen(p) - 1]))

If strlen(p) == 0, then this is an access out of bounds.

The cast should be (unsigned char) not (int).

> +			p[strlen(p) - 1] = '\0';
> +		while (isspace((int)*p))
> +			p++;
> +		if (*p == '\0')
> +			continue;
> +		else
> +			warncount += parse(p, lineno);
> +	}
> +	fclose(file);
> +
> +	return (warncount);
> +}
> +
>  /* These functions will dump out various interesting structures. */
> 
>  static int

-- 
Jilles Tjoelker
Received on Sun Dec 02 2012 - 12:00:28 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:40:32 UTC