[PATCH i-g-t v3 3/6] lib/igt_kmod: Stop passing debugfs_dir around
Lucas De Marchi
lucas.demarchi at intel.com
Thu Nov 14 21:45:31 UTC 2024
On Thu, Nov 14, 2024 at 02:58:49PM -0600, Lucas De Marchi wrote:
>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.
humn.... what do you think about keeping open/close in the fixture and
just removing the pointer to pointer? Something like this:
diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index 3729a053c..2b96d2c45 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");
@@ -1232,6 +1225,9 @@ void igt_kunit(const char *module_name, const char *suite, const char *opts)
igt_skip_on(igt_ktest_init(&tst, module_name));
igt_skip_on(igt_ktest_begin(&tst));
+ debugfs_dir = opendir(debugfs_path);
+ igt_skip_on(!debugfs_dir);
+
igt_assert(igt_list_empty(&tests));
}
@@ -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);
}
>
>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);
>>> }
>>>
>>>
>>
>>
>>
>>
More information about the igt-dev
mailing list