On 1 December 2012 16:30, Garrett Cooper <yanegomi_at_gmail.com> wrote: > On Sat, Dec 1, 2012 at 3:37 PM, Konstantin Belousov <kostikbel_at_gmail.com> wrote: >> On Sun, Dec 02, 2012 at 08:21:50AM +0900, Hiroki Sato wrote: >>> Garrett Cooper <yanegomi_at_gmail.com> wrote >>> in <CAGH67wShpcmOKhc09+MP5c-AOm7EAPG+Gqv=J0PRq0sGuTzKRQ_at_mail.gmail.com>: >>> >>> ya> On Sat, Dec 1, 2012 at 2:10 PM, Garrett Cooper <yanegomi_at_gmail.com> wrote: >>> ya> > Why change the tool when we can change the rc script to do the >>> ya> > right thing? I have a patch I'm working on to resolve this (you hit an >>> ya> > itch I've been meaning to scratch for a little while). >>> ya> >>> ya> This should work. I also refactored the script to get it down to >>> ya> 80 columns. I've attached the debug output and the diff for the debug >>> ya> version of the script. >>> >>> You will find out the following test case does not work (this is one >>> of the test strings I used): >>> >>> kern.domainname="c$EDITOR.\"\ hoge\ \"\#hoge2\\$ \# h$$\#oge"# >>> >>> The reason why I changed sysctl(8) was that the rc.d/sysctl script >>> was too complex and slow even if it could support meta characters in >>> shell script syntax. I created several prototypes as script but >>> noticed that keeping consistency was quite difficult and >>> maintainability was poor due to tricky handling of variables. >>> >>> Although my patch in the previous email does not support meta >>> characters completely, I still think it is more reasonable to >>> implement this functionality on the sysctl(8) side. >> >> I fully agree with the proposal to add the -f switch to the sysctl(8). >> This is consistent with several other administrative tools. Putting >> the ability to parse and apply arbitrary sysctl.conf-like file into >> the rc script is weird. > > The point was to augment rc.d/sysctl to do the right thing in most > sane cases. What's shown above frankly doesn't make sense for a domain > name, and while I agree that it's a good negative test, it seems a bit > on the insane side for a real world example. > I was trying to provide an alternative using an existing > functioning system, instead of having to go and hack a widely used > utility to function in a way that isn't standard. Since when sysctl(8) is standard and the "-f file" option is not? http://man.NetBSD.org/man/sysctl+8 I also agree with the proposal that adding "-f file" directly to sysctl(8) sounds like a much better approach than going through the non-standard (and slow) rc.d route. > With that in mind, reviewing the original proposed code... > > ... > > +.It Fl f Ar filename I think this might be s/filename/file/, e.g. as in at(1), apmd(8), boot0cfg(8), bsdlabel(8), devd(8), restore(8), pfctl(8) etc. > +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. > > gcooper> This doesn't discuss the file format in complete, gory > detail, and no examples were added to aid the reader in how things can > be done in the new world order. +1. Also, note that NetBSD's sysctl(8) supports line continuation with "\"; is this something that would at all be useful for sysctl? Then it should probably be implemented and documented from the start, to ensure that we don't have to needlessly break backwards compatibility with a future change. > > .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] ...", I think the "-f file" should be a separate usage paradigm here, and, most likely, it should not be possible to use both "-f file" and "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; > > gcooper> style bug. > > 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); > > gcooper> Not checking for != NULL. > > ... > > /* > _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) > > gcooper> Since this is being modified, could string be changed to > const char * for completeness? > > ... > > _at__at_ -191,15 +213,20 _at__at_ > > if (len < 0) { > if (iflag) > - return; > - if (qflag) > + return (1); > > gcooper> This should be return (0); > > + 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); > > gcooper> This should be arguably be `return (0);` too. > > ... > > +static int > +parsefile(const char *filename) > +{ > + FILE *file; > + char line[BUFSIZ], *p; > + int warncount = 0, lineno = 0; > > gcooper> Style bugs (declaring and initializing on the same line). Not according to style(9): http://www.freebsd.org/cgi/man.cgi?query=style&sektion=9&manpath=FreeBSD+9.0-RELEASE << Be careful to not obfuscate the code by initializing variables in the declarations. Use this feature only thoughtfully. DO NOT use function calls in initializers. >> I think the above initialisations are just fine and used thoughtfully. C. > > + > + 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) { > > gcooper> Style bug (needs space between `while` and `(`). > > + if (p == line || *(p-1) != '\\') { > > gcooper> Why are you allowing '\#' ? > > + *p = '\0'; > > ... > > static int > Index: etc/rc.d/sysctl > =================================================================== > --- etc/rc.d/sysctl (revision 243756) > +++ etc/rc.d/sysctl (working copy) > _at__at_ -8,51 +8,27 _at__at_ > . /etc/rc.subr > > name="sysctl" > +command=/sbin/sysctl > > gcooper> Please quote this for consistency with the bulk majority of > rc.d scripts.Received on Sun Dec 02 2012 - 02:32:02 UTC
This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:40:32 UTC