diff mbox

[i-g-t,v2,1/4] lib/igt_core: Add igt_exec helpers

Message ID 1496059690-3308-1-git-send-email-abdiel.janulgue@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Abdiel Janulgue May 29, 2017, 12:08 p.m. UTC
Support executing external processes with the goal of capturing its
standard streams to the igt logging infrastructure in addition to its
exit status.

v2: Fix leaks on fd teardown. Make sure redirected process printout when
    > 64kb still works, like full dmesg. (Petri)

Cc: Petri Latvala <petri.latvala@intel.com>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
---
 lib/igt_core.c | 154 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_core.h |  10 ++++
 2 files changed, 164 insertions(+)

Comments

Chris Wilson May 29, 2017, 1:01 p.m. UTC | #1
On Mon, May 29, 2017 at 03:08:07PM +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.

This is not an exec wrapper. This is a system() replacement. That it
invokes shell to evaluate the cmd should be front and centre.
-Chris
Abdiel Janulgue May 30, 2017, 10:31 a.m. UTC | #2
On 29.05.2017 16:01, Chris Wilson wrote:
> On Mon, May 29, 2017 at 03:08:07PM +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.
> 
> This is not an exec wrapper. This is a system() replacement. That it
> invokes shell to evaluate the cmd should be front and centre.
> -Chris
> 

The problem with system() is that it waits until the process is
completed. If the process' output is more than PIPE_BUF, it'll block. Do
you have any suggestions?
Chris Wilson May 30, 2017, 10:55 a.m. UTC | #3
On Tue, May 30, 2017 at 01:31:46PM +0300, Abdiel Janulgue wrote:
> 
> 
> On 29.05.2017 16:01, Chris Wilson wrote:
> > On Mon, May 29, 2017 at 03:08:07PM +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.
> > 
> > This is not an exec wrapper. This is a system() replacement. That it
> > invokes shell to evaluate the cmd should be front and centre.
> > 
> 
> The problem with system() is that it waits until the process is
> completed. If the process' output is more than PIPE_BUF, it'll block. Do
> you have any suggestions?

I'm just saying the name is misleading. igt_system() and explain the
improvements (concurrency and accessible piped output).
-Chris
diff mbox

Patch

diff --git a/lib/igt_core.c b/lib/igt_core.c
index 9c3b37f..c2c8872 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -2099,3 +2099,157 @@  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];
+
+	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);
+
+	return false;
+}
+
+static void unredirect_output(struct output_pipe *p)
+{
+	if (!p->redirected)
+		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_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, status;
+	struct igt_helper_process process = {};
+	char buf[PIPE_BUF];
+
+	if (!redirect_output(&op[OUT], STDOUT_FILENO, IGT_LOG_INFO))
+		goto err;
+	if (!redirect_output(&op[ERR], STDERR_FILENO, IGT_LOG_WARN))
+		goto err;
+
+	igt_fork_helper(&process) {
+		igt_assert(execl("/bin/sh", "sh", "-c", command,
+				 (char *) NULL) != -1);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(op); i++) {
+		struct output_pipe *current = &op[i];
+
+		/* 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);
+	}
+	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]);
+	}
+
+	return -1;
+}
+
+/**
+ * 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)
+		goto err;
+	if ((stdout_fd_copy = dup(STDOUT_FILENO)) == -1)
+		goto err;
+	if ((stderr_fd_copy = dup(STDERR_FILENO)) == -1)
+		goto err;
+
+	if (dup2(nullfd, STDOUT_FILENO) == -1)
+		goto err;
+	if (dup2(nullfd, STDERR_FILENO) == -1)
+		goto err;
+
+	if ((status = system(command)) == -1)
+		goto err;
+
+	/* restore */
+	if (dup2(stdout_fd_copy, STDOUT_FILENO) == -1)
+		goto err;
+	if (dup2(stderr_fd_copy, STDERR_FILENO) == -1)
+		goto err;
+
+	close(stdout_fd_copy);
+	close(stderr_fd_copy);
+	close(nullfd);
+
+	return WEXITSTATUS(status);
+err:
+	close(stderr_fd_copy);
+	close(stdout_fd_copy);
+	close(nullfd);
+
+	return -1;
+}
diff --git a/lib/igt_core.h b/lib/igt_core.h
index 4a125af..c41aa1a 100644
--- a/lib/igt_core.h
+++ b/lib/igt_core.h
@@ -919,4 +919,14 @@  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);
+#define igt_exec_cmd(status, format...) \
+	do { \
+		char *buf = 0; \
+		igt_assert(asprintf(&buf, format) != -1); \
+	        status = igt_exec(buf); \
+		free(buf); \
+	} while (0)
+
 #endif /* IGT_CORE_H */