| Message ID | 1492676028-15372-1-git-send-email-abdiel.janulgue@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 20, 2017 at 11:13:45AM +0300, Abdiel Janulgue wrote: > Support executing external processes with the goal of capturing its > standard streams to the igt logging infrastructure in addition to its > exit status. > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Petri Latvala <petri.latvala@intel.com> > Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com> > --- > lib/igt_core.c | 151 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > lib/igt_core.h | 3 ++ > 2 files changed, 154 insertions(+) > > diff --git a/lib/igt_core.c b/lib/igt_core.c > index 403b942..8a7ba0d 100644 > --- a/lib/igt_core.c > +++ b/lib/igt_core.c > @@ -2073,3 +2073,154 @@ 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) > +{ > + int fds[2], flags; > + > + if (pipe(fds) == -1) > + return false; If this succeeds.... > + > + if ((flags = fcntl(fds[0], F_GETFL) == -1)) > + return false; > + > + flags |= O_NONBLOCK; > + if (fcntl(fds[0], F_SETFL, flags) == -1) > + return false; > + > + /* save output */ > + if ((p->save_fd = dup(output_fd)) == -1) > + return false; > + > + /* Redirect output to our buffer */ > + if (dup2(fds[1], output_fd) == -1) > + return false; .... and these don't, the pipe fds are never closed. Same for the save_fd if the dup2 fails, if the caller of this function doesn't call unredirect_output, as the return value might direct it. > + > + p->output_fd = output_fd; > + p->read_fd = fds[0]; > + p->write_fd = fds[1]; > + p->redirected = true; > + p->log_level = level; > + > + return true; > +} > + > +static bool unredirect_output(struct output_pipe *p) > +{ > + close(p->write_fd); > + if (dup2(p->save_fd, p->output_fd) == -1) > + return false; > + close(p->save_fd); > + > + p->redirected = false; > + > + return true; > +} > + > +/** > + * igt_exec: > + * > + * Executes the shell command specified in @command and capture its stdout and > + * stderr to igt_log or igt_warn respectively. > + * > + * Returns: The exit status of the executed process. -1 for failure. > + */ > +int igt_exec(const char *command) > +{ > +#define OUT 0 > +#define ERR 1 > + struct output_pipe op[2]; > + int i, sel_fd, status; > + fd_set fds; > + struct timeval timeout = { .tv_sec = 0, .tv_usec = 0 }; > + char buf[PIPE_BUF]; > + > + if (!redirect_output(&op[OUT], STDOUT_FILENO, IGT_LOG_INFO)) > + return -1; > + if (!redirect_output(&op[ERR], STDERR_FILENO, IGT_LOG_WARN)) > + return -1; If redirect_output for stderr returns false, unredirect_output for stdout will not get called. > + > + if ((status = system(command)) == -1) > + return -1; > + > + FD_ZERO(&fds); > + FD_SET(op[OUT].read_fd, &fds); > + FD_SET(op[ERR].read_fd, &fds); > + > + sel_fd = max(op[OUT].read_fd, op[ERR].read_fd); > + if (select(sel_fd + 1, &fds, NULL, NULL, &timeout) == -1) > + return -1; > + > + for (i = 0; i < ARRAY_SIZE(op); i++) { > + struct output_pipe *current = &op[i]; > + > + if (!FD_ISSET(current->read_fd, &fds)) { > + close(current->read_fd); > + if (!unredirect_output(current)) > + return -1; > + continue; > + } > + > + memset(buf, 0, sizeof(buf)); > + while (read(current->read_fd, buf, sizeof(buf)) > 0) { > + if (current->redirected) { > + if (!unredirect_output(current)) > + return -1; > + } > + igt_log(IGT_LOG_DOMAIN, current->log_level, > + "[cmd] %s", buf); > + memset(buf, 0, sizeof(buf)); > + } > + close(current->read_fd); > + } Unredirect_output calls for both pipes need to be called on all exit paths. In redirect_output you set only the read fd of the pipe() pair to O_NONBLOCK. That will make the executed command block on its writes indefinitely if it prints more than whatsitnow, 64kB? > + > + return WEXITSTATUS(status); > +} > + > +/** > + * igt_exec_quiet: > + * Similar to igt_exec(), except redirect output to /dev/null > + * > + * Returns: The exit status of the executed process. -1 for failure. > + */ > +int igt_exec_quiet(const char *command) > +{ > + int stderr_fd_copy, stdout_fd_copy, status, nullfd; > + > + /* redirect */ > + if ((nullfd = open("/dev/null", O_WRONLY)) == -1) > + return -1; > + if ((stdout_fd_copy = dup(STDOUT_FILENO)) == -1) > + return -1; > + if ((stderr_fd_copy = dup(STDERR_FILENO)) == -1) > + return -1; > + > + if (dup2(nullfd, STDOUT_FILENO) == -1) > + return -1; > + if (dup2(nullfd, STDERR_FILENO) == -1) > + return -1; > + > + if ((status = system(command)) == -1) > + return -1; > + > + /* restore */ > + if (dup2(stdout_fd_copy, STDOUT_FILENO) == -1) > + return -1; > + if (dup2(stderr_fd_copy, STDERR_FILENO) == -1) > + return -1; > + > + close(stdout_fd_copy); > + close(stderr_fd_copy); Same fd leaks here, all exit paths should close the opened fds. -- Petri Latvala > + > + return WEXITSTATUS(status); > +} > diff --git a/lib/igt_core.h b/lib/igt_core.h > index 51b98d8..6d4cf60 100644 > --- a/lib/igt_core.h > +++ b/lib/igt_core.h > @@ -918,4 +918,7 @@ FILE *__igt_fopen_data(const char* igt_srcdir, const char* igt_datadir, > #define igt_fopen_data(filename) \ > __igt_fopen_data(IGT_SRCDIR, IGT_DATADIR, filename) > > +int igt_exec(const char *command); > +int igt_exec_quiet(const char *command); > + > #endif /* IGT_CORE_H */ > -- > 2.7.4 >
On 09.05.2017 13:18, Petri Latvala wrote: snip 8< ----- >> + memset(buf, 0, sizeof(buf)); >> + while (read(current->read_fd, buf, sizeof(buf)) > 0) { >> + if (current->redirected) { >> + if (!unredirect_output(current)) >> + return -1; >> + } >> + igt_log(IGT_LOG_DOMAIN, current->log_level, >> + "[cmd] %s", buf); >> + memset(buf, 0, sizeof(buf)); >> + } >> + close(current->read_fd); >> + } > > > Unredirect_output calls for both pipes need to be called on all exit > paths. > > In redirect_output you set only the read fd of the pipe() pair to > O_NONBLOCK. That will make the executed command block on its writes > indefinitely if it prints more than whatsitnow, 64kB? > the In case the stream output is more than the pipe buf, the read loop above would just unblock the rest of the entries. From pipe(2) page: "Data written to the write end of the pipe is buffered by the kernel until it is read from the read end of the pipe." Worked well when I tried it with igt_exec("../tools/intel_reg dump") which dumps a screenfull of info.
On Wed, May 10, 2017 at 03:10:16PM +0300, Abdiel Janulgue wrote: > In case the stream output is more than the pipe buf, the read loop above > would just unblock the rest of the entries. From pipe(2) page: But you don't reach that read loop until system() returns. Which happens after the program has terminated. Which never happens. -- Petri Latvala
On 10.05.2017 15:14, Petri Latvala wrote: > On Wed, May 10, 2017 at 03:10:16PM +0300, Abdiel Janulgue wrote: >> In case the stream output is more than the pipe buf, the read loop above >> would just unblock the rest of the entries. From pipe(2) page: > > > But you don't reach that read loop until system() returns. Which > happens after the program has terminated. Which never happens. > Experimented with setting the write end of the pipe as O_NONBLOCK, I actually didn't notice any difference when dumping tons of stdout compared with just setting only the read end. Maybe it's because we are not using write() directly? Not sure if this is expected behavior or not... > > > -- > Petri Latvala >
diff --git a/lib/igt_core.c b/lib/igt_core.c index 403b942..8a7ba0d 100644 --- a/lib/igt_core.c +++ b/lib/igt_core.c @@ -2073,3 +2073,154 @@ 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) +{ + int fds[2], flags; + + if (pipe(fds) == -1) + return false; + + if ((flags = fcntl(fds[0], F_GETFL) == -1)) + return false; + + flags |= O_NONBLOCK; + if (fcntl(fds[0], F_SETFL, flags) == -1) + return false; + + /* save output */ + if ((p->save_fd = dup(output_fd)) == -1) + return false; + + /* Redirect output to our buffer */ + if (dup2(fds[1], output_fd) == -1) + return false; + + p->output_fd = output_fd; + p->read_fd = fds[0]; + p->write_fd = fds[1]; + p->redirected = true; + p->log_level = level; + + return true; +} + +static bool unredirect_output(struct output_pipe *p) +{ + close(p->write_fd); + if (dup2(p->save_fd, p->output_fd) == -1) + return false; + close(p->save_fd); + + p->redirected = false; + + return true; +} + +/** + * igt_exec: + * + * Executes the shell command specified in @command and capture its stdout and + * stderr to igt_log or igt_warn respectively. + * + * Returns: The exit status of the executed process. -1 for failure. + */ +int igt_exec(const char *command) +{ +#define OUT 0 +#define ERR 1 + struct output_pipe op[2]; + int i, sel_fd, status; + fd_set fds; + struct timeval timeout = { .tv_sec = 0, .tv_usec = 0 }; + char buf[PIPE_BUF]; + + if (!redirect_output(&op[OUT], STDOUT_FILENO, IGT_LOG_INFO)) + return -1; + if (!redirect_output(&op[ERR], STDERR_FILENO, IGT_LOG_WARN)) + return -1; + + if ((status = system(command)) == -1) + return -1; + + FD_ZERO(&fds); + FD_SET(op[OUT].read_fd, &fds); + FD_SET(op[ERR].read_fd, &fds); + + sel_fd = max(op[OUT].read_fd, op[ERR].read_fd); + if (select(sel_fd + 1, &fds, NULL, NULL, &timeout) == -1) + return -1; + + for (i = 0; i < ARRAY_SIZE(op); i++) { + struct output_pipe *current = &op[i]; + + if (!FD_ISSET(current->read_fd, &fds)) { + close(current->read_fd); + if (!unredirect_output(current)) + return -1; + continue; + } + + memset(buf, 0, sizeof(buf)); + while (read(current->read_fd, buf, sizeof(buf)) > 0) { + if (current->redirected) { + if (!unredirect_output(current)) + return -1; + } + igt_log(IGT_LOG_DOMAIN, current->log_level, + "[cmd] %s", buf); + memset(buf, 0, sizeof(buf)); + } + close(current->read_fd); + } + + return WEXITSTATUS(status); +} + +/** + * igt_exec_quiet: + * Similar to igt_exec(), except redirect output to /dev/null + * + * Returns: The exit status of the executed process. -1 for failure. + */ +int igt_exec_quiet(const char *command) +{ + int stderr_fd_copy, stdout_fd_copy, status, nullfd; + + /* redirect */ + if ((nullfd = open("/dev/null", O_WRONLY)) == -1) + return -1; + if ((stdout_fd_copy = dup(STDOUT_FILENO)) == -1) + return -1; + if ((stderr_fd_copy = dup(STDERR_FILENO)) == -1) + return -1; + + if (dup2(nullfd, STDOUT_FILENO) == -1) + return -1; + if (dup2(nullfd, STDERR_FILENO) == -1) + return -1; + + if ((status = system(command)) == -1) + return -1; + + /* restore */ + if (dup2(stdout_fd_copy, STDOUT_FILENO) == -1) + return -1; + if (dup2(stderr_fd_copy, STDERR_FILENO) == -1) + return -1; + + close(stdout_fd_copy); + close(stderr_fd_copy); + + return WEXITSTATUS(status); +} diff --git a/lib/igt_core.h b/lib/igt_core.h index 51b98d8..6d4cf60 100644 --- a/lib/igt_core.h +++ b/lib/igt_core.h @@ -918,4 +918,7 @@ FILE *__igt_fopen_data(const char* igt_srcdir, const char* igt_datadir, #define igt_fopen_data(filename) \ __igt_fopen_data(IGT_SRCDIR, IGT_DATADIR, filename) +int igt_exec(const char *command); +int igt_exec_quiet(const char *command); + #endif /* IGT_CORE_H */
Support executing external processes with the goal of capturing its standard streams to the igt logging infrastructure in addition to its exit status. Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Petri Latvala <petri.latvala@intel.com> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com> --- lib/igt_core.c | 151 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ lib/igt_core.h | 3 ++ 2 files changed, 154 insertions(+)