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>

4 files changed, 52 insertions, 99 deletions

diff --git a/blkparse.c b/blkparse.c
index e732af7..f837f39 100644
--- a/blkparse.c
+++ b/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/main.c b/main.c
index c9ddd9c..4441dfc 100644
--- a/main.c
+++ b/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/tracers.c b/tracers.c
index 6ecf24c..4c3d10d 100644
--- a/tracers.c
+++ b/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/tracers.h b/tracers.h
index 77c31a6..9b4f6a5 100644
--- a/tracers.h
+++ b/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