Re: [PATCH i-g-t v2 2/7] runner/settings: Use wrapper functions for each type

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

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@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?

>+
>+#define PARSE_LINE(s, name, val, field, _f)        \
>         if (!strcmp(name, #field)) {                \
>-                s->field = write;                \
>+                s->field = _f(&val);                \
>                 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.)

> 
>+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
>