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

[Lucas De Marchi]

On Wed, Jan 22, 2025 at 01:21:43PM +0100, Peter Senna Tschudin wrote:
>Hi Lucas,
>
>On 21.01.2025 23:57, Lucas De Marchi wrote:
>> Simplify assigning the variables by using wrapper functions. This avoids
>macros, not functions(please also update subject). When I read v2 and saw
>functions here, I went oh cool, but they are still macros. :-)

the wrappers are actually functions: parse_int, parse_ul, parse_str.
The additional macros is just to avoid the repetition of doing this
everywhere:

	PARSE_LINE(s, name, val, field, parse_int)

I'm not a fan of these macros, but it would be more verbose without.
Probably something like this:

	if (!strcmp(name, "field_name")
		settings->field_name = parse_int(val);
	else if (!strcmp ...

in other places I´ve coded this pattern by using a table and offsetof().
It's another possibility but will make the "allocate after parsing a
field" in the patches after this harder.

I didn't feel much the need to polish this though as it's barely
touched. Maybe the if/else chain wouldn't be so horrible.

>
>
>> 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;
>> +}
>> +
>> +#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)
>>
>> +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
>
>Please add undefs for PARSE_INT, PARSE_UL, and PARSE_STR.
>
>>
>>  /**
>>   * read_env_vars_from_file() - load env vars from a fileTested-by: Peter Senna Tschudin <peter.senna at linux.intel.com>
>Reviewed-by: Peter Senna Tschudin <peter.senna at linux.intel.com>
>