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 <abdiel.janulgue@xxxxxxxxxxxxxxx>
> Signed-off-by: Petri Latvala <petri.latvala@xxxxxxxxx>
Thanks for fixing this. Series is
Reviewed-by: Abdiel Janulgue <abdiel.janulgue@xxxxxxxxxxxxxxx>
> ---
> 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
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
