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

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

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?


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

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