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

[Kamil Konieczny]

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.

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

Regards,
Kamil

> 
> As I had explained to Jonathan Cavitt in a previous email, the alternative
> to adding an extra option would be to change how the entire file allocates
> argv[] which felt like an overkill. If adding an extra option that has no
> downstream effect is unacceptable, please let me know how you want me to
> grow the array with an example.
> 
> > 
> >>  				       "--dry-run",
> >>  				       "--allow-non-root",
> >>  				       "test-root-dir",
> >> @@ -727,21 +733,29 @@ igt_main
> >>  		igt_assert(parse_options(ARRAY_SIZE(argv), (char**)argv, settings));
> >>  
> >>  		igt_assert_eqstr(settings->name, "foo");
> >> +		igt_assert(settings->overwrite);
> > 
> > Same here, unrelated.
> Updating the unit testing to take overwrite into account.
> 
> > 
> >>  		igt_assert(settings->dry_run);
> >>  		igt_assert(!settings->test_list);
> >>  		igt_assert(!settings->facts);
> >> +		igt_assert(!settings->kmemleak);
> >>  		igt_assert(!settings->sync);
> >>  
> >>  		argv[1] = "--test-list";
> >> +		argv[2] = "foo"; /* Unchanged */
> > 
> > Same here, unrelated.
> This is unchanged as the comment indicates. It is here only for documentation.
> It was defined a few lines before: const char *argv[] = { "runner", "-n", "foo",
> 
> > 
> >>  		argv[3] = "--facts";
> >> -		argv[4] = "--sync";
> > 
> > Same here, unrelated.
> Just read the next two lines please.
> 
> > 
> >> +		argv[4] = "--kmemleak";
> >> +		argv[5] = "--sync";
> > 
> > Same here, unrelated.
> Just read the previous two lines please.
> 
> > 
> >> +		argv[6] = "test-root-dir"; /* Unchanged */
> > 
> > Same here, unrelated.
> Same explanation as 'foo'. No changes.
> > 
> >> +		argv[7] = "results-path"; /* Unchanged */
> > 
> > Same here, unrelated.
> Same explanation as 'foo'. No changes.
> 
> > 
> >>  
> >>  		igt_assert(parse_options(ARRAY_SIZE(argv), (char**)argv, settings));
> >>  
> >>  		igt_assert_eqstr(settings->name, "results-path");
> >>  		igt_assert(!settings->dry_run);
> >> +		igt_assert(!settings->overwrite);
> > 
> > Same here, unrelated.
> 
> Same as before.
> 
> [...]
> 
> >>  	settings->dmesg_warn_level = -1;
> >>  	settings->prune_mode = -1;
> >>  
> >> -	while ((c = getopt_long(argc, argv, "hn:dt:x:e:fsl:omb:L",
> >> +	while ((c = getopt_long(argc, argv, "hn:dt:x:e:fsl:omb:Lk::",
> > 
> > Why there are two colons at end? Also imho 'k' should be placed after 'f'
> > like below:
> 
> From Getopt documentation*: If an option character is followed by two colons
> (‘::’), its argument is optional;
> 
> > 
> > 	while ((c = getopt_long(argc, argv, "hn:dt:x:e:fk:sl:omb:L",
> 
> This is wrong as it is missing one ':'. I will fix the order.
> 
> > 
> > especially that you add its code after OPT_FACTS below.
> > 
> >>  				long_options, NULL)) != -1) {
> >>  		switch (c) {
> >>  		case OPT_VERSION:
> >> @@ -742,6 +754,19 @@ bool parse_options(int argc, char **argv,
> >>  		case OPT_FACTS:
> >>  			settings->facts = true;
> >>  			break;
> >> +		case OPT_KMEMLEAK:
> >> +			settings->kmemleak = true;
> >> +			if (optarg) {
> >> +				if (strcmp(optarg, "once") == 0)
> >> +					settings->kmemleak_each = false;
> >> +				else if (strcmp(optarg, "each") == 0)
> >> +					settings->kmemleak_each = true;
> >> +				else {
> >> +					usage(stderr, "Invalid kmemleak option");
> >> +					goto error;
> >> +				}
> > 
> > Use braces here: if () { ... } else if (...) { ... } else { ... }
> 
> Sure
> 
> [...]
> 
> * - https://www.gnu.org/software/libc/manual/html_node/Using-Getopt.html