[PATCH i-g-t] lib/kunit: Read results from debugfs

Janusz Krzysztofik janusz.krzysztofik at linux.intel.com
Thu Mar 28 12:51:18 UTC 2024


On Wednesday, 27 March 2024 22:54:53 CET Janusz Krzysztofik wrote:
> On Wednesday, 27 March 2024 17:03:01 CET Lucas De Marchi wrote:
> > On Wed, Mar 27, 2024 at 12:22:54PM +0100, Janusz Krzysztofik wrote:
> > >KUnit can provide KTAP reports from test modules via debugfs files, one
> > >per test suite.  Using that source of test results instead of extracting
> > >them from dmesg, where they may be interleaved with other kernel messages,
> > >seems more easy to handle and less error prone.  Switch to it.
> > >
> > >If KUnit debugfs support is found not configured then fall back to legacy
> > >processing path.
> > >
> > >Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
> > >---
> > > lib/igt_kmod.c | 143 ++++++++++++++++++++++++++++++++++++-------------
> > > 1 file changed, 105 insertions(+), 38 deletions(-)
> > >
> > >diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > >index 1ec9c8a602..a5b170ca9c 100644
> > >--- a/lib/igt_kmod.c
> > >+++ b/lib/igt_kmod.c
> > >@@ -28,6 +28,7 @@
> > > #include <limits.h>
> > > #include <pthread.h>
> > > #include <signal.h>
> > >+#include <stdio.h>
> > > #include <stdlib.h>
> > > #include <string.h>
> > > #include <sys/stat.h>
> > >@@ -39,6 +40,7 @@
> > >
> > > #include "igt_aux.h"
> > > #include "igt_core.h"
> > >+#include "igt_debugfs.h"
> > > #include "igt_kmod.h"
> > > #include "igt_ktap.h"
> > > #include "igt_sysfs.h"
> > >@@ -864,6 +866,31 @@ static int open_parameters(const char *module_name)
> > > 	return open(path, O_RDONLY);
> > > }
> > >
> > >+static DIR *kunit_debugfs_open(void)
> > >+{
> > >+	const char *debugfs_path = igt_debugfs_mount();
> > >+	int debugfs_fd, kunit_fd;
> > >+	DIR *kunit_dir;
> > >+
> > >+	if (igt_debug_on(!debugfs_path))
> > >+		return NULL;
> > >+
> > >+	debugfs_fd = open(debugfs_path, O_DIRECTORY);
> > >+	if (igt_debug_on(debugfs_fd < 0))
> > >+		return NULL;
> > >+
> > >+	kunit_fd = openat(debugfs_fd, "kunit", O_DIRECTORY);
> > >+	close(debugfs_fd);
> > >+	if (igt_debug_on(kunit_fd < 0))
> > >+		return NULL;
> > >+
> > >+	kunit_dir = fdopendir(kunit_fd);
> > >+	if (igt_debug_on(!kunit_dir))
> > >+		close(kunit_fd);
> > >+
> > >+	return kunit_dir;
> > 
> > 
> > any reason not to use strcat() + return fopen()
> 
> To me the code looked simpler than if I copied and concatenated strings to a 
> local buffer of fixed size with potential truncation handling.  I guess 
> you prefer your pattern over mine, but you haven't explained why.  Would you 
> like to convince me?
> 
> > 
> > >+}
> > >+
> > > static bool kunit_set_filtering(const char *filter_glob, const char *filter,
> > > 				const char *filter_action)
> > > {
> > >@@ -1071,23 +1098,48 @@ static void kunit_results_free(struct igt_list_head *results,
> > > 	free(*suite_name);
> > > }
> > >
> > >-static int kunit_get_results(struct igt_list_head *results, int kmsg_fd,
> > >-			     struct igt_ktap_results **ktap)
> > >+static int kunit_get_results(struct igt_list_head *results, int debugfs_fd,
> > >+			     const char *suite, struct igt_ktap_results **ktap)
> > > {
> > >-	int err;
> > >+	FILE *results_stream;
> > >+	int ret, results_fd;
> > >+	char *buf = NULL;
> > >+	size_t size = 0;
> > >+	ssize_t len;
> > >+
> > >+	if (igt_debug_on((ret = openat(debugfs_fd, suite, O_DIRECTORY), ret < 0)))
> > 
> > a little odd to return on any value != 0, but log on < 0. did you mean
> > to compare < 0 in the first arg?.
> 
> I'm not able to recall what I could mean, but anyway, you are right, 
> 	if (igt_debug_on((ret = openat(...)) < 0))
> will be more correct.

At a second glance, my

	if (igt_debug_on((ret = openat(...), ret < 0)))

was equally correct (both log and return on ret < 0), and now I recall what I 
meant: I tried to work around checkpatch rule of not using assignment 
expressions in if conditions -- with no success.  Let me refactor that line to 
make checkpatch happy.

Thanks,
Janusz

> 
> > 
> > >+		return ret;
> > >+
> > >+	results_fd = openat(ret, "results", O_RDONLY);
> > >+	close(ret);
> > >+	if (igt_debug_on(results_fd < 0))
> > >+		return results_fd;
> > >+
> > >+	results_stream = fdopen(results_fd, "r");
> > >+	if (igt_debug_on(!results_stream)) {
> > >+		close(results_fd);
> > >+		return -errno;
> > >+	}
> > >
> > > 	*ktap = igt_ktap_alloc(results);
> > >-	if (igt_debug_on(!*ktap))
> > >-		return -ENOMEM;
> > >+	if (igt_debug_on(!*ktap)) {
> > >+		ret = -ENOMEM;
> > >+		goto out_fclose;
> > >+	}
> > >+
> > >+	while (len = getline(&buf, &size, results_stream), len > 0) {
> > >+		ret = igt_ktap_parse(buf, *ktap);
> > >+		if (ret != -EINPROGRESS)
> > >+			break;
> > >+	}
> > >
> > >-	do
> > >-		igt_debug_on((err = kunit_kmsg_result_get(results, NULL, kmsg_fd, *ktap),
> > >-			      err && err != -EINPROGRESS));
> > >-	while (err == -EINPROGRESS);
> > >+	free(buf);
> > >
> > > 	igt_ktap_free(ktap);
> > >+out_fclose:
> > >+	fclose(results_stream);
> > >
> > >-	return err;
> > >+	return ret;
> > > }
> > >
> > > static void __igt_kunit_legacy(struct igt_ktest *tst,
> > >@@ -1101,7 +1153,13 @@ static void __igt_kunit_legacy(struct igt_ktest *tst,
> > > 	pthread_mutexattr_t attr;
> > > 	IGT_LIST_HEAD(results);
> > > 	unsigned long taints;
> > >-	int ret;
> > >+	int flags, ret;
> > >+
> > >+	igt_skip_on_f(tst->kmsg < 0, "Could not open /dev/kmsg\n");
> > >+
> > >+	igt_skip_on((flags = fcntl(tst->kmsg, F_GETFL, 0), flags < 0));
> > >+	igt_skip_on_f(fcntl(tst->kmsg, F_SETFL, flags & ~O_NONBLOCK) == -1,
> > >+		      "Could not set /dev/kmsg to blocking mode\n");
> > >
> > > 	igt_skip_on(lseek(tst->kmsg, 0, SEEK_END) < 0);
> > >
> > >@@ -1224,30 +1282,20 @@ static void __igt_kunit_legacy(struct igt_ktest *tst,
> > > 	igt_skip_on_f(ret, "KTAP parser failed\n");
> > > }
> > >
> > >-static void kunit_get_tests_timeout(int signal)
> > >-{
> > >-	igt_skip("Timed out while trying to extract a list of KUnit test cases from /dev/kmsg\n");
> > >-}
> > >-
> > > static bool kunit_get_tests(struct igt_list_head *tests,
> > > 			    struct igt_ktest *tst,
> > > 			    const char *suite,
> > > 			    const char *opts,
> > >+			    DIR *debugfs_dir,
> > > 			    struct igt_ktap_results **ktap)
> > > {
> > >-	struct sigaction sigalrm = { .sa_handler = kunit_get_tests_timeout, },
> > >-			 *saved;
> > > 	struct igt_ktap_result *r, *rn;
> > >+	struct dirent *subdir;
> > > 	unsigned long taints;
> > >-	int flags, err;
> > >-
> > >-	igt_skip_on_f(tst->kmsg < 0, "Could not open /dev/kmsg\n");
> > >+	int debugfs_fd;
> > >
> > >-	igt_skip_on((flags = fcntl(tst->kmsg, F_GETFL, 0), flags < 0));
> > >-	igt_skip_on_f(fcntl(tst->kmsg, F_SETFL, flags & ~O_NONBLOCK) == -1,
> > >-		      "Could not set /dev/kmsg to blocking mode\n");
> > >-
> > >-	igt_skip_on(lseek(tst->kmsg, 0, SEEK_END) < 0);
> > >+	if (igt_debug_on(!debugfs_dir))
> > >+		return false;
> > >
> > > 	/*
> > > 	 * To get a list of test cases provided by a kunit test module, ask the
> > >@@ -1260,19 +1308,32 @@ static bool kunit_get_tests(struct igt_list_head *tests,
> > > 	if (igt_debug_on(!kunit_set_filtering(suite, "module=none", "skip")))
> > > 		return false;
> > >
> > >+	if (!suite) {
> > >+		seekdir(debugfs_dir, 2);	/* directory itself and its parent */
> > >+		errno = 0;
> > >+		igt_skip_on_f(readdir(debugfs_dir) || errno,
> > >+			      "Require empty KUnit debugfs directory\n");
> > >+		rewinddir(debugfs_dir);
> > >+	}
> > >+
> > > 	igt_skip_on(modprobe(tst->kmod, opts));
> > > 	igt_skip_on(igt_kernel_tainted(&taints));
> > >
> > >-	igt_skip_on(sigaction(SIGALRM, &sigalrm, saved));
> > >-	alarm(10);
> > >+	debugfs_fd = dirfd(debugfs_dir);
> > >+	if (suite) {
> > >+		igt_skip_on(kunit_get_results(tests, debugfs_fd, suite, ktap));
> > 
> > instead of skipping, do we need to treat it specially if this returns
> > -EINPROGRESS? That would probably be bug in our ktap parser or a format
> > change or something like that so we may want to start failing rather
> > than skipping.
> 
> Unfortunately we are not allowed to fail here because kunit_get_tests() is 
> called from inside a body of igt_subtest_with_dynamic() but outside of any 
> igt_dynamic(), where fails are allowed again.  We may use a verbose variant of 
> igt_skip() to provide additional information on that skip.  We may also 
> precede that skip with igt_warn(), but I'm not sure how that will by reported 
> by upper layers (igt_runner, CI).

For symmetry with the below case of NULL suite argument, I'd rather use 
igt_warn_on() here and let the subtest skip automatically if the list of KUnit 
test cases occurs empty and no IGT dynamic sub-subtests are executed.  Or 
maybe still better, drop that if section preceding the while section below and 
handle the non-NULL suite case inside the while loop.

Thanks,
Janusz

> 
> > 
> > anyway, consider the comments above as just nits. This seems like a
> > great improvement.
> > 
> > Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>
> 
> Thank you,
> Janusz
> 
> > 
> > thanks
> > Lucas De Marchi
> > 
> > >
> > >-	err = kunit_get_results(tests, tst->kmsg, ktap);
> > >+	} else while (subdir = readdir(debugfs_dir), subdir) {
> > >+		if (!(subdir->d_type & DT_DIR))
> > >+			continue;
> > >
> > >-	alarm(0);
> > >-	igt_debug_on(sigaction(SIGALRM, saved, NULL));
> > >+		if (!strcmp(subdir->d_name, ".") || !strcmp(subdir->d_name, ".."))
> > >+			continue;
> > >
> > >-	igt_skip_on_f(err,
> > >-		      "KTAP parser failed while getting a list of test cases\n");
> > >+		igt_warn_on_f(kunit_get_results(tests, debugfs_fd, subdir->d_name, ktap),
> > >+			      "parsing KTAP report from test suite \"%s\" failed\n",
> > >+			      subdir->d_name);
> > >+	}
> > >
> > > 	igt_list_for_each_entry_safe(r, rn, tests, link)
> > > 		igt_require_f(r->code == IGT_EXIT_SKIP,
> > >@@ -1287,6 +1348,7 @@ static void __igt_kunit(struct igt_ktest *tst,
> > > 			const char *subtest,
> > > 			const char *suite,
> > > 			const char *opts,
> > >+			int debugfs_fd,
> > > 			struct igt_list_head *tests,
> > > 			struct igt_ktap_results **ktap)
> > > {
> > >@@ -1307,8 +1369,6 @@ static void __igt_kunit(struct igt_ktest *tst,
> > >
> > > 			igt_skip_on(igt_kernel_tainted(&taints));
> > >
> > >-			igt_fail_on(lseek(tst->kmsg, 0, SEEK_END) == -1 && errno);
> > >-
> > > 			igt_assert_lt(snprintf(glob, sizeof(glob), "%s.%s",
> > > 					       t->suite_name, t->case_name),
> > > 				      sizeof(glob));
> > >@@ -1317,7 +1377,8 @@ static void __igt_kunit(struct igt_ktest *tst,
> > > 			igt_assert_eq(modprobe(tst->kmod, opts), 0);
> > > 			igt_assert_eq(igt_kernel_tainted(&taints), 0);
> > >
> > >-			igt_assert_eq(kunit_get_results(&results, tst->kmsg, ktap), 0);
> > >+			igt_assert_eq(kunit_get_results(&results, debugfs_fd,
> > >+							t->suite_name, ktap), 0);
> > >
> > > 			for (i = 0; i < 2; i++) {
> > > 				kunit_result_free(&r, &suite_name, &case_name);
> > >@@ -1388,6 +1449,7 @@ void igt_kunit(const char *module_name, const char *suite, const char *opts)
> > > 	struct igt_ktest tst = { .kmsg = -1, };
> > > 	struct igt_ktap_results *ktap = NULL;
> > > 	const char *subtest = suite;
> > >+	DIR *debugfs_dir = NULL;
> > > 	IGT_LIST_HEAD(tests);
> > >
> > > 	/*
> > >@@ -1435,10 +1497,12 @@ void igt_kunit(const char *module_name, const char *suite, const char *opts)
> > > 		 *	 LTS kernels not capable of using KUnit filters for
> > > 		 *	 listing test cases in KTAP format, with igt_require.
> > > 		 */
> > >-		if (!kunit_get_tests(&tests, &tst, suite, opts, &ktap))
> > >+		debugfs_dir = kunit_debugfs_open();
> > >+		if (!kunit_get_tests(&tests, &tst, suite, opts, debugfs_dir, &ktap))
> > > 			__igt_kunit_legacy(&tst, subtest, opts);
> > > 		else
> > >-			__igt_kunit(&tst, subtest, suite, opts, &tests, &ktap);
> > >+			__igt_kunit(&tst, subtest, suite, opts,
> > >+				    dirfd(debugfs_dir), &tests, &ktap);
> > > 	}
> > >
> > > 	igt_fixture {
> > >@@ -1448,6 +1512,9 @@ 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