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 TjoelkerReceived 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