From 49559b611256ea542f3f9f74951bb787c418fbf8 Mon Sep 17 00:00:00 2001 From: Andrew Price Date: Sun, 27 Apr 2014 01:55:57 +0100 Subject: [PATCH] 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 --- iowatcher/blkparse.c | 2 +- iowatcher/main.c | 33 +++++++------ iowatcher/tracers.c | 112 +++++++++++++------------------------------ iowatcher/tracers.h | 4 +- 4 files changed, 52 insertions(+), 99 deletions(-) diff --git a/iowatcher/blkparse.c b/iowatcher/blkparse.c index e732af7..f837f39 100644 --- 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 c9ddd9c..4441dfc 100644 --- a/iowatcher/main.c +++ b/iowatcher/main.c @@ -33,6 +33,7 @@ #include #include #include +#include #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 6ecf24c..4c3d10d 100644 --- 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 77c31a6..9b4f6a5 100644 --- 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 -- 2.25.1