Re: [PATCH i-g-t v1 1/2] runner/executor: Build test command line before fork

[Gustavo Sousa <gustavo.sousa@intel.com>]

Quoting Kamil Konieczny (2025-07-16 12:31:01-03:00)
>Create a separate test command line building in main runner
>process before forking, so it could be later processed for other
>purposes like reporting.
>
>Signed-off-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>---
> runner/executor.c | 64 ++++++++++++++++++++++++++++++-----------------
> 1 file changed, 41 insertions(+), 23 deletions(-)
>
>diff --git a/runner/executor.c b/runner/executor.c
>index 847abe481..7565f2571 100644
>--- a/runner/executor.c
>+++ b/runner/executor.c
>@@ -1615,28 +1615,29 @@ static int monitor_output(pid_t child,
>         return killed;
> }
> 
>-static void __attribute__((noreturn))
>-execute_test_process(int outfd, int errfd, int socketfd,
>-                     struct settings *settings,
>-                     struct job_list_entry *entry)
>+static void free_test_command(struct igt_vec *arg_vec)
> {
>-        struct igt_vec arg_vec;
>-        char *arg;
>-        size_t rootlen;
>+        for (size_t i = 0; i < igt_vec_length(arg_vec); i++)
>+                free(*((char **)igt_vec_elem(arg_vec, i)));
> 
>-        dup2(outfd, STDOUT_FILENO);
>-        dup2(errfd, STDERR_FILENO);
>+        igt_vec_fini(arg_vec);
>+}
> 
>-        setpgid(0, 0);
>+static void build_test_command(struct settings *settings,
>+                               struct job_list_entry *entry,
>+                               struct igt_vec *arg_vec)
>+{
>+        size_t rootlen;
>+        char *arg;
> 
>-        igt_vec_init(&arg_vec, sizeof(char *));
>+        igt_vec_init(arg_vec, sizeof(char *));
> 
>         rootlen = strlen(settings->test_root);
>         arg = malloc(rootlen + strlen(entry->binary) + 2);
>         strcpy(arg, settings->test_root);
>         arg[rootlen] = '/';
>         strcpy(arg + rootlen + 1, entry->binary);
>-        igt_vec_push(&arg_vec, &arg);
>+        igt_vec_push(arg_vec, &arg);
> 
>         if (entry->subtest_count) {
>                 size_t argsize;
>@@ -1644,7 +1645,7 @@ execute_test_process(int outfd, int errfd, int socketfd,
>                 size_t i;
> 
>                 arg = strdup("--run-subtest");
>-                igt_vec_push(&arg_vec, &arg);
>+                igt_vec_push(arg_vec, &arg);
> 
>                 if ((dynbegin = strchr(entry->subtests[0], '@')) != NULL)
>                         argsize = dynbegin - entry->subtests[0];
>@@ -1667,35 +1668,49 @@ execute_test_process(int outfd, int errfd, int socketfd,
>                         argsize += sublen + 1;
>                 }
> 
>-                igt_vec_push(&arg_vec, &arg);
>+                igt_vec_push(arg_vec, &arg);
> 
>                 if (dynbegin) {
>                         arg = strdup("--dynamic-subtest");
>-                        igt_vec_push(&arg_vec, &arg);
>+                        igt_vec_push(arg_vec, &arg);
>                         arg = strdup(dynbegin + 1);
>-                        igt_vec_push(&arg_vec, &arg);
>+                        igt_vec_push(arg_vec, &arg);
>                 }
>         }
> 
>         for (size_t i = 0; i < igt_vec_length(&settings->hook_strs); i++) {
>                 arg = strdup("--hook");
>-                igt_vec_push(&arg_vec, &arg);
>+                igt_vec_push(arg_vec, &arg);
>                 arg = strdup(*((char **)igt_vec_elem(&settings->hook_strs, i)));
>-                igt_vec_push(&arg_vec, &arg);
>+                igt_vec_push(arg_vec, &arg);
>         }
> 
>         arg = NULL;
>-        igt_vec_push(&arg_vec, &arg);
>+        igt_vec_push(arg_vec, &arg);
>+}
>+
>+static void __attribute__((noreturn))
>+execute_test_process(int outfd, int errfd, int socketfd,
>+                     struct settings *settings,
>+                     struct job_list_entry *entry,
>+                     struct igt_vec *arg_vec)
>+{
>+        char *arg;
>+
>+        dup2(outfd, STDOUT_FILENO);
>+        dup2(errfd, STDERR_FILENO);
>+
>+        setpgid(0, 0);
> 
>         if (socketfd >= 0) {
>                 struct runnerpacket *packet;
> 
>-                packet = runnerpacket_exec(arg_vec.elems);
>+                packet = runnerpacket_exec(arg_vec->elems);
>                 write(socketfd, packet, packet->size);
>         }
> 
>-        arg = *((char **)igt_vec_elem(&arg_vec, 0));
>-        execv(arg, arg_vec.elems);
>+        arg = *((char **)igt_vec_elem(arg_vec, 0));
>+        execv(arg, arg_vec->elems);
>         fprintf(stderr, "Cannot execute %s\n", arg);
>         exit(IGT_EXIT_INVALID);
> }
>@@ -1772,6 +1787,7 @@ static int execute_next_entry(struct execute_state *state,
>                               char **abortreason,
>                               bool *abort_already_written)
> {
>+        struct igt_vec arg_vec;
>         int dirfd;
>         int outputs[_F_LAST];
>         int kmsgfd;
>@@ -1821,6 +1837,7 @@ static int execute_next_entry(struct execute_state *state,
>                 lseek(kmsgfd, 0, SEEK_END);
>         }
> 
>+        build_test_command(settings, entry, &arg_vec);
> 
>         if (settings->log_level >= LOG_LEVEL_NORMAL) {
>                 char buf[100];
>@@ -1871,7 +1888,7 @@ static int execute_next_entry(struct execute_state *state,
>                 }
>                 setenv("IGT_SENTINEL_ON_STDERR", "1", 1);
> 
>-                execute_test_process(outfd, errfd, socketfd, settings, entry);
>+                execute_test_process(outfd, errfd, socketfd, settings, entry, &arg_vec);
>                 /* unreachable */
>         }
> 
>@@ -1890,6 +1907,7 @@ static int execute_next_entry(struct execute_state *state,
> 
> out_kmsgfd:
>         close(kmsgfd);
>+        free_test_command(&arg_vec);

Since arg_vec is built after kmsgfd is opened, I think it would make
sense to call free_test_command(&arg_vec) before close(kmsgfd), to keep
it consistent with the "stacked" approach and avoid any gotchas in the
future regarding ordering of operations.

In this case, a rename of the label out_kmsgfd would also be warranted.

--
Gustavo Sousa

> out_pipe:
>         close(outpipe[0]);
>         close(outpipe[1]);
>-- 
>2.50.1
>