Re: RFC: sysctl -f filename

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.
    With that in mind, reviewing the original proposed code...

...

+.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.

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.

 .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;

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).

+
+	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.