[PATCH i-g-t v2 2/7] runner/settings: Use wrapper functions for each type
Lucas De Marchi
lucas.demarchi at intel.com
Thu Jan 23 06:07:09 UTC 2025
On Wed, Jan 22, 2025 at 08:20:24AM -0300, Gustavo Sousa wrote:
>Quoting Lucas De Marchi (2025-01-21 19:57:28-03:00)
>>Simplify assigning the variables by using wrapper functions. This avoids
>>calling atoi() on every iteration and will allow to simplify the
>>strdup() calls in future.
>>
>>Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>>---
>> runner/settings.c | 74 ++++++++++++++++++++++++++++-------------------
>> 1 file changed, 45 insertions(+), 29 deletions(-)
>>
>>diff --git a/runner/settings.c b/runner/settings.c
>>index 80d95be5b..13694a51c 100644
>>--- a/runner/settings.c
>>+++ b/runner/settings.c
>>@@ -1152,43 +1152,60 @@ bool serialize_settings(struct settings *settings)
>> #undef SERIALIZE_LINE
>> }
>>
>>-bool read_settings_from_file(struct settings *settings, FILE *f)
>>+static int parse_int(char **pval)
>>+{
>>+ return atoi(*pval);
>>+}
>>+
>>+static unsigned long parse_ul(char **pval)
>>+{
>>+ return strtoul(*pval, NULL, 10);
>>+}
>>+
>>+static char *parse_str(char **pval)
>> {
>>-#define PARSE_LINE(s, name, val, field, write) \
>>+ return *pval ? strdup(*pval) : NULL;
>>+}
>
>Why do we need char **pval for those functions instead of simply char
>*val?
you will see on next patch, but :
>
>>+
>>+#define PARSE_LINE(s, name, val, field, _f) \
>> if (!strcmp(name, #field)) { \
>>- s->field = write; \
>>+ s->field = _f(&val); \
right now it does this ^ and in the cleanup it does the free and assign
the variable back to NULL. In the end we are doing
foo = alloc()
bar == strdup(foo)
free(foo)
when we actually only need the first allocation and pass the ownership
to the outside. When coding this I thought about calling the function
leak_str() instead of parse_str(), which would probably clarify (but
should be done only in the subsequent patch).
>> goto cleanup; \
>> }
>>+#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)
>
>I would have kept these inside read_settings_from_file(), since it is
>very specific for that function.
>
>(And in a follow-up patch even remove the s, name and val parameters to
>make it simpler.)
Matter of preference, but the defines inside the function pollutes a lot
the variable declaration. Particularly when we increase the number of
macros. No reason not to keep them just above the
function and undefine them just below the function. You will see in the
next patches I didn't bother undefining them all since just undefining
a few gives enough protection that those macros can't be used anymore.
As for the arguments, I also don't really like macros that use variables
not passed as argument. This leads to unmaintainable code - we used to
have macros in i915 that depended on having a "dev_priv" variable
somewhere - it took years to cleanup that to rename to i915 and allow
the driver to run with more than 1 devices (we made a mess with some
files, declaring a file-scoped variables).
Lucas De Marchi
>
>>
>>+bool read_settings_from_file(struct settings *settings, FILE *f)
>>+{
>> char *name = NULL, *val = NULL;
>>
>> settings->dmesg_warn_level = -1;
>>
>> while (fscanf(f, "%ms : %m[^\n]", &name, &val) == 2) {
>>- int numval = atoi(val);
>>- PARSE_LINE(settings, name, val, abort_mask, numval);
>>- PARSE_LINE(settings, name, val, disk_usage_limit, strtoul(val, NULL, 10));
>>- PARSE_LINE(settings, name, val, test_list, val ? strdup(val) : NULL);
>>- PARSE_LINE(settings, name, val, name, val ? strdup(val) : NULL);
>>- PARSE_LINE(settings, name, val, dry_run, numval);
>>- PARSE_LINE(settings, name, val, allow_non_root, numval);
>>- PARSE_LINE(settings, name, val, facts, numval);
>>- PARSE_LINE(settings, name, val, sync, numval);
>>- PARSE_LINE(settings, name, val, log_level, numval);
>>- PARSE_LINE(settings, name, val, overwrite, numval);
>>- PARSE_LINE(settings, name, val, multiple_mode, numval);
>>- PARSE_LINE(settings, name, val, inactivity_timeout, numval);
>>- PARSE_LINE(settings, name, val, per_test_timeout, numval);
>>- PARSE_LINE(settings, name, val, overall_timeout, numval);
>>- PARSE_LINE(settings, name, val, use_watchdog, numval);
>>- PARSE_LINE(settings, name, val, piglit_style_dmesg, numval);
>>- PARSE_LINE(settings, name, val, dmesg_warn_level, numval);
>>- PARSE_LINE(settings, name, val, prune_mode, numval);
>>- PARSE_LINE(settings, name, val, test_root, val ? strdup(val) : NULL);
>>- PARSE_LINE(settings, name, val, results_path, val ? strdup(val) : NULL);
>>- PARSE_LINE(settings, name, val, enable_code_coverage, numval);
>>- PARSE_LINE(settings, name, val, cov_results_per_test, numval);
>>- PARSE_LINE(settings, name, val, code_coverage_script, val ? strdup(val) : NULL);
>>+ PARSE_INT(settings, name, val, abort_mask);
>>+ PARSE_UL(settings, name, val, disk_usage_limit);
>>+ PARSE_STR(settings, name, val, test_list);
>>+ PARSE_STR(settings, name, val, name);
>>+ PARSE_INT(settings, name, val, dry_run);
>>+ PARSE_INT(settings, name, val, allow_non_root);
>>+ PARSE_INT(settings, name, val, facts);
>>+ PARSE_INT(settings, name, val, sync);
>>+ PARSE_INT(settings, name, val, log_level);
>>+ PARSE_INT(settings, name, val, overwrite);
>>+ PARSE_INT(settings, name, val, multiple_mode);
>>+ PARSE_INT(settings, name, val, inactivity_timeout);
>>+ PARSE_INT(settings, name, val, per_test_timeout);
>>+ PARSE_INT(settings, name, val, overall_timeout);
>>+ PARSE_INT(settings, name, val, use_watchdog);
>>+ PARSE_INT(settings, name, val, piglit_style_dmesg);
>>+ PARSE_INT(settings, name, val, dmesg_warn_level);
>>+ PARSE_INT(settings, name, val, prune_mode);
>>+ PARSE_STR(settings, name, val, test_root);
>>+ PARSE_STR(settings, name, val, results_path);
>>+ 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);
>>
>> printf("Warning: Unknown field in settings file: %s = %s\n",
>> name, val);
>>@@ -1210,9 +1227,8 @@ cleanup:
>> free(val);
>>
>> return true;
>>-
>>-#undef PARSE_LINE
>> }
>>+#undef PARSE_LINE
>
>Missing undef for the other macros here?
>
>--
>Gustavo Sousa
>
>>
>> /**
>> * read_env_vars_from_file() - load env vars from a file
>>--
>>2.48.0
>>
More information about the igt-dev
mailing list