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)
> @@ -28,7 +28,7 @@
> .\" From: @(#)sysctl.8 8.1 (Berkeley) 6/6/93
> .\" $FreeBSD$
> .\"
> -.Dd January 17, 2011
> +.Dd November 22, 2012
> .Dt SYSCTL 8
> .Os
> .Sh NAME
> @@ -37,6 +37,7 @@
> .Sh SYNOPSIS
> .Nm
> .Op Fl bdehiNnoqx
> +.Op Fl f Ar filename
> .Ar name Ns Op = Ns Ar value
> .Ar ...
> .Nm
> @@ -80,6 +81,11 @@
> 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)
> @@ -56,13 +56,16 @@
> #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 *);
> @@ -74,7 +77,7 @@
> {
>
> (void)fprintf(stderr, "%s\n%s\n",
> - "usage: sysctl [-bdehiNnoqx] name[=value] ...",
> + "usage: sysctl [-bdehiNnoqx] [-f filename] name[=value] ...",
> " sysctl [-bdehNnoqx] -a");
> exit(1);
> }
> @@ -83,12 +86,13 @@
> 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 */
> @@ -106,6 +110,9 @@
> case 'e':
> eflag = 1;
> break;
> + case 'f':
> + conffile = strdup(optarg);
> + break;
> case 'h':
> hflag = 1;
> break;
> @@ -146,13 +153,15 @@
> 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);
> }
>
> /*
> @@ -160,8 +169,8 @@
> * 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;
> @@ -173,17 +182,30 @@
> 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);
> }
> @@ -191,15 +213,20 @@
>
> 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) {
> @@ -215,16 +242,20 @@
> 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 ||
> @@ -233,22 +264,28 @@
> (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);
> @@ -256,16 +293,16 @@
> 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;
> @@ -273,7 +310,7 @@
> 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;
> @@ -282,16 +319,16 @@
> 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;
> @@ -299,8 +336,8 @@
> /* 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);
> @@ -309,18 +346,20 @@
> 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)
> @@ -332,8 +371,46 @@
> 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
_______________________________________________
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
