Re: [PATCH i-g-t v2 6/7] runner/settings: Serialize command line

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

Quoting Lucas De Marchi (2025-01-21 19:57:32-03:00)
>Serialize the command line to metadata.txt. The expected format in the
>metadata.txt is like below:
>
>        cmdline.argc : 6
>        cmdline.argv[0] : ./build/runner/igt_runner
>        cmdline.argv[1] : -o
>        cmdline.argv[2] : --test-list
>        cmdline.argv[3] : /tmp/testlist.txt
>        cmdline.argv[4] : build/tests/
>        cmdline.argv[5] : /tmp/results

One limitation here is that we would run into problems if one of the
arguments contains a newline character. We need to have a proper way of
handling that.

One option would be to have string serialization to escape newlines
characters, and string parsing to undo the escaping.

Another one would be to generalize read_hook_strs_from_file() (and with
a proper rename) to be reusable for both hook strings and argv.

>
>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>---
> runner/settings.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> runner/settings.h |  5 +++++
> 2 files changed, 47 insertions(+)
>
>diff --git a/runner/settings.c b/runner/settings.c
>index 2787869ee..ed1afc205 100644
>--- a/runner/settings.c
>+++ b/runner/settings.c
>@@ -529,6 +529,18 @@ static void free_hook_strs(struct igt_vec *hook_strs)
>         igt_vec_fini(hook_strs);
> }
> 
>+static void free_cmdline(struct settings *settings)
>+{
>+        if (!settings->cmdline.allocated)
>+                return;

I would just make life simpler and have cmdline being always allocated.
But I'm not blocking on this...

>+
>+        for (size_t i = 0; i < settings->cmdline.argc; i++)
>+                free(settings->cmdline.argv[i]);
>+
>+        free(settings->cmdline.argv);
>+}
>+
>+
> static bool file_exists_at(int dirfd, const char *filename)
> {
>         return faccessat(dirfd, filename, F_OK, 0) == 0;
>@@ -646,6 +658,7 @@ void clear_settings(struct settings *settings)
>         free_regexes(&settings->exclude_regexes);
>         free_env_vars(&settings->env_vars);
>         free_hook_strs(&settings->hook_strs);
>+        free_cmdline(settings);
> 
>         init_settings(settings);
> }
>@@ -875,6 +888,8 @@ bool parse_options(int argc, char **argv,
>                 goto error;
>         }
> 
>+        settings->cmdline.argc = argc;
>+        settings->cmdline.argv = argv;
> 
>         return true;
> 
>@@ -1055,6 +1070,7 @@ static bool serialize_hook_strs(struct settings *settings, int dirfd)
> bool serialize_settings(struct settings *settings)
> {
> #define SERIALIZE_LINE(f, s, name, format) fprintf(f, "%s : " format "\n", #name, s->name)
>+#define SERIALIZE_ARRAY_ITEM(f, s, name, _i, format) fprintf(f, "%s[%d] : " format "\n", #name, _i, s->name[_i])
> 
>         FILE *f;
>         int dirfd, covfd;
>@@ -1123,6 +1139,10 @@ bool serialize_settings(struct settings *settings)
>         SERIALIZE_LINE(f, settings, cov_results_per_test, "%d");
>         SERIALIZE_LINE(f, settings, code_coverage_script, "%s");
> 
>+        SERIALIZE_LINE(f, settings, cmdline.argc, "%d");
>+        for (int i = 0; i < settings->cmdline.argc; i++)
>+                SERIALIZE_ARRAY_ITEM(f, settings, cmdline.argv, i, "%s");
>+
>         if (settings->sync) {
>                 fflush(f);
>                 fsync(fileno(f));
>@@ -1177,9 +1197,21 @@ static char *parse_str(char **pval)
>                 s->field = _f(&val);                \
>                 goto cleanup;                        \
>         }
>+#define PARSE_LINE_ARRAY(s, name, val, field, _f, _max)                \
>+        do {                                                        \
>+                int idx;                                        \
>+                if (sscanf(name, #field "[%u]", &idx) == 1 &&        \
>+                    idx < s->_max) {                                \
>+                        s->field[idx] = _f(&val);                \
>+                        goto cleanup;                                \
>+                }                                                \
>+        } while (0)
>+
> #define PARSE_INT(s, name, val, field) PARSE_LINE(s, name, val, field, parse_int)
> #define PARSE_UL(s, name, val, field)  PARSE_LINE(s, name, val, field, parse_ul)
> #define PARSE_STR(s, name, val, field) PARSE_LINE(s, name, val, field, parse_str)
>+#define PARSE_ARRAY_STR(s, name, val, field, _max) \
>+        PARSE_LINE_ARRAY(s, name, val, field, parse_str, _max)
> 
> bool read_settings_from_file(struct settings *settings, FILE *f)
> {
>@@ -1211,6 +1243,15 @@ bool read_settings_from_file(struct settings *settings, FILE *f)
>                 PARSE_INT(settings, name, val, enable_code_coverage);
>                 PARSE_INT(settings, name, val, cov_results_per_test);
>                 PARSE_STR(settings, name, val, code_coverage_script);
>+                PARSE_INT(settings, name, val, cmdline.argc);
>+
>+                if (settings->cmdline.argc && !settings->cmdline.argv) {
>+                        settings->cmdline.allocated = true;
>+                        settings->cmdline.argv = calloc(settings->cmdline.argc,
>+                                                        sizeof(*settings->cmdline.argv));
>+                }

I think we could have a PARSE_ARRAY_LENGTH() that would encapsulate
reading the int and allocating the array. Of course, that would require
having a "known format" for an array structure in settings to be used by
others in the future.

The advantage of such a macro, is that we don't need to keep checking if
we already have the length and that we haven't done the allocation yet,
like done above.

--
Gustavo Sousa

>+
>+                PARSE_ARRAY_STR(settings, name, val, cmdline.argv, cmdline.argc);
> 
>                 printf("Warning: Unknown field in settings file: %s = %s\n",
>                        name, val);
>@@ -1234,6 +1275,7 @@ cleanup:
>         return true;
> }
> #undef PARSE_LINE
>+#undef PARSE_LINE_ARRAY
> 
> /**
>  * read_env_vars_from_file() - load env vars from a file
>diff --git a/runner/settings.h b/runner/settings.h
>index f69f09778..d563a0d16 100644
>--- a/runner/settings.h
>+++ b/runner/settings.h
>@@ -75,6 +75,11 @@ struct settings {
>         char *code_coverage_script;
>         bool enable_code_coverage;
>         bool cov_results_per_test;
>+        struct {
>+                bool allocated;
>+                int argc;
>+                char **argv;
>+        } cmdline;
> };
> 
> /**
>-- 
>2.48.0
>