Re: [PATCH i-g-t v1 2/2] runner/executor: Report command used for test exec

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

Quoting Kamil Konieczny (2025-07-16 12:31:02-03:00)
>Add printing of command line used for executing a test, when
>the log level is normal or above.
>
>Signed-off-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>---
> runner/executor.c | 41 ++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 36 insertions(+), 5 deletions(-)
>
>diff --git a/runner/executor.c b/runner/executor.c
>index 7565f2571..6ee446cdb 100644
>--- a/runner/executor.c
>+++ b/runner/executor.c
>@@ -1623,17 +1623,20 @@ static void free_test_command(struct igt_vec *arg_vec)
>         igt_vec_fini(arg_vec);
> }
> 
>-static void build_test_command(struct settings *settings,
>-                               struct job_list_entry *entry,
>-                               struct igt_vec *arg_vec)
>+static char *build_test_command(struct settings *settings,
>+                                struct job_list_entry *entry,
>+                                struct igt_vec *arg_vec)
> {
>+        char *cmd_line;
>+        size_t cmd_size;
>         size_t rootlen;
>         char *arg;
> 
>         igt_vec_init(arg_vec, sizeof(char *));
> 
>         rootlen = strlen(settings->test_root);
>-        arg = malloc(rootlen + strlen(entry->binary) + 2);
>+        cmd_size = rootlen + strlen(entry->binary) + 2;

I think adding to cmd_size for each time we add an element to arg_vec is
a bit error prone. Maybe a more robust solution would be for cmd_size to
be calculated at the end, by iterating over elements of arg_vec.

That said, see further below that I sugest that we build the log string
in a separate function.

>+        arg = malloc(cmd_size);
>         strcpy(arg, settings->test_root);
>         arg[rootlen] = '/';
>         strcpy(arg + rootlen + 1, entry->binary);
>@@ -1645,6 +1648,7 @@ static void build_test_command(struct settings *settings,
>                 size_t i;
> 
>                 arg = strdup("--run-subtest");
>+                cmd_size += strlen(arg) + 1;
>                 igt_vec_push(arg_vec, &arg);
> 
>                 if ((dynbegin = strchr(entry->subtests[0], '@')) != NULL)
>@@ -1668,25 +1672,47 @@ static void build_test_command(struct settings *settings,
>                         argsize += sublen + 1;
>                 }
> 
>+                cmd_size += strlen(arg) + 1;
>                 igt_vec_push(arg_vec, &arg);
> 
>                 if (dynbegin) {
>                         arg = strdup("--dynamic-subtest");
>+                        cmd_size += strlen(arg) + 1;
>                         igt_vec_push(arg_vec, &arg);
>+
>                         arg = strdup(dynbegin + 1);
>+                        cmd_size += strlen(arg) + 1;
>                         igt_vec_push(arg_vec, &arg);
>                 }
>         }
> 
>         for (size_t i = 0; i < igt_vec_length(&settings->hook_strs); i++) {
>                 arg = strdup("--hook");
>+                cmd_size += strlen(arg) + 1;
>                 igt_vec_push(arg_vec, &arg);
>                 arg = strdup(*((char **)igt_vec_elem(&settings->hook_strs, i)));
>+                cmd_size += strlen(arg) + 1;
>                 igt_vec_push(arg_vec, &arg);
>         }
> 
>+        cmd_size *= 2;
>+        cmd_line = malloc(cmd_size);
>+        if (cmd_line) {
>+                char *elem;
>+                int len;
>+
>+                elem = *((char **)igt_vec_elem(arg_vec, 0));
>+                len = snprintf(cmd_line, cmd_size, "%s", elem);
>+                for (size_t i = 1; i < igt_vec_length(arg_vec) && len < cmd_size; i++) {
>+                        elem = *((char **)igt_vec_elem(arg_vec, i));
>+                        len += snprintf(cmd_line + len, cmd_size - len, " %s", elem);

I think this would give a broken cmdline if there are spaces in
arguments (e.g. hook strings via --hook or environment variables via
-e).

Since we are just using the generated string for logging, that's not
very problematic on our side, but it could be inconvenient for anyone
that wants to copy/paste the string into the shell.

>+                }
>+        }
>+
>         arg = NULL;
>         igt_vec_push(arg_vec, &arg);
>+
>+        return cmd_line;
> }
> 
> static void __attribute__((noreturn))
>@@ -1788,6 +1814,7 @@ static int execute_next_entry(struct execute_state *state,
>                               bool *abort_already_written)
> {
>         struct igt_vec arg_vec;
>+        char *cmd_line;
>         int dirfd;
>         int outputs[_F_LAST];
>         int kmsgfd;
>@@ -1837,7 +1864,7 @@ static int execute_next_entry(struct execute_state *state,
>                 lseek(kmsgfd, 0, SEEK_END);
>         }
> 
>-        build_test_command(settings, entry, &arg_vec);
>+        cmd_line = build_test_command(settings, entry, &arg_vec);
> 
>         if (settings->log_level >= LOG_LEVEL_NORMAL) {
>                 char buf[100];
>@@ -1856,6 +1883,8 @@ static int execute_next_entry(struct execute_state *state,
>                 free(displayname);
> 
>                 outf("%s\n", buf);
>+                if (cmd_line)
>+                        outf("Exec test: %s\n", cmd_line);

I think a cleaner solution would be to have a specific function that
takse arg_vec and builds the string to be output. Then we would call
that function here. The benefits IMO are:

* We do not overload build_test_command() to do an extra thing (i.e.
  build the log string).

* The log string is built only if necessary.

--
Gustavo Sousa

>         }
> 
>         /*
>@@ -1908,6 +1937,8 @@ static int execute_next_entry(struct execute_state *state,
> out_kmsgfd:
>         close(kmsgfd);
>         free_test_command(&arg_vec);
>+        if (cmd_line)
>+                free(cmd_line);
> out_pipe:
>         close(outpipe[0]);
>         close(outpipe[1]);
>-- 
>2.50.1
>