iowatcher: 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>

diff --git a/iowatcher/blkparse.c b/iowatcher/blkparse.c
index 447d14cf9d6de2d153e7184baf17b5f6e1452d02..e732af7dc781ecde48b36c0fb31d6582c2458e69 100644 (file)
--- a/iowatcher/blkparse.c
+++ b/iowatcher/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/iowatcher.1 b/iowatcher/iowatcher.1
index 8a061261b08fc4f85676e4359a9b97131fcc6c51..d9b77b8cc2a676ed38c213e7d9f6719dbac9074b 100644 (file)
--- a/iowatcher/iowatcher.1
+++ b/iowatcher/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/iowatcher/main.c b/iowatcher/main.c
index ebbcf3db2bd4038f2eca1ba02e7eb65db4367dcd..c9ddd9c92b6a802421f39fafd4e44dea10e105c8 100644 (file)
--- a/iowatcher/main.c
+++ b/iowatcher/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/iowatcher/tracers.c b/iowatcher/tracers.c
index 12efb4e6ffea26921b66c8d58d916171761415e1..4844deb482663cb1780e67db82ba04ba2d681914 100644 (file)
--- a/iowatcher/tracers.c
+++ b/iowatcher/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/iowatcher/tracers.h b/iowatcher/tracers.h
index d0b8b6ba768d7b8a3c38d7070805f5112efe5786..ed57f5de33b6cd9a82b130e68ee05cd22f3f2f08 100644 (file)
--- a/iowatcher/tracers.h
+++ b/iowatcher/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);