backquote bugfixes, extension_path changes, command "echo"

Hi Rusty,

I have found three bugs in your changes to my backquote patches, the following patches fix them. Another patch eliminates extension_path and modifies $PATH the way I proposed today, and the final one implements an "echo" command, which is required for the "backquotes" nfsim test (for nfsim-testsuite).

Max

Mon Oct 10 21:11:02 CEST 2005 [email protected]
* typo: refer to line[i] instead of *line

diff -rN -u old-nfsim-0/core/tui.c new-nfsim-0/core/tui.c
@@ -337,7 +337,7 @@
 		if (isspace(line[i])) {
 			line[i] = '\0';
 			prevspace = true;
-		} else if (*line == '`') {
+		} else if (line[i] == '`') {
 			/* FIXME: Assumes ` forms arg by itself. */
 			argv[argc++] = backquote(line, &i);
 		} else if (prevspace) {

Mon Oct 10 22:15:58 CEST 2005 [email protected]
* remove extension_path, alter $PATH instead

diff -rN -u old-nfsim-0/core/core.c new-nfsim-0/core/core.c
@@ -427,6 +427,27 @@
 		(*p)();
 }
 
+static void add_to_path(const char *dir) {
+	int ret;
+	struct stat st;
+	char *old_path, *new_path;
+
+	ret = stat(dir, &st);
+	if (ret == -1 || !S_ISDIR(st.st_mode))
+		return;
+
+	old_path = getenv("PATH");
+	if (old_path == NULL)
+		old_path = "";
+
+	new_path = talloc_asprintf(NULL, "%s:%s",
+				   dir, old_path);
+
+	setenv("PATH", new_path, 1);
+
+	talloc_free(new_path);
+}
+
 int main(int argc, char **argv)
 {
 	int c, input_fd = STDIN_FILENO;

@@ -454,6 +475,10 @@
 	} else if (get_failtest())
 		barf("Not clever enough to use --failtest interactively");
 
+	add_to_path(".");
+	if (getenv("NFSIM_EXTPATH"))
+		add_to_path(getenv("NFSIM_EXTPATH"));
+
 	/* Hack to make users' lives easier: set LD_LIBRARY_PATH for
 	 * fakesockopt.so, based on where we are. */
 	p = strrchr(argv[0], '/');

@@ -467,7 +492,10 @@
 		setenv("LD_LIBRARY_PATH", path, 1);
 		talloc_free(path);
 		module_path = talloc_asprintf(NULL, "%s/nfsim", LIBDIR);
-		extension_path = module_path;
+
+		path = talloc_asprintf(NULL, "%s/nfsim/extensions", LIBDIR);
+		add_to_path(path);
+		talloc_free(path);
 	} else {
 		char *oldpath, *path;
 

@@ -480,13 +508,14 @@
 		talloc_free(path);
 		module_path = talloc_asprintf(NULL, "%.*s/netfilter/ipv4",
 					      (int)(p - argv[0]), argv[0]);
-		extension_path = talloc_asprintf(NULL, "%.*s/tools/extensions",
-					      (int)(p - argv[0]), argv[0]);
+
+		path = talloc_asprintf(NULL, "%.*s/nfsim/extensions",
+				       (int)(p - argv[0]), argv[0]);
+		add_to_path(path);
+		talloc_free(path);
 	}
 	if (getenv("NFSIM_MODPATH"))
 		module_path = getenv("NFSIM_MODPATH");
-	if (getenv("NFSIM_EXTPATH"))
-		extension_path = getenv("NFSIM_EXTPATH");
 
 	/* Don't do failures during initialization. */
 	suppress_failtest++;

diff -rN -u old-nfsim-0/core/tui.c new-nfsim-0/core/tui.c
@@ -37,7 +37,6 @@
 int tui_abort_on_fail;
 int tui_quiet;
 int tui_linenum = 1;
-char *extension_path;
 static bool stop;
 
 struct command {

@@ -289,8 +288,8 @@
 		script_fail("no matching \"`\" found");
 
 	len = end - (line + *off);
-	cmdstr = talloc_asprintf(line, "%s/%.*s",
-				 extension_path, len, line + *off);
+	cmdstr = talloc_asprintf(line, "%.*s",
+				 len, line + *off);
 	cmdfile = popen(cmdstr, "r");
 	if (!cmdfile)
 		script_fail("failed to popen '%s': %s\n",

Mon Oct 10 22:52:09 CEST 2005 [email protected]
* fix rusty's off-by-one error in backquote()

diff -rN -u old-nfsim-0/core/tui.c new-nfsim-0/core/tui.c
@@ -339,6 +339,7 @@
 		} else if (line[i] == '`') {
 			/* FIXME: Assumes ` forms arg by itself. */
 			argv[argc++] = backquote(line, &i);
+			--i;
 		} else if (prevspace) {
 			/* If it is a comment, stop before we process `` */
 			if (argc == 0 && line[i] == '#')

Mon Oct 10 23:45:49 CEST 2005 [email protected]
* implement the echo command

Tue Oct 11 00:03:04 CEST 2005 [email protected]
* handle concatenated backquotes

Index: 00simulator/02backquote.sim
===================================================================
@@ -0,0 +1,20 @@
+expect echo foo
+echo foo
+
+expect echo bar
+echo `echo -n bar`
+
+expect echo foo bar
+echo foo `echo -n bar`
+
+expect echo foobar
+echo foo`echo -n bar`
+
+expect echo barfoo
+echo `echo -n bar`foo
+
+expect echo farboo
+echo `echo -n far``echo -n boo`
+
+expect echo far0boo
+echo `echo -n far`0`echo -n boo`

Thanks Max!

Since it looks like we're going to work together more and more in the
future, I'll comment on these patches in detail. They're not wrong, but my projects have a preferred style (which, unfortunately for you, you can only figure out by mails like this).

I've applied the first fix (line[i]), no changes. I changed your out-by-one fix because it makes as much sense for backquote() to only process up to the trailing quote (it adds one, then we subtract one). I think this makes it slightly more readable, but needs a comment still:

        --- core/tui.c	(revision 4344)
        +++ core/tui.c	(working copy)
        @@ -274,7 +274,7 @@
         	return dest;
         }
         
        -/* Process `command`. */
        +/* Process `command`: update off to point to tail backquote */
         static char *backquote(char *line, unsigned int *off)
         {
         	char *end, *cmdstr, *str;
        @@ -296,8 +296,8 @@
         		script_fail("failed to popen '%s': %s\n",
         			    cmdstr, strerror(errno));
         
        -	/* Jump over tail backquote. */
        -	*off += len + 1;
        +	/* Jump to backquote. */
        +	*off += len;
         
         	/* Read command output. */
         	used = 0;

The echo patch was great, logical, exactly how I would have done it. Except that the recent change to ensure argv[] is a talloc pointer means we can hang the buffer off this, and it will be freed for us after the command is run. I also fixed the synopsis XML, and removed a trailing line. So here are the changes:

        --- tools/echo.c	2005-10-11 21:09:10.000000000 +1000
        +++ tools/echo-new.c	2005-10-11 21:05:11.000000000 +1000
        @@ -34,9 +34,6 @@
              <title><command>echo</command></title>
              <para>Print a message</para>
              <cmdsynopsis>
        -      <arg choice="req"><replaceable>text...</replaceable></arg>
        -     </cmdsynopsis>
        -     <cmdsynopsis>
               <command>echo</command>
               <arg choice="req"><replaceable>text...</replaceable></arg>
              </cmdsynopsis>
        @@ -47,26 +44,13 @@
         
         static bool echo(int argc, char **argv)
         {
        -	char buffer[8192];
        -	size_t length = 0, next_length;
        +	char *buffer;
         	int i;
         
        -	for (i = 1; i < argc; i++) {
        -		next_length = strlen(argv[i]);
        -		if (length + next_length + 1 >= sizeof(buffer)) {
        -			nfsim_log(LOG_ALWAYS, "Argument list too long");
        -			return false;
        -		}
        -
        -		if (length > 0)
        -			buffer[length++] = ' ';
        -
        -		memcpy(buffer + length, argv[i], next_length);
        -		length += next_length;
        -	}
        -
        -	buffer[length] = 0;
        -
        +	buffer = talloc_strdup(argv, "");
        +	for (i = 1; i < argc; i++)
        +		buffer = talloc_asprintf_append(buffer, "%s%s",
        +						i == 1 ? "" : " ", argv[i]);
         	nfsim_log(LOG_ALWAYS, "%s", buffer);
         
         	return true;
        @@ -78,4 +62,3 @@
         }
         
         init_call(init);
        -

Finally, you no doubt realize that the code to allow backquote within an argument was ugly. I thought about this, and decided to use this solution:

        Index: core/tui.c
        ===================================================================
        --- core/tui.c	(revision 4344)
        +++ core/tui.c	(working copy)
        @@ -321,36 +321,50 @@
         	return escape(str, used);
         }
         
        +static char *append_char(char **argv, unsigned int argc, char c)
        +{
        +	if (!argv[argc])
        +		return talloc_asprintf(argv, "%c", c);
        +	return talloc_asprintf_append(argv[argc], "%c", c);
        +}
        +
        +static char *append_string(char **argv, unsigned int argc, const char *str)
        +{
        +	if (!argv[argc])
        +		return talloc_asprintf(argv, "%s", str);
        +	return talloc_asprintf_append(argv[argc], "%s", str);
        +}
        +
         static void process_line(char *line, unsigned int off)
         {
         	unsigned int argc, i;
        -	bool prevspace = true;
         	char **argv;
         
         	if (tui_echo_commands)
         		printf("%u:%s\n", tui_linenum, line + off);
         
         	/* Talloc argv off line so commands can use it for auto-cleanup. */
        -	argv = talloc_array(line, char *, TUI_MAX_ARGS+1);
        +	argv = talloc_zero_array(line, char *, TUI_MAX_ARGS+1);
         	argc = 0;
         	for (i = off; line[i]; i++) {
         		if (isspace(line[i])) {
        -			line[i] = '\0';
        -			prevspace = true;
        +			/* If anything in this arg, move to next. */
        +			if (argv[argc])
        +				argc++;
         		} else if (line[i] == '`') {
        -			/* FIXME: Assumes ` forms arg by itself. */
        -			argv[argc++] = backquote(line, &i);
        -		} else if (prevspace) {
        +			char *inside = backquote(line, &i);
        +			argv[argc] = append_string(argv, argc, inside);
        +		} else {
         			/* If it is a comment, stop before we process `` */
        -			if (argc == 0 && line[i] == '#')
        +			if (!argv[0] && line[i] == '#')
         				goto out;
        -			argv[argc++] = line+i;
        -			prevspace = false;
        +
        +			argv[argc] = append_char(argv, argc, line[i]);
         		}
         	}
         
        -	if (argc > 0) {
        -		argv[argc] = NULL;
        +	if (argv[0]) {
        +		argv[++argc] = NULL;
         		tui_do_command(argc, argv, tui_abort_on_fail);
         	}

A bad analogy is like a leaky screwdriver -- Richard Braakman