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-23 03:07:09-03:00)
>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@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).

I guess that's a different case, though? Those macros were used
everywhere in the source code, and having an implicit dependenty on the
dev_priv variable is not good in that case.

These ones are specific for read_settings_from_file(), as far as I
understand.

In any case, I just think it would make the code less verbose, but not a
big deal if we end up not doing that.

--
Gustavo Sousa

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