[Intel-gfx] [i-g-t PATCH 1/4] lib/igt_core: Add igt_exec helpers

[Petri Latvala]

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 at ffwll.ch>
> Cc: Petri Latvala <petri.latvala at intel.com>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue at 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
>