[PATCH i-g-t v3 3/6] lib/igt_kmod: Stop passing debugfs_dir around

[Lucas De Marchi]

On Thu, Nov 14, 2024 at 03:24:39PM +0100, Janusz Krzysztofik wrote:
>Hi Lucas,
>
>On Thursday, 7 November 2024 06:52:51 CET Lucas De Marchi wrote:
>> Just open and closes it while getting the list of tests.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>>  lib/igt_kmod.c | 27 ++++++++++++---------------
>>  1 file changed, 12 insertions(+), 15 deletions(-)
>>
>> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
>> index 3729a053c..36578aa68 100644
>> --- a/lib/igt_kmod.c
>> +++ b/lib/igt_kmod.c
>> @@ -1038,15 +1038,15 @@ static void kunit_get_tests(struct igt_list_head *tests,
>>  			    const char *suite,
>>  			    const char *opts,
>>  			    const char *debugfs_path,
>> -			    DIR **debugfs_dir,
>>  			    struct igt_ktap_results **ktap)
>>  {
>>  	struct igt_ktap_result *r, *rn;
>>  	struct dirent *subdir;
>>  	unsigned long taints;
>> +	DIR *debugfs_dir;
>>
>> -	*debugfs_dir = opendir(debugfs_path);
>> -	if (igt_debug_on(!*debugfs_dir))
>> +	debugfs_dir = opendir(debugfs_path);
>> +	if (igt_debug_on(!debugfs_dir))
>>  		return;
>>
>>  	/*
>> @@ -1057,21 +1057,23 @@ static void kunit_get_tests(struct igt_list_head *tests,
>>  	 * we already can do perfectly -- seems to be more safe than extracting
>>  	 * a free text list of unknown length from /dev/kmsg.
>>  	 */
>> -	if (igt_debug_on(!kunit_set_filtering(suite, "module=none", "skip")))
>> +	if (igt_debug_on(!kunit_set_filtering(suite, "module=none", "skip"))) {
>> +		closedir(debugfs_dir);
>>  		return;
>> +	}
>>
>>  	if (!suite) {
>> -		seekdir(*debugfs_dir, 2);	/* directory itself and its parent */
>> +		seekdir(debugfs_dir, 2);	/* directory itself and its parent */
>>  		errno = 0;
>> -		igt_skip_on_f(readdir(*debugfs_dir) || errno,
>> +		igt_skip_on_f(readdir(debugfs_dir) || errno,
>>  			      "Require empty KUnit debugfs directory\n");
>> -		rewinddir(*debugfs_dir);
>> +		rewinddir(debugfs_dir);
>>  	}
>>
>>  	igt_skip_on(modprobe(tst->kmod, opts));
>>  	igt_skip_on(igt_kernel_tainted(&taints));
>
>How are we going to avoid leaking the open debugfs_dir descriptor if one of
>those igt_skips triggers a longjmp?

ugh, you're right. I hate these longjmp :(. I wish the require/assert
would be left to the *tests* and lib code act as lib code :-/

I will drop this.

Thanks
Lucas De Marchi

>
>Thanks,
>Janusz
>
>>
>> -	while (subdir = readdir(*debugfs_dir), subdir) {
>> +	while (subdir = readdir(debugfs_dir), subdir) {
>>  		if (!(subdir->d_type & DT_DIR))
>>  			continue;
>>
>> @@ -1089,8 +1091,7 @@ static void kunit_get_tests(struct igt_list_head *tests,
>>  			break;
>>  	}
>>
>> -	closedir(*debugfs_dir);
>> -	*debugfs_dir = NULL;
>> +	closedir(debugfs_dir);
>>
>>  	igt_list_for_each_entry_safe(r, rn, tests, link)
>>  		igt_require_f(r->code == IGT_EXIT_SKIP,
>> @@ -1197,7 +1198,6 @@ void igt_kunit(const char *module_name, const char *suite, const char *opts)
>>  	char debugfs_path[PATH_MAX] = { '\0', };
>>  	struct igt_ktest tst = { .kmsg = -1, };
>>  	struct igt_ktap_results *ktap = NULL;
>> -	DIR *debugfs_dir = NULL;
>>  	char *subtest;
>>  	IGT_LIST_HEAD(tests);
>>
>> @@ -1244,7 +1244,7 @@ void igt_kunit(const char *module_name, const char *suite, const char *opts)
>>  	 */
>>  	igt_subtest_with_dynamic(subtest) {
>>  		kunit_get_tests(&tests, &tst, suite, opts,
>> -				debugfs_path, &debugfs_dir, &ktap);
>> +				debugfs_path, &ktap);
>>  		__igt_kunit(&tst, subtest, opts, debugfs_path, &tests, &ktap);
>>  	}
>>
>> @@ -1255,9 +1255,6 @@ void igt_kunit(const char *module_name, const char *suite, const char *opts)
>>
>>  		kunit_results_free(&tests, &suite_name, &case_name);
>>
>> -		if (debugfs_dir)
>> -			closedir(debugfs_dir);
>> -
>>  		igt_ktest_end(&tst);
>>  	}
>>
>>
>
>
>
>