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@xxxxxxxx>
> Cc: Petri Latvala <petri.latvala@xxxxxxxxx>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@xxxxxxxxxxxxxxx>
> ---
> 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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
