On 09/21/2017 03:52 PM, Petri Latvala wrote: > Instead of redirecting output to pipes and forking, redirect after > forking to avoid having to carefully unredirect before logging > anything. > > igt@tools_test@sysfs_l3_parity had a racy condition where it prints > the output of intel_l3_parity prepended by [cmd], but that ended up > being printed again prepended by [cmd] because output was redirected, > causing outputs to appear multiple times. This patch fixes that. > > CC: Abdiel Janulgue <[email protected]> > Signed-off-by: Petri Latvala <[email protected]>
Thanks for fixing this. Series is Reviewed-by: Abdiel Janulgue <[email protected]> > --- > lib/igt_core.c | 115 > ++++++++++++++++++++------------------------------------- > 1 file changed, 40 insertions(+), 75 deletions(-) > > diff --git a/lib/igt_core.c b/lib/igt_core.c > index 9f4ee68b..47b4682d 100644 > --- a/lib/igt_core.c > +++ b/lib/igt_core.c > @@ -2237,58 +2237,23 @@ FILE *__igt_fopen_data(const char* igt_srcdir, const > char* igt_datadir, > return fp; > } > > -struct output_pipe { > - int output_fd; > - int save_fd; > - int read_fd; > - int write_fd; > - bool redirected; > - enum igt_log_level log_level; > -}; > - > -static bool redirect_output(struct output_pipe *p, int output_fd, > - enum igt_log_level level) > +static void log_output(int *fd, enum igt_log_level level) > { > - int fds[2]; > - > - if (pipe(fds) == -1) > - goto err; > - > - /* save output */ > - if ((p->save_fd = dup(output_fd)) == -1) > - goto err; > - > - /* Redirect output to our buffer */ > - if (dup2(fds[1], output_fd) == -1) > - goto err; > - > - p->output_fd = output_fd; > - p->read_fd = fds[0]; > - p->write_fd = fds[1]; > - p->redirected = true; > - p->log_level = level; > - > - return true; > -err: > - close(fds[0]); > - close(fds[1]); > - close(p->save_fd); > + ssize_t len; > + char buf[PIPE_BUF]; > > - return false; > -} > + if (*fd < 0) > + return; > > -static void unredirect_output(struct output_pipe *p) > -{ > - if (!p->redirected) > + memset(buf, 0, sizeof(buf)); > + len = read(*fd, buf, sizeof(buf)); > + if (len <= 0) { > + close(*fd); > + *fd = -1; > return; > + } > > - /* read_fd is closed separately. We still need to read its > - * buffered contents after un-redirecting the stream. > - */ > - close(p->write_fd); > - dup2(p->save_fd, p->output_fd); > - close(p->save_fd); > - p->redirected = false; > + igt_log(IGT_LOG_DOMAIN, level, "[cmd] %s", buf); > } > > /** > @@ -2304,48 +2269,48 @@ static void unredirect_output(struct output_pipe *p) > */ > int igt_system(const char *command) > { > -#define OUT 0 > -#define ERR 1 > - struct output_pipe op[2]; > - int i, status; > + int outpipe[2] = { -1, -1 }; > + int errpipe[2] = { -1, -1 }; > + int status; > struct igt_helper_process process = {}; > - char buf[PIPE_BUF]; > > - if (!redirect_output(&op[OUT], STDOUT_FILENO, IGT_LOG_INFO)) > + if (pipe(outpipe) < 0) > goto err; > - if (!redirect_output(&op[ERR], STDERR_FILENO, IGT_LOG_WARN)) > + if (pipe(errpipe) < 0) > goto err; > > igt_fork_helper(&process) { > - igt_assert(execl("/bin/sh", "sh", "-c", command, > - (char *) NULL) != -1); > + close(outpipe[0]); > + close(errpipe[0]); > + > + if (dup2(outpipe[1], STDOUT_FILENO) < 0) > + goto child_err; > + if (dup2(errpipe[1], STDERR_FILENO) < 0) > + goto child_err; > + > + execl("/bin/sh", "sh", "-c", command, > + (char *) NULL); > + > + child_err: > + exit(EXIT_FAILURE); > } > > - for (i = 0; i < ARRAY_SIZE(op); i++) { > - struct output_pipe *current = &op[i]; > + close(outpipe[1]); > + close(errpipe[1]); > > - /* Unredirect so igt_log() works */ > - unredirect_output(current); > - memset(buf, 0, sizeof(buf)); > - while (read(current->read_fd, buf, sizeof(buf)) > 0) { > - igt_log(IGT_LOG_DOMAIN, current->log_level, > - "[cmd] %s", buf); > - memset(buf, 0, sizeof(buf)); > - } > - close(current->read_fd); > + while (outpipe[0] >= 0 || errpipe[0] >= 0) { > + log_output(&outpipe[0], IGT_LOG_INFO); > + log_output(&errpipe[0], IGT_LOG_WARN); > } > + > status = igt_wait_helper(&process); > > return WEXITSTATUS(status); > err: > - /* Failed to redirect one or both streams. Roll back changes. */ > - for (i = 0; i < ARRAY_SIZE(op); i++) { > - if (!op[i].redirected) > - continue; > - close(op[i].read_fd); > - unredirect_output(&op[i]); > - } > - > + close(outpipe[0]); > + close(outpipe[1]); > + close(errpipe[0]); > + close(errpipe[1]); > return -1; > } > > _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
