Re: [PATCH i-g-t 2/2] runner/executor: Integrate igt_kmemleak scans

[Peter Senna Tschudin <peter.senna@linux.intel.com>]

On 22.01.2025 19:18, Cavitt, Jonathan wrote:

[...]

>> diff --git a/runner/runner_tests.c b/runner/runner_tests.c
>> index 8441763f2..a072a92c7 100644
>> --- a/runner/runner_tests.c
>> +++ b/runner/runner_tests.c
>> @@ -191,6 +191,7 @@ static void assert_settings_equal(struct settings *one, struct settings *two)
>>  	igt_assert_eq(one->dry_run, two->dry_run);
>>  	igt_assert_eq(one->allow_non_root, two->allow_non_root);
>>  	igt_assert_eq(one->facts, two->facts);
>> +	igt_assert_eq(one->kmemleak, two->kmemleak);
>>  	igt_assert_eq(one->sync, two->sync);
>>  	igt_assert_eq(one->log_level, two->log_level);
>>  	igt_assert_eq(one->overwrite, two->overwrite);
>> @@ -304,6 +305,7 @@ igt_main
>>  		igt_assert(igt_list_empty(&settings->env_vars));
>>  		igt_assert(!igt_vec_length(&settings->hook_strs));
>>  		igt_assert(!settings->facts);
>> +		igt_assert(!settings->kmemleak);
>>  		igt_assert(!settings->sync);
>>  		igt_assert_eq(settings->log_level, LOG_LEVEL_NORMAL);
>>  		igt_assert(!settings->overwrite);
>> @@ -426,6 +428,7 @@ igt_main
>>  		igt_assert_eq(settings->include_regexes.size, 0);
>>  		igt_assert_eq(settings->exclude_regexes.size, 0);
>>  		igt_assert(!settings->facts);
>> +		igt_assert(!settings->kmemleak);
>>  		igt_assert(!settings->sync);
>>  		igt_assert_eq(settings->log_level, LOG_LEVEL_NORMAL);
>>  		igt_assert(!settings->overwrite);
>> @@ -464,6 +467,7 @@ igt_main
>>  				       "-b", blacklist_name,
>>  				       "--blacklist", blacklist2_name,
>>  				       "-f",
>> +				       "-k",
>>  				       "-s",
>>  				       "-l", "verbose",
>>  				       "--overwrite",
>> @@ -523,6 +527,7 @@ igt_main
>>  		igt_assert_eqstr(*((char **)igt_vec_elem(&settings->hook_strs, 1)), "echo world");
>>  
>>  		igt_assert(settings->facts);
>> +		igt_assert(settings->kmemleak);
>>  		igt_assert(settings->sync);
>>  		igt_assert_eq(settings->log_level, LOG_LEVEL_VERBOSE);
>>  		igt_assert(settings->overwrite);
>> @@ -718,30 +723,39 @@ igt_main
>>  	igt_subtest("parse-clears-old-data") {
>>  		const char *argv[] = { "runner",
>>  				       "-n", "foo",
>> +				       "--overwrite",
> 
> NIT:
> What does overwrite do here?  Is its addition related to integrating igt_kmemleak scans?
> 
> If we're adding it in just because it was missing, then that should probably be done
> as a separate patch series.
I needed a larger array, and I could not find a way to increase it's size that would
not require me to rewrite the entire file other than adding a new option. So overwrite
was a simple option that does no harm.


> 
> Lower down, we add "foo", "test-root-dir", and "result-path" to the argv list, and I have
> similar comments with respect to those as well.  Same with the igt_asserts on
> settings->overwrite.
> 
>>  				       "--dry-run",
>>  				       "--allow-non-root",
>>  				       "test-root-dir",
>> -				       "results-path",
>> +				       "results-path"
> 
> NIT:
> Removing the comma from the last element of the list is unnecessary, as many
> of the lists in IGT leave the comma there.

I unfortunately missed this from my review. It was removed by accident. Let me know
if you want me to resend with the comma.

> 
> There's definitely a discussion to be had as to whether or not this is proper
> coding style, and if it's not, there's going to be a lot of refactoring work for us
> in the future.  But irrespective of what that discussion results in, refactoring
> work like this is probably out of scope for this patch series.  Especially since
> "overwrite" isn't being appended to the end of the list (and maybe shouldn't
> be added at all?  See first NIT).
> -Jonathan Cavitt
> 

[...]