| Message ID | 20240327113700.7123-2-janusz.krzysztofik@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t] lib/kunit: Read results from debugfs | expand |
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@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() >+} >+ > 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?. >+ 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. anyway, consider the comments above as just nits. This seems like a great improvement. Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> 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); > } > >-- >2.44.0 >
Hi Janusz, On 2024-03-27 at 12:22:54 +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@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); imho here: close(kunit_fd); igt_debug_on(!kunit_dir); > + > + return kunit_dir; > +} > + > 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))) > + 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; imho this should be before you set blocking mode or before you call this function. > > /* > * 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); Why you removed alarm(10)? Regards, Kamil > + debugfs_fd = dirfd(debugfs_dir); > + if (suite) { > + igt_skip_on(kunit_get_results(tests, debugfs_fd, suite, ktap)); > > - 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); > } > > -- > 2.44.0 >
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@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. > > >+ 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). > > anyway, consider the comments above as just nits. This seems like a > great improvement. > > Reviewed-by: Lucas De Marchi <lucas.demarchi@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); > > } > > >
On Wednesday, 27 March 2024 18:27:50 CET Kamil Konieczny wrote: > Hi Janusz, > On 2024-03-27 at 12:22:54 +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@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); > > imho here: > close(kunit_fd); > igt_debug_on(!kunit_dir); No, we are not allowed to close it. fdopendir(3) states: "After a successful call to fdopendir(), fd is used internally by the implementation, and should not otherwise be used by the application." > > > + > > + return kunit_dir; > > +} > > + > > 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))) > > + 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; > > imho this should be before you set blocking mode This patch removes the lines that were setting blocking mode. That mode was needed only when reading from /dev/kmsg. > or before you call this function. OK. > > > > > /* > > * 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); > > Why you removed alarm(10)? Because we no longer need it. When we were extracting a KTAP report from /dev/kmsg then it was needed because if KUnit filters produced no KTAP report then that operation might never complete unless interrupted. Now our attempt to open a nonexistent debugfs report file fails immediately. Thanks, Janusz > > Regards, > Kamil > > > + debugfs_fd = dirfd(debugfs_dir); > > + if (suite) { > > + igt_skip_on(kunit_get_results(tests, debugfs_fd, suite, ktap)); > > > > - 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); > > } > > >
Hi Janusz, On 2024-03-27 at 23:26:03 +0100, Janusz Krzysztofik wrote: > On Wednesday, 27 March 2024 18:27:50 CET Kamil Konieczny wrote: > > Hi Janusz, > > On 2024-03-27 at 12:22:54 +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@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); > > > > imho here: > > close(kunit_fd); > > igt_debug_on(!kunit_dir); > > No, we are not allowed to close it. fdopendir(3) states: > "After a successful call to fdopendir(), fd is used internally by the > implementation, and should not otherwise be used by the application." > You are right, thank you for explanation and pointing me to man page. Regards, Kamil > > > > > + > > > + return kunit_dir; > > > +} > > > + > > > 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))) > > > + 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; > > > > imho this should be before you set blocking mode > > This patch removes the lines that were setting blocking mode. That mode was > needed only when reading from /dev/kmsg. > > > or before you call this function. > > OK. > > > > > > > > > /* > > > * 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); > > > > Why you removed alarm(10)? > > Because we no longer need it. When we were extracting a KTAP report from > /dev/kmsg then it was needed because if KUnit filters produced no KTAP report > then that operation might never complete unless interrupted. Now our attempt > to open a nonexistent debugfs report file fails immediately. > > Thanks, > Janusz > > > > > Regards, > > Kamil > > > > > + debugfs_fd = dirfd(debugfs_dir); > > > + if (suite) { > > > + igt_skip_on(kunit_get_results(tests, debugfs_fd, suite, ktap)); > > > > > > - 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); > > > } > > > > > > > > >
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@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@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); > > > } > > > > > > >
On Wed, Mar 27, 2024 at 10:54:53PM +0100, Janusz Krzysztofik wrote: >> >+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? not really important. It just seems simpler to me, with no calls into the kernel... completely untested, but something like: const char *debugfs_path = igt_debugfs_mount(); char path[PATH_MAX]; if (igt_debug_on(!debugfs_path || (strlen(debugfs_path) + strlen("/kunit") >= PATH_MAX))) return NULL; strcat(stpcpy(path, debugfs_path), "/kunit"); return opendir(path); Lucas De Marchi
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; +} + 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))) + 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)); - 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); }
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@linux.intel.com> --- lib/igt_kmod.c | 143 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 105 insertions(+), 38 deletions(-)