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

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

Hi Kamil,

On 12.02.2025 15:24, Kamil Konieczny wrote:
> Hi Peter,
> On 2025-02-03 at 10:10:53 +0100, Peter Senna Tschudin wrote:
>> Hi Kamil,
>>
>> Thank you for the review.
>>
>> [...]
>>
>>>>  		igt_facts_lists_init();
>>>>  
>>>> +	if (settings->kmemleak)
>>>> +		if (!igt_kmemleak_init(NULL)) {
>>>> +			errf("Failed to initialize kmemleak. Is kernel support enabled?\n");
>>>> +			errf("Disabling kmemleak on igt_runner and continuing...\n");
>>>
>>> Make it one errf() call, split string if needed.
>> chekcpatch does not like multi-line strings. Can you show me the code
>> you want me to use here that satisfies you and checkpatch at the same time?
>>
> 
> It is a tool and it do happen when developer could ignore it,
> you have one example here and other is for drm structs which
> use camel case names.

So you want a single errf() with a multi line string and you are happy to ignore
checkpatch complaining about it. Cool, will do.

> 
>>
>> [...]
>>>
>>>> +			settings->kmemleak = settings->kmemleak_each = false;
>>>
>>> Avoid this, write two assinments here.
>>
>> I personally find this very elegant, but sure, I will make the change.
>>
>> [...]
>>
>>>> @@ -718,6 +723,7 @@ igt_main
>>>>  	igt_subtest("parse-clears-old-data") {
>>>>  		const char *argv[] = { "runner",
>>>>  				       "-n", "foo",
>>>> +				       "--overwrite",
>>>
>>> Do not make unrelated changes, add only kmemleak.
>>> You also didn't write about this change in description, if you
>>> think it is a fix it should go in separate patch, not here.
>>
>> These are not unrelated. First as this is unit testing, this has no
>> downstream effects. Second, I am adding --overwrite to grow the argv
>> array. The reason for that is to be able to test the new argument.
> 
> Well, I do no understand why are you doing this in this patch?
> When you added facts you change only small fragment but now it
> looks like an unneccecery big change. Please make this change
> small and consider adding all that bigger unrelated changes
> in separate cleanup patch after this one.

I was lucky that there was a free slot on the array for facts. Now I cannot
unit test facts and kmemleak at the same time without growing the array.
Sorry. Do you see an easy way that I am missing?

Thank you,

Peter