[PATCH i-g-t v2 6/7] runner/settings: Serialize command line
Gustavo Sousa
gustavo.sousa at intel.com
Wed Jan 22 18:26:34 UTC 2025
Quoting Peter Senna Tschudin (2025-01-22 15:16:59-03:00)
>
>
>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?
If you are using bash, I'm afraid '\n' in '(?m)\b\w*exec\w*\b\n' is not
turned into a real newline character. Something like $'foo\nbar'
would.
Example:
$ echo 'foo\nbar'
foo\nbar
$ echo $'foo\nbar'
foo
bar
--
Gustavo Sousa
>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