[PATCH i-g-t v2 6/7] runner/settings: Serialize command line
Peter Senna Tschudin
peter.senna at linux.intel.com
Wed Jan 22 18:16:59 UTC 2025
On 22.01.2025 13:40, Gustavo Sousa wrote:
> 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.
I tested this by adding -t '(?m)\b\w*exec\w*\b\n', is that what you mean?
Here is the full json:
"cmdline":[
".\/build\/runner\/igt_runner",
"-o",
"-f",
"--per-test-timeout",
"100",
"--dry-run",
"--test-list",
"build\/docs\/testplan\/intel-ci-tests\/xe\/default\/bat.testlist",
"-t",
"(?m)\\b\\w*exec\\w*\\b\\n",
"\/home\/gta\/igt\/3"
],
The newline Looks good to me...
I also tried adding a multi line regex, and that fails. However it fails without
this change as well so Lucas did not introduce any regression. So maybe it is ok
to accept this patch as is. Here is what I tried with and without Lucas' patches:
$ time sudo IGT_TEST_ROOT='/home/gta/UPSTREAM/igt-gpu-tools/build/tests/' ./build/runner/igt_runner -o -f --per-test-timeout 100 --dry-run --test-list 'build/docs/testplan/intel-ci-tests/xe/default/bat.testlist' -t '(?s) \
\b \
\w*exec\w*\b' /home/gta/igt/4
If my example is wrong, please help me with a good one.
>
> 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 at 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
>>
More information about the igt-dev
mailing list