Re: [PATCH i-g-t v3 09/10] runner/settings: Serialize command line

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

Quoting Gustavo Sousa (2025-02-07 17:36:44-03:00)
>Quoting Lucas De Marchi (2025-01-30 14:21:48-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
>>
>>Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>>---
>> runner/settings.c | 21 +++++++++++++++++++++
>> runner/settings.h |  4 ++++
>> 2 files changed, 25 insertions(+)
>>
>>diff --git a/runner/settings.c b/runner/settings.c
>>index 62afd3534..84d8c85e9 100644
>>--- a/runner/settings.c
>>+++ b/runner/settings.c
>>@@ -529,6 +529,14 @@ static void free_hook_strs(struct igt_vec *hook_strs)
>>         igt_vec_fini(hook_strs);
>> }
>> 
>>+static void free_array_deep(void **arr, size_t n)
>>+{
>
>We need to make sure that arr is not NULL before going into the for-loop
>below. That would be the case if "settings->cmdline.argv = calloc(argc,
>sizeof(*settings->cmdline.argv))" failed.

Hm... Although n would be zero in that case, huh... Well, your call. I
would still check, but feel free to use the r-b without the check as
well.

--
Gustavo Sousa

>
>With that fixed,
>
>    Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>
>
>>+        for (size_t i = 0; i < n; i++)
>>+                free(arr[i]);
>>+
>>+        free(arr);
>>+}
>>+
>> static bool file_exists_at(int dirfd, const char *filename)
>> {
>>         return faccessat(dirfd, filename, F_OK, 0) == 0;
>>@@ -646,6 +654,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_array_deep((void **)settings->cmdline.argv, settings->cmdline.argc);
>> 
>>         init_settings(settings);
>> }
>>@@ -875,6 +884,16 @@ bool parse_options(int argc, char **argv,
>>                 goto error;
>>         }
>> 
>>+        settings->cmdline.argv = calloc(argc, sizeof(*settings->cmdline.argv));
>>+        if (!settings->cmdline.argv)
>>+                goto error;
>>+
>>+        settings->cmdline.argc = argc;
>>+        for (int i = 0; i < argc; i++) {
>>+                settings->cmdline.argv[i] = strdup(argv[i]);
>>+                if (!settings->cmdline.argv[i])
>>+                        goto error;
>>+        }
>> 
>>         return true;
>> 
>>@@ -1202,6 +1221,7 @@ bool serialize_settings(struct settings *settings)
>>         SERIALIZE_INT(f, settings, enable_code_coverage);
>>         SERIALIZE_INT(f, settings, cov_results_per_test);
>>         SERIALIZE_STR(f, settings, code_coverage_script);
>>+        SERIALIZE_STR_ARRAY(f, settings, cmdline.argv, cmdline.argc);
>> 
>>         if (settings->sync) {
>>                 fflush(f);
>>@@ -1315,6 +1335,7 @@ 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_STR_ARRAY(settings, name, val, cmdline.argv, cmdline.argc);
>> 
>>                 printf("Warning: Unknown field in settings file: %s = %s\n",
>>                        name, val);
>>diff --git a/runner/settings.h b/runner/settings.h
>>index f69f09778..2266118a7 100644
>>--- a/runner/settings.h
>>+++ b/runner/settings.h
>>@@ -75,6 +75,10 @@ struct settings {
>>         char *code_coverage_script;
>>         bool enable_code_coverage;
>>         bool cov_results_per_test;
>>+        struct {
>>+                int argc;
>>+                char **argv;
>>+        } cmdline;
>> };
>> 
>> /**
>>-- 
>>2.48.0
>>