[PATCH i-g-t v4 3/6] lib/igt_kmod: Let the fixture open/close debugfs

[Lucas De Marchi]

On Tue, Nov 19, 2024 at 12:02:58PM +0100, Janusz Krzysztofik wrote:
>Hi Lucas,
>
>On Tuesday, 19 November 2024 06:29:51 CET Lucas De Marchi wrote:
>> Do not pass as argument and let the called function open/close since the
>> caller also has to deal with fallouts due to igt_require() and the like.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>>  lib/igt_kmod.c | 27 +++++++++++----------------
>>  1 file changed, 11 insertions(+), 16 deletions(-)
>>
>> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
>> index 3729a053c..51032502f 100644
>> --- a/lib/igt_kmod.c
>> +++ b/lib/igt_kmod.c
>> @@ -1038,17 +1038,13 @@ static void kunit_get_tests(struct igt_list_head *tests,
>>  			    const char *suite,
>>  			    const char *opts,
>>  			    const char *debugfs_path,
>> -			    DIR **debugfs_dir,
>> +			    DIR *debugfs_dir,
>>  			    struct igt_ktap_results **ktap)
>>  {
>>  	struct igt_ktap_result *r, *rn;
>>  	struct dirent *subdir;
>>  	unsigned long taints;
>>
>> -	*debugfs_dir = opendir(debugfs_path);
>> -	if (igt_debug_on(!*debugfs_dir))
>> -		return;
>> -
>>  	/*
>>  	 * To get a list of test cases provided by a kunit test module, ask the
>>  	 * generic kunit module to respond with SKIP result for each test found.
>> @@ -1061,17 +1057,17 @@ static void kunit_get_tests(struct igt_list_head *tests,
>>  		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));
>>
>> -	while (subdir = readdir(*debugfs_dir), subdir) {
>> +	while (subdir = readdir(debugfs_dir), subdir) {
>>  		if (!(subdir->d_type & DT_DIR))
>>  			continue;
>>
>> @@ -1089,9 +1085,6 @@ static void kunit_get_tests(struct igt_list_head *tests,
>>  			break;
>>  	}
>>
>> -	closedir(*debugfs_dir);
>> -	*debugfs_dir = NULL;
>> -
>>  	igt_list_for_each_entry_safe(r, rn, tests, link)
>>  		igt_require_f(r->code == IGT_EXIT_SKIP,
>>  			      "Unexpected non-SKIP result while listing test cases\n");
>> @@ -1226,13 +1219,16 @@ void igt_kunit(const char *module_name, const char *suite, const char *opts)
>>  	kunit_debugfs_path(debugfs_path);
>>
>>  	igt_fixture {
>> +		igt_assert(igt_list_empty(&tests));
>> +
>>  		igt_require(subtest);
>>  		igt_require(*debugfs_path);
>>
>>  		igt_skip_on(igt_ktest_init(&tst, module_name));
>>  		igt_skip_on(igt_ktest_begin(&tst));
>>
>> -		igt_assert(igt_list_empty(&tests));
>> +		debugfs_dir = opendir(debugfs_path);
>> +		igt_skip_on(!debugfs_dir);
>
>I shouldn't complain because that's exactly what I suggested, but now looking
>at it again, I'm still not satisfied, sorry.
>
>In my original version of that opening igt_fixture section, there were no
>other skips nor fails after the igt_kip_on(igt_ktest_begin()).
>
>Looking at an equivalent code in igt_kselftest(), my assumption was that it
>was safe to call igt_ktest_fini() unconditionally, even if igt_ktest_init()
>failed, and that was not the case with igt_ktest_end(), which could be called
>safely only if igt_ktest_begin() succeeded.  That's why I placed
>igt_ktest_end() inside, and igt_ktest_fini() outside a closing igt_fixture
>section.
>
>Then, in one of my patches I added that unfortunate igt_assert() to the end of
>the section and introduced a risk of the closing igt_fixture not executed,
>then igt_ktest_end() not being called, and some resources stored in tst
>leaked.  That needs to be fixed, in a separate patch, I believe, e.g. by
>moving that igt_assert() in front of igt_ktest_begin().  I would also add a
>comment like:
>   /* no further igt_skips or igt_fails to avoid leaking of tst resources */
>right after the call to igt_ktest_begin().
>
>Taking that into account, your igt_skip_on(!debugfs_dir) may be called either
>before igt_ktest_begin(), or from the top of igt_subtest_with_dynamic() body,
>and closedir(debugfs_dir) should be moved out of the closing igt_fixture
>section, and still called conditionally to be on the safe side.


ugh.... this is ugly. For now, I'm just pushing the first 2 commits and
will look into the rest when I have more time.

thanks
Lucas De Marchi

>
>Thanks,
>Janusz
>
>>  	}
>>
>>  	/*
>> @@ -1244,7 +1240,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, debugfs_dir, &ktap);
>>  		__igt_kunit(&tst, subtest, opts, debugfs_path, &tests, &ktap);
>>  	}
>>
>> @@ -1255,8 +1251,7 @@ 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);
>> +		closedir(debugfs_dir);
>>
>>  		igt_ktest_end(&tst);
>>  	}
>>
>
>
>
>