iowatcher: Convert start_mpstat to run_program

For consistency and deduplication, use run_program in start_mpstat.  Add
the ability to pass a path to run_program, which will be opened in the
spawned process and used as stdout, in order to capture mpstat output.
This fixes a tricky descriptor leak in start_mpstat which could have
caused a race condition if it was fixed with close().

Some output formatting tweaks have also been added and a bug from a
previous patch, where tracers were killed immediately when -p wasn't
specified, has been fixed.

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

diff --git a/iowatcher/blkparse.c b/iowatcher/blkparse.c
index e732af7dc781ecde48b36c0fb31d6582c2458e69..f837f39de9727c0add5f292d340432c338b01308 100644 (file)
--- a/iowatcher/blkparse.c
+++ b/iowatcher/blkparse.c
@@ -801,7 +801,7 @@ static int dump_traces(struct tracelist *traces, int count, char *dumpfile)
                argv[i++] = tl->name;
        }
 
-       err = run_program(argc, argv, 1, NULL);
+       err = run_program(argc, argv, 1, NULL, NULL);
        if (err)
                fprintf(stderr, "%s exited with %d, expected 0\n", argv[0], err);
        free(argv);

diff --git a/iowatcher/main.c b/iowatcher/main.c
index c9ddd9c92b6a802421f39fafd4e44dea10e105c8..4441dfc55fb74debfb6e058388fb8946644220a9 100644 (file)
--- a/iowatcher/main.c
+++ b/iowatcher/main.c
@@ -33,6 +33,7 @@
 #include <getopt.h>
 #include <limits.h>
 #include <float.h>
+#include <signal.h>
 
 #include "plot.h"
 #include "blkparse.h"
@@ -1620,16 +1621,9 @@ int main(int ac, char **av)
        }
 
        if (num_blktrace_devices) {
-               char *path;
-
+               char *path = join_path(blktrace_dest_dir, blktrace_outfile);
                dest_mkdir(blktrace_dest_dir);
-               if (num_blktrace_devices > 1) {
-                       snprintf(line, line_len, "%s/%s", blktrace_dest_dir,
-                                blktrace_outfile);
-                       dest_mkdir(line);
-               }
-
-               path = join_path(blktrace_dest_dir, blktrace_outfile);
+               dest_mkdir(path);
 
                snprintf(line, line_len, "%s.dump", path);
                unlink(line);
@@ -1637,19 +1631,24 @@ int main(int ac, char **av)
                ret = start_blktrace(blktrace_devices, num_blktrace_devices,
                                     blktrace_outfile, blktrace_dest_dir);
                if (ret) {
-                       fprintf(stderr, "exiting due to blktrace failure\n");
-                       exit(1);
+                       perror("Exiting due to blktrace failure");
+                       exit(ret);
                }
 
-               start_mpstat(path);
+               snprintf(line, line_len, "%s.mpstat", path);
+               ret = start_mpstat(line);
+               if (ret) {
+                       perror("Exiting due to mpstat failure");
+                       exit(ret);
+               }
 
                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);
+                       run_program(prog_argc, prog_argv, 1, NULL, NULL);
+                       wait_for_tracers(SIGINT);
+               } else {
+                       printf("Tracing until interrupted...\n");
+                       wait_for_tracers(0);
                }
-
-               wait_for_tracers();
                free(path);
        }
 

diff --git a/iowatcher/tracers.c b/iowatcher/tracers.c
index 6ecf24ce4ce2c9faaf3ed8a69ecf3af72b37a1d0..4c3d10dfb5f7bbb5ce707f6c64da89344b7a538e 100644 (file)
--- a/iowatcher/tracers.c
+++ b/iowatcher/tracers.c
@@ -40,50 +40,13 @@
 
 extern char **environ;
 
-static int line_len = 1024;
-static char line[1024];
-
 static pid_t blktrace_pid = 0;
 static pid_t mpstat_pid = 0;
 
-char *mpstat_args[] = {
-       "mpstat",
-       "-P", "ALL", "1",
-       NULL,
-};
-
-int stop_tracer(pid_t *tracer_pid)
-{
-       int ret;
-       pid_t pid = *tracer_pid;
-       pid_t pid_ret;
-       int status = 0;
-
-       if (pid == 0)
-               return 0;
-
-       *tracer_pid = 0;
-       ret = kill(pid, SIGTERM);
-       if (ret) {
-               fprintf(stderr, "failed to stop tracer pid %lu error %s\n",
-                       (unsigned long)pid, strerror(errno));
-               return -errno;
-       }
-       pid_ret = waitpid(pid, &status, WUNTRACED);
-       if (pid_ret == pid && WIFEXITED(status) == 0) {
-               fprintf(stderr, "blktrace returns error %d\n", WEXITSTATUS(status));
-       }
-       return 0;
-}
-
 static void sig_handler_for_quit(int val)
 {
        fprintf(stderr, "Received signal %d. Terminating tracers.\n", val);
-       if (blktrace_pid) {
-               wait_program(blktrace_pid, "blktrace", SIGTERM);
-               blktrace_pid = 0;
-       }
-       stop_tracer(&mpstat_pid);
+       wait_for_tracers(SIGTERM);
 }
 
 int start_blktrace(char **devices, int num_devices, char *trace_name, char *dest)
@@ -127,7 +90,7 @@ int start_blktrace(char **devices, int num_devices, char *trace_name, char *dest
        argv[argc] = NULL;
        signal(SIGTERM, sig_handler_for_quit);
        signal(SIGINT, sig_handler_for_quit);
-       ret = run_program(argc, argv, 0, &blktrace_pid);
+       ret = run_program(argc, argv, 0, &blktrace_pid, NULL);
        return ret;
 }
 
@@ -151,6 +114,8 @@ int wait_program(pid_t pid, const char *pname, int sig)
                ret = WEXITSTATUS(status);
                if (ret == 127) /* spawnp failed after forking */
                        fprintf(stderr, "Failed to run '%s'\n", pname);
+               else
+                       fprintf(stderr, "Exit (%d): %s\n", ret, pname);
        } else if (WIFSIGNALED(status) && sig && WTERMSIG(status) != sig) {
                fprintf(stderr, "'%s' killed by signal %d\n", pname, WTERMSIG(status));
                ret = -1;
@@ -158,18 +123,26 @@ int wait_program(pid_t pid, const char *pname, int sig)
        return ret;
 }
 
-int run_program(int argc, char **argv, int wait, pid_t *pid)
+int run_program(int argc, char **argv, int wait, pid_t *pid, char *outpath)
 {
        int i;
        int err;
        pid_t _pid;
+       posix_spawn_file_actions_t facts;
+       posix_spawn_file_actions_t *factp = NULL;
 
-       fprintf(stderr, "running");
+       if (outpath != NULL) {
+               posix_spawn_file_actions_init(&facts);
+               posix_spawn_file_actions_addopen(&facts, 1, outpath, O_WRONLY|O_CREAT|O_TRUNC, 0600);
+               factp = &facts;
+       }
+
+       fprintf(stderr, "Start");
        for (i = 0; i < argc; i++)
-               fprintf(stderr, " '%s'", argv[i]);
+               fprintf(stderr, " %s", argv[i]);
        fprintf(stderr, "\n");
 
-       err = posix_spawnp(&_pid, argv[0], NULL, NULL, argv, environ);
+       err = posix_spawnp(&_pid, argv[0], factp, NULL, argv, environ);
        if (err != 0) {
                fprintf(stderr, "Could not run '%s': %s\n", argv[0], strerror(err));
        } else if (wait) {
@@ -183,53 +156,34 @@ int run_program(int argc, char **argv, int wait, pid_t *pid)
        return err;
 }
 
-int wait_for_tracers(void)
+int wait_for_tracers(int sig)
 {
+       int err;
        if (blktrace_pid != 0) {
-               int err = wait_program(blktrace_pid, "blktrace", SIGINT);
+               err = wait_program(blktrace_pid, "blktrace", sig);
                if (err)
                        exit(1);
                blktrace_pid = 0;
        }
        if (mpstat_pid != 0) {
-               int status = 0;
-               stop_tracer(&mpstat_pid);
-               waitpid(mpstat_pid, &status, WUNTRACED);
+               err = wait_program(mpstat_pid, "mpstat", sig);
+               if (err)
+                       exit(1);
                mpstat_pid = 0;
        }
        return 0;
 }
 
-int start_mpstat(char *trace_name)
+int start_mpstat(char *path)
 {
-       int fd;
-       pid_t pid;
-
-       snprintf(line, line_len, "%s.mpstat", trace_name);
-
-       fd = open(line, O_WRONLY | O_CREAT | O_TRUNC, 0600);
-       if (fd < 0) {
-               fprintf(stderr, "unable to open %s for writing err %s\n",
-                       line, strerror(errno));
-               exit(1);
-       }
-       pid = fork();
-       if (pid == 0) {
-               int ret;
-
-               close(1);
-               ret = dup2(fd, 1);
-               if (ret < 0) {
-                       fprintf(stderr, "failed to setup output file for mpstat\n");
-                       exit(1);
-               }
-               ret = execvp("mpstat", mpstat_args);
-               if (ret < 0) {
-                       fprintf(stderr, "failed to exec mpstat err %s\n",
-                               strerror(ret));
-                       exit(1);
-               }
-       }
-       mpstat_pid = pid;
-       return 0;
+       int ret;
+       int argc = 4;
+       char *argv[] = {
+               "mpstat",
+               "-P", "ALL", "1",
+               NULL,
+       };
+
+       ret = run_program(argc, argv, 0, &mpstat_pid, path);
+       return ret;
 }

diff --git a/iowatcher/tracers.h b/iowatcher/tracers.h
index 77c31a6d69f9aa6a29661f4349e269fd956ed95d..9b4f6a5480e4316aa5a147ccd7d88653b9f6c8b7 100644 (file)
--- a/iowatcher/tracers.h
+++ b/iowatcher/tracers.h
@@ -17,12 +17,12 @@
  */
 #ifndef __IOWATCH_TRACERS
 #define __IOWATCH_TRACERS
-int run_program(int argc, char **argv, int wait, pid_t *pid);
+int run_program(int argc, char **argv, int wait, pid_t *pid, char *stdoutpath);
 int wait_program(pid_t pid, const char *pname, int signal);
 int stop_blktrace(void);
 int start_blktrace(char **devices, int num_devices, char *trace_name, char *dest);
 int start_mpstat(char *trace_name);
-int wait_for_tracers(void);
+int wait_for_tracers(int sig);
 
 
 #endif