Rework --prog to make arg processing safer

Previously the --prog option required the program-to-be-run to be
specified as a single string. This meant that shell escaping would be
lost in translation and a sub-shell would be run. Rework --prog to not
take an argument and accept the arguments left after option processing
has ended as the argv for the program-to-be-run.

As we have the program as an argv, run_program2() can now be used to run
it, and now that run_program() is no longer used we can remove it and
remove the '2' from run_program2.

New usage example:

 # iowatcher -p -t foo -d /dev/sda3 sleep 10
 running blktrace blktrace -b 8192 -a queue -a complete -a issue -a notify -D . -d /dev/sda3 -o foo
 running 'sleep' '10'
 sleep exited with 0
 ...

Docs have been updated accordingly.

Signed-off-by: Andrew Price <anprice@redhat.com>

5 files changed, 47 insertions, 48 deletions

diff --git a/blkparse.c b/blkparse.c
index 447d14c..e732af7 100644
--- a/blkparse.c
+++ b/blkparse.c
@@ -801,7 +801,9 @@ static int dump_traces(struct tracelist *traces, int count, char *dumpfile)
 		argv[i++] = tl->name;
 	}
 
-	err = run_program2(argc, argv, 0, NULL);
+	err = run_program(argc, argv, 1, NULL);
+	if (err)
+		fprintf(stderr, "%s exited with %d, expected 0\n", argv[0], err);
 	free(argv);
 	return err;
 }
diff --git a/iowatcher.1 b/iowatcher.1
index 8a06126..d9b77b8 100644
--- a/iowatcher.1
+++ b/iowatcher.1
@@ -5,7 +5,7 @@ iowatcher - Create visualizations from blktrace results
 
 .SH SYNOPSIS
 .B iowatcher
-\fIOPTIONS...\fR
+\fI[options]\fR [--] \fI[program arguments ...]\fR
 
 .SH DESCRIPTION
 iowatcher graphs the results of a blktrace run.  It can graph the result of an existing blktrace, start a new blktrace, or start a new blktrace and a benchmark run.  It can then create an image or movie of the IO from a given trace.  iowatcher can produce either SVG files or movies in mp4 format (with ffmpeg) or ogg format (with png2theora).
@@ -21,8 +21,16 @@ Controls which device you are tracing.  You can only trace one device at a time
 \fB-D, --blktrace-destination\fP <destination>
 Destination for blktrace.
 .TP
-\fB-p, --prog\fP <program>
-Program to run while blktrace is run.
+\fB-p, --prog\fP
+Run a program while blktrace is run. The program and its arguments must be
+specified after all other options.  Note that this option previously required
+the program to be given as a single argument but it now flags that iowatcher
+should expect extra arguments to be run during the trace.
+.TP
+\fB--\fP
+End option parsing. If \fB--prog\fP is specified, everything after \fB--\fP is
+the program to be run. This can be useful if the program name could otherwise
+be mistaken for an option.
 .TP
 \fB-K, --keep-movie-svgs\fP
 Keep the SVG files generated for movie mode.
diff --git a/main.c b/main.c
index ebbcf3d..c9ddd9c 100644
--- a/main.c
+++ b/main.c
@@ -148,7 +148,8 @@ static char *blktrace_devices[MAX_DEVICES_PER_TRACE];
 static int num_blktrace_devices = 0;
 static char *blktrace_outfile = "trace";
 static char *blktrace_dest_dir = ".";
-static char *program_to_run = NULL;
+static char **prog_argv = NULL;
+static int prog_argc = 0;
 static char *ffmpeg_codec = "libx264";
 
 static void alloc_mpstat_gld(struct trace_file *tf)
@@ -1307,7 +1308,7 @@ enum {
 	HELP_LONG_OPT = 1,
 };
 
-char *option_string = "F:T:t:o:l:r:O:N:d:D:p:m::h:w:c:x:y:a:C:PK";
+char *option_string = "+F:T:t:o:l:r:O:N:d:D:pm::h:w:c:x:y:a:C:PK";
 static struct option long_options[] = {
 	{"columns", required_argument, 0, 'c'},
 	{"fio-trace", required_argument, 0, 'F'},
@@ -1320,7 +1321,7 @@ static struct option long_options[] = {
 	{"only-graph", required_argument, 0, 'O'},
 	{"device", required_argument, 0, 'd'},
 	{"blktrace-destination", required_argument, 0, 'D'},
-	{"prog", required_argument, 0, 'p'},
+	{"prog", no_argument, 0, 'p'},
 	{"movie", optional_argument, 0, 'm'},
 	{"codec", optional_argument, 0, 'C'},
 	{"keep-movie-svgs", no_argument, 0, 'K'},
@@ -1343,7 +1344,7 @@ static void print_usage(void)
 		"\t-F (--fio-trace): fio bandwidth trace (more than one allowed)\n"
 		"\t-l (--label): trace label in the graph\n"
 		"\t-o (--output): output file name (SVG only)\n"
-		"\t-p (--prog): program to run while blktrace is run\n"
+		"\t-p (--prog): run a program while blktrace is run\n"
 		"\t-K (--keep-movie-svgs keep svgs generated for movie mode\n"
 		"\t-m (--movie [=spindle|rect]): create IO animations\n"
 		"\t-C (--codec): ffmpeg codec. Use ffmpeg -codecs to list\n"
@@ -1420,9 +1421,9 @@ static int parse_options(int ac, char **av)
 {
 	int c;
 	int disabled = 0;
+	int p_flagged = 0;
 
 	while (1) {
-		// int this_option_optind = optind ? optind : 1;
 		int option_index = 0;
 
 		c = getopt_long(ac, av, option_string,
@@ -1476,7 +1477,7 @@ static int parse_options(int ac, char **av)
 			}
 			break;
 		case 'p':
-			program_to_run = strdup(optarg);
+			p_flagged = 1;
 			break;
 		case 'K':
 			keep_movie_svgs = 1;
@@ -1549,6 +1550,18 @@ action_err:
 			break;
 		}
 	}
+
+	if (optind < ac && p_flagged) {
+		prog_argv = &av[optind];
+		prog_argc = ac - optind;
+	} else if (p_flagged) {
+		fprintf(stderr, "--prog or -p given but no program specified\n");
+		exit(1);
+	} else if (optind < ac) {
+		fprintf(stderr, "Extra arguments '%s'... (and --prog not specified)\n", av[optind]);
+		exit(1);
+	}
+
 	return 0;
 }
 
@@ -1629,20 +1642,14 @@ int main(int ac, char **av)
 		}
 
 		start_mpstat(path);
-		if (program_to_run) {
-			ret = run_program(program_to_run);
-			if (ret) {
-				fprintf(stderr, "failed to run %s\n",
-					program_to_run);
-				exit(1);
-			}
-			wait_for_tracers();
-		} else {
-			/* no program specified, just wait for
-			 * blktrace to exit
-			 */
-			wait_for_tracers();
+
+		if (prog_argv && prog_argc) {
+			ret = run_program(prog_argc, prog_argv, 1, NULL);
+			if (ret != 127)
+				printf("%s exited with %d\n", prog_argv[0], ret);
 		}
+
+		wait_for_tracers();
 		free(path);
 	}
 
diff --git a/tracers.c b/tracers.c
index 12efb4e..4844deb 100644
--- a/tracers.c
+++ b/tracers.c
@@ -164,22 +164,7 @@ int start_blktrace(char **devices, int num_devices, char *trace_name, char *dest
 	return 0;
 }
 
-int run_program(char *str)
-{
-	int ret;
-
-	fprintf(stderr, "running program %s\n", str);
-	ret = system(str);
-	if (ret == -1) {
-		fprintf(stderr, "failed to run program %s error %s\n", str, strerror(errno));
-		stop_all_tracers();
-		return -errno;
-	}
-	stop_all_tracers();
-	return 0;
-}
-
-int wait_program(pid_t pid, const char *pname, int expexit)
+int wait_program(pid_t pid, const char *pname)
 {
 	int status;
 	int ret = 0;
@@ -189,9 +174,6 @@ int wait_program(pid_t pid, const char *pname, int expexit)
 		ret = WEXITSTATUS(status);
 		if (ret == 127) /* spawnp failed after forking */
 			fprintf(stderr, "Failed to run '%s'\n", pname);
-		else if (ret != expexit)
-			fprintf(stderr, "'%s' exit status %d, expected %d\n",
-			        pname, ret, expexit);
 	} else if (WIFSIGNALED(status)) {
 		fprintf(stderr, "'%s' killed by signal %d\n", pname, WTERMSIG(status));
 		ret = -1;
@@ -199,7 +181,7 @@ int wait_program(pid_t pid, const char *pname, int expexit)
 	return ret;
 }
 
-int run_program2(int argc, char **argv, int expexit, pid_t *pid)
+int run_program(int argc, char **argv, int wait, pid_t *pid)
 {
 	int i;
 	int err;
@@ -213,8 +195,8 @@ int run_program2(int argc, char **argv, int expexit, pid_t *pid)
 	err = posix_spawnp(&_pid, argv[0], NULL, NULL, argv, environ);
 	if (err != 0) {
 		fprintf(stderr, "Could not run '%s': %s\n", argv[0], strerror(err));
-	} else if (expexit >= 0) {
-		err = wait_program(_pid, argv[0], expexit);
+	} else if (wait) {
+		err = wait_program(_pid, argv[0]);
 	} else if (!pid) {
 		fprintf(stderr, "Warning: %s (%ld): Not saving pid and not waiting for it.\n",
 		        argv[0], (long)_pid);
@@ -227,6 +209,7 @@ int run_program2(int argc, char **argv, int expexit, pid_t *pid)
 int wait_for_tracers(void)
 {
 	int status = 0;
+	stop_all_tracers();
 	if (blktrace_pid != 0) {
 		waitpid(blktrace_pid, &status, WUNTRACED);
 		blktrace_pid = 0;
diff --git a/tracers.h b/tracers.h
index d0b8b6b..ed57f5d 100644
--- a/tracers.h
+++ b/tracers.h
@@ -17,9 +17,8 @@
  */
 #ifndef __IOWATCH_TRACERS
 #define __IOWATCH_TRACERS
-int run_program(char *str);
-int run_program2(int argc, char **argv, int expexit, pid_t *pid);
-int wait_program(pid_t pid, const char *pname, int expexit);
+int run_program(int argc, char **argv, int wait, pid_t *pid);
+int wait_program(pid_t pid, const char *pname);
 int stop_blktrace(void);
 int start_blktrace(char **devices, int num_devices, char *trace_name, char *dest);
 int start_mpstat(char *trace_name);