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

[Cavitt, Jonathan]

-----Original Message-----
From: Peter Senna Tschudin <peter.senna at linux.intel.com> 
Sent: Monday, January 27, 2025 3:06 AM
To: Cavitt, Jonathan <jonathan.cavitt at intel.com>; igt-dev at lists.freedesktop.org
Cc: stylon.wang at amd.com; Rodrigo.Siqueira at amd.com; Gandi, Ramadevi <ramadevi.gandi at intel.com>; Knop, Ryszard <ryszard.knop at intel.com>; Lattannavar, Sameer <sameer.lattannavar at intel.com>; De Marchi, Lucas <lucas.demarchi at intel.com>; Saarinen, Jani <jani.saarinen at intel.com>; Piecielska, Katarzyna <katarzyna.piecielska at intel.com>; Roper, Matthew D <matthew.d.roper at intel.com>; gregory.f.germano at intel.com; Taylor, Clinton A <clinton.a.taylor at intel.com>; Vivekanandan, Balasubramani <balasubramani.vivekanandan at intel.com>; Yu, Jianshui <jianshui.yu at intel.com>
Subject: Re: [PATCH i-g-t 2/2] runner/executor: Integrate igt_kmemleak scans
> On 22.01.2025 19:18, Cavitt, Jonathan wrote:
> 
> [...]
> 

[...]

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

Ah... Okay, I think I understand.  I was under the initial impression that we were adding
a new tag to some runner setup code, but it looks like this is done as an argv parsing
test.  In other words, though I initially believed this might have an impact on
functionality, that turned out to not be the case.

It's at least more understandable now why the tag needed to be added.  In an ideal
world, we wouldn't need to add that tag to this test, but I won't force the issue.

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

I don't think it's necessary unless someone else calls it out.  Or, if you get more review
notes, you can just change it for v3.
-Jonathan Cavitt

> 
> > 
> > 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
> > 
> 
> [...]
>