| Message ID | 1570546546-549-2-git-send-email-alan.maguire@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kunit: support building core/tests as modules | expand |
On Tue, Oct 08, 2019 at 03:55:44PM +0100, Alan Maguire wrote: > as tests are added to kunit, it will become less feasible to execute > all built tests together. By supporting modular tests we provide > a simple way to do selective execution on a running system; specifying > > CONFIG_KUNIT=y > CONFIG_KUNIT_EXAMPLE_TEST=m > > ...means we can simply "insmod example-test.ko" to run the tests. > > To achieve this we need to > > o export the required symbols in kunit > o support a new way of declaring test suites. Because a module cannot > do multiple late_initcall()s, we provide a kunit_test_suites() macro > to declare multiple suites within the same module at once. > > Signed-off-by: Alan Maguire <alan.maguire@oracle.com> > Signed-off-by: Knut Omang <knut.omang@oracle.com> > > --- > include/kunit/test.h | 30 +++++++++++++++++++++++------- > kernel/sysctl-test.c | 6 +++++- > lib/Kconfig.debug | 4 ++-- > lib/kunit/Kconfig | 4 ++-- > lib/kunit/assert.c | 8 ++++++++ > lib/kunit/example-test.c | 6 +++++- > lib/kunit/string-stream-test.c | 9 +++++++-- > lib/kunit/string-stream.c | 7 +++++++ > lib/kunit/test-test.c | 8 ++++++-- > lib/kunit/test.c | 8 ++++++++ > lib/kunit/try-catch.c | 8 ++++++-- > 11 files changed, 79 insertions(+), 19 deletions(-) > > diff --git a/include/kunit/test.h b/include/kunit/test.h > index dba4830..9fc6c1b 100644 > --- a/include/kunit/test.h > +++ b/include/kunit/test.h > @@ -12,6 +12,7 @@ > #include <kunit/assert.h> > #include <kunit/try-catch.h> > #include <linux/kernel.h> > +#include <linux/module.h> > #include <linux/slab.h> > #include <linux/types.h> > > @@ -204,24 +205,39 @@ struct kunit { > * Registers @suite with the test framework. See &struct kunit_suite for > * more information. > * > - * NOTE: Currently KUnit tests are all run as late_initcalls; this means > + * When builtin, KUnit tests are all run as late_initcalls; this means > * that they cannot test anything where tests must run at a different init > * phase. One significant restriction resulting from this is that KUnit > * cannot reliably test anything that is initialize in the late_init phase; > * another is that KUnit is useless to test things that need to be run in > * an earlier init phase. > * > + * An alternative is to build the tests as a module. Because modules > + * do not support multiple late_initcall()s, we need to initialize an > + * array of suites for a module. > + * > * TODO(brendanhiggins@google.com): Don't run all KUnit tests as > * late_initcalls. I have some future work planned to dispatch all KUnit > * tests from the same place, and at the very least to do so after > * everything else is definitely initialized. > */ > -#define kunit_test_suite(suite) \ > - static int kunit_suite_init##suite(void) \ > - { \ > - return kunit_run_tests(&suite); \ > - } \ > - late_initcall(kunit_suite_init##suite) > +#define kunit_test_suites(...) \ > + static struct kunit_suite *suites[] = { __VA_ARGS__, NULL}; \ > + static int kunit_test_suites_init(void) \ > + { \ > + unsigned int i; \ > + for (i = 0; suites[i] != NULL; i++) \ > + kunit_run_tests(suites[i]); \ > + return 0; \ > + } \ > + late_initcall(kunit_test_suites_init); \ > + static void __exit kunit_test_suites_exit(void) \ > + { \ > + return; \ > + } \ > + module_exit(kunit_test_suites_exit) > + > +#define kunit_test_suite(suite) kunit_test_suites(suite) I think it is fine to just rename this kunit_test_suites. > /* > * Like kunit_alloc_resource() below, but returns the struct kunit_resource > diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c > index 2a63241..15161c5 100644 > --- a/kernel/sysctl-test.c > +++ b/kernel/sysctl-test.c > @@ -389,4 +389,8 @@ static void sysctl_test_api_dointvec_write_single_greater_int_max( > .test_cases = sysctl_test_cases, > }; > > -kunit_test_suite(sysctl_test_suite); > +kunit_test_suite(&sysctl_test_suite); > + > +#ifdef MODULE > +MODULE_LICENSE("GPL"); > +#endif /* MODULE */ Here and elsewhere: the "ifdef/endif MODULE" should not be necessary. > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index a3017a5..f9f411a6 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -1951,10 +1951,10 @@ config TEST_SYSCTL > If unsure, say N. > > config SYSCTL_KUNIT_TEST > - bool "KUnit test for sysctl" > + tristate "KUnit test for sysctl" > depends on KUNIT > help > - This builds the proc sysctl unit test, which runs on boot. > + This builds the proc sysctl unit test, which runs on boot/module load. > Tests the API contract and implementation correctness of sysctl. > For more information on KUnit and unit tests in general please refer > to the KUnit documentation in Documentation/dev-tools/kunit/. [...] > diff --git a/lib/kunit/example-test.c b/lib/kunit/example-test.c > index f64a829..6c6a408 100644 > --- a/lib/kunit/example-test.c > +++ b/lib/kunit/example-test.c > @@ -85,4 +85,8 @@ static int example_test_init(struct kunit *test) > * This registers the above test suite telling KUnit that this is a suite of > * tests that need to be run. > */ > -kunit_test_suite(example_test_suite); > +kunit_test_suite(&example_test_suite); > + > +#ifdef MODULE > +MODULE_LICENSE("GPL"); > +#endif /* MODULE */ nit: The "ifdef/endif MODULE" should not be necessary. > diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c > index 76cc05e..7a3e7a0 100644 > --- a/lib/kunit/string-stream-test.c > +++ b/lib/kunit/string-stream-test.c > @@ -45,8 +45,13 @@ static void string_stream_test_get_string(struct kunit *test) > {} > }; > > -static struct kunit_suite string_stream_test_suite = { > +struct kunit_suite string_stream_test_suite = { > .name = "string-stream-test", > .test_cases = string_stream_test_cases > }; > -kunit_test_suite(string_stream_test_suite); > + > +kunit_test_suite(&string_stream_test_suite); > + > +#ifdef MODULE > +MODULE_LICENSE("GPL"); > +#endif /* MODULE */ > diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c > index e6d17aa..e4f3a97 100644 > --- a/lib/kunit/string-stream.c > +++ b/lib/kunit/string-stream.c > @@ -100,6 +100,7 @@ int string_stream_vadd(struct string_stream *stream, > > return 0; > } > +EXPORT_SYMBOL_GPL(string_stream_vadd); Is this actually needed by anything other than lib/kunit/test.c right now? Maybe we should move the include file into the kunit/ directory to hide these so no one else can use them. > int string_stream_add(struct string_stream *stream, const char *fmt, ...) > { > @@ -112,6 +113,7 @@ int string_stream_add(struct string_stream *stream, const char *fmt, ...) > > return result; > } > +EXPORT_SYMBOL_GPL(string_stream_add); [...] > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index c83c0fa..e7896f1 100644 > --- a/lib/kunit/test.c > +++ b/lib/kunit/test.c [...] > @@ -50,6 +51,7 @@ static unsigned long kunit_test_timeout(void) > * For more background on this topic, see: > * https://mike-bland.com/2011/11/01/small-medium-large.html > */ > +#ifndef MODULE Why is this block of code "ifndef MODULE"? > if (sysctl_hung_task_timeout_secs) { > /* > * If sysctl_hung_task is active, just set the timeout to some > @@ -60,9 +62,9 @@ static unsigned long kunit_test_timeout(void) > */ > timeout_msecs = (sysctl_hung_task_timeout_secs - 1) * > MSEC_PER_SEC; > - } else { > + } else > +#endif > timeout_msecs = 300 * MSEC_PER_SEC; /* 5 min */ > - } > > return timeout_msecs; > } > @@ -106,6 +108,7 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context) > > try_catch->catch(try_catch->context); > } > +EXPORT_SYMBOL_GPL(kunit_try_catch_run); > > void kunit_try_catch_init(struct kunit_try_catch *try_catch, > struct kunit *test, > @@ -116,3 +119,4 @@ void kunit_try_catch_init(struct kunit_try_catch *try_catch, > try_catch->try = try; > try_catch->catch = catch; > } > +EXPORT_SYMBOL_GPL(kunit_try_catch_init); This code should also probably be hidden from outside of kunit/.
On Tue, 8 Oct 2019, Brendan Higgins wrote: > On Tue, Oct 08, 2019 at 03:55:44PM +0100, Alan Maguire wrote: > > as tests are added to kunit, it will become less feasible to execute > > all built tests together. By supporting modular tests we provide > > a simple way to do selective execution on a running system; specifying > > > > CONFIG_KUNIT=y > > CONFIG_KUNIT_EXAMPLE_TEST=m > > > > ...means we can simply "insmod example-test.ko" to run the tests. > > > > To achieve this we need to > > > > o export the required symbols in kunit > > o support a new way of declaring test suites. Because a module cannot > > do multiple late_initcall()s, we provide a kunit_test_suites() macro > > to declare multiple suites within the same module at once. > > > > Signed-off-by: Alan Maguire <alan.maguire@oracle.com> > > Signed-off-by: Knut Omang <knut.omang@oracle.com> > > > > --- > > include/kunit/test.h | 30 +++++++++++++++++++++++------- > > kernel/sysctl-test.c | 6 +++++- > > lib/Kconfig.debug | 4 ++-- > > lib/kunit/Kconfig | 4 ++-- > > lib/kunit/assert.c | 8 ++++++++ > > lib/kunit/example-test.c | 6 +++++- > > lib/kunit/string-stream-test.c | 9 +++++++-- > > lib/kunit/string-stream.c | 7 +++++++ > > lib/kunit/test-test.c | 8 ++++++-- > > lib/kunit/test.c | 8 ++++++++ > > lib/kunit/try-catch.c | 8 ++++++-- > > 11 files changed, 79 insertions(+), 19 deletions(-) > > > > diff --git a/include/kunit/test.h b/include/kunit/test.h > > index dba4830..9fc6c1b 100644 > > --- a/include/kunit/test.h > > +++ b/include/kunit/test.h > > @@ -12,6 +12,7 @@ > > #include <kunit/assert.h> > > #include <kunit/try-catch.h> > > #include <linux/kernel.h> > > +#include <linux/module.h> > > #include <linux/slab.h> > > #include <linux/types.h> > > > > @@ -204,24 +205,39 @@ struct kunit { > > * Registers @suite with the test framework. See &struct kunit_suite for > > * more information. > > * > > - * NOTE: Currently KUnit tests are all run as late_initcalls; this means > > + * When builtin, KUnit tests are all run as late_initcalls; this means > > * that they cannot test anything where tests must run at a different init > > * phase. One significant restriction resulting from this is that KUnit > > * cannot reliably test anything that is initialize in the late_init phase; > > * another is that KUnit is useless to test things that need to be run in > > * an earlier init phase. > > * > > + * An alternative is to build the tests as a module. Because modules > > + * do not support multiple late_initcall()s, we need to initialize an > > + * array of suites for a module. > > + * > > * TODO(brendanhiggins@google.com): Don't run all KUnit tests as > > * late_initcalls. I have some future work planned to dispatch all KUnit > > * tests from the same place, and at the very least to do so after > > * everything else is definitely initialized. > > */ > > -#define kunit_test_suite(suite) \ > > - static int kunit_suite_init##suite(void) \ > > - { \ > > - return kunit_run_tests(&suite); \ > > - } \ > > - late_initcall(kunit_suite_init##suite) > > +#define kunit_test_suites(...) \ > > + static struct kunit_suite *suites[] = { __VA_ARGS__, NULL}; \ > > + static int kunit_test_suites_init(void) \ > > + { \ > > + unsigned int i; \ > > + for (i = 0; suites[i] != NULL; i++) \ > > + kunit_run_tests(suites[i]); \ > > + return 0; \ > > + } \ > > + late_initcall(kunit_test_suites_init); \ > > + static void __exit kunit_test_suites_exit(void) \ > > + { \ > > + return; \ > > + } \ > > + module_exit(kunit_test_suites_exit) > > + > > +#define kunit_test_suite(suite) kunit_test_suites(suite) > > I think it is fine to just rename this kunit_test_suites. > Will do. > > /* > > * Like kunit_alloc_resource() below, but returns the struct kunit_resource > > diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c > > index 2a63241..15161c5 100644 > > --- a/kernel/sysctl-test.c > > +++ b/kernel/sysctl-test.c > > @@ -389,4 +389,8 @@ static void sysctl_test_api_dointvec_write_single_greater_int_max( > > .test_cases = sysctl_test_cases, > > }; > > > > -kunit_test_suite(sysctl_test_suite); > > +kunit_test_suite(&sysctl_test_suite); > > + > > +#ifdef MODULE > > +MODULE_LICENSE("GPL"); > > +#endif /* MODULE */ > > Here and elsewhere: the "ifdef/endif MODULE" should not be necessary. > Will fix, thanks! > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > > index a3017a5..f9f411a6 100644 > > --- a/lib/Kconfig.debug > > +++ b/lib/Kconfig.debug > > @@ -1951,10 +1951,10 @@ config TEST_SYSCTL > > If unsure, say N. > > > > config SYSCTL_KUNIT_TEST > > - bool "KUnit test for sysctl" > > + tristate "KUnit test for sysctl" > > depends on KUNIT > > help > > - This builds the proc sysctl unit test, which runs on boot. > > + This builds the proc sysctl unit test, which runs on boot/module load. > > Tests the API contract and implementation correctness of sysctl. > > For more information on KUnit and unit tests in general please refer > > to the KUnit documentation in Documentation/dev-tools/kunit/. > [...] > > diff --git a/lib/kunit/example-test.c b/lib/kunit/example-test.c > > index f64a829..6c6a408 100644 > > --- a/lib/kunit/example-test.c > > +++ b/lib/kunit/example-test.c > > @@ -85,4 +85,8 @@ static int example_test_init(struct kunit *test) > > * This registers the above test suite telling KUnit that this is a suite of > > * tests that need to be run. > > */ > > -kunit_test_suite(example_test_suite); > > +kunit_test_suite(&example_test_suite); > > + > > +#ifdef MODULE > > +MODULE_LICENSE("GPL"); > > +#endif /* MODULE */ > > nit: The "ifdef/endif MODULE" should not be necessary. > > > diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c > > index 76cc05e..7a3e7a0 100644 > > --- a/lib/kunit/string-stream-test.c > > +++ b/lib/kunit/string-stream-test.c > > @@ -45,8 +45,13 @@ static void string_stream_test_get_string(struct kunit *test) > > {} > > }; > > > > -static struct kunit_suite string_stream_test_suite = { > > +struct kunit_suite string_stream_test_suite = { > > .name = "string-stream-test", > > .test_cases = string_stream_test_cases > > }; > > -kunit_test_suite(string_stream_test_suite); > > + > > +kunit_test_suite(&string_stream_test_suite); > > + > > +#ifdef MODULE > > +MODULE_LICENSE("GPL"); > > +#endif /* MODULE */ > > diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c > > index e6d17aa..e4f3a97 100644 > > --- a/lib/kunit/string-stream.c > > +++ b/lib/kunit/string-stream.c > > @@ -100,6 +100,7 @@ int string_stream_vadd(struct string_stream *stream, > > > > return 0; > > } > > +EXPORT_SYMBOL_GPL(string_stream_vadd); > > Is this actually needed by anything other than lib/kunit/test.c right > now? Maybe we should move the include file into the kunit/ directory to > hide these so no one else can use them. > I tried this, and it's the right answer I think but it exposes a problem with symbol visibility when kunit is compiled as a module. More on this below... > > int string_stream_add(struct string_stream *stream, const char *fmt, ...) > > { > > @@ -112,6 +113,7 @@ int string_stream_add(struct string_stream *stream, const char *fmt, ...) > > > > return result; > > } > > +EXPORT_SYMBOL_GPL(string_stream_add); > [...] > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > > index c83c0fa..e7896f1 100644 > > --- a/lib/kunit/test.c > > +++ b/lib/kunit/test.c > [...] > > @@ -50,6 +51,7 @@ static unsigned long kunit_test_timeout(void) > > * For more background on this topic, see: > > * https://mike-bland.com/2011/11/01/small-medium-large.html > > */ > > +#ifndef MODULE > > Why is this block of code "ifndef MODULE"? > Symbol visibility is the problem again; sysctl_hung_task_timeout_secs isn't exported so when kunit is a module it can't find the symbol. I think I saw Kees mentioned something about symbol lookup too; in KTF Knut solved this by defining ktf_find_symbol(). I'd suggest we may need a kunit_find_symbol() with a function signature void *kunit_find_symbol(const char *modname, const char *symbol_name); ...which does a [module_]kallsyms_lookup_sym(). If the above makes sense I can look at adding it as a patch (and adding a test of it of course!). What do you think? > > if (sysctl_hung_task_timeout_secs) { > > /* > > * If sysctl_hung_task is active, just set the timeout to some > > @@ -60,9 +62,9 @@ static unsigned long kunit_test_timeout(void) > > */ > > timeout_msecs = (sysctl_hung_task_timeout_secs - 1) * > > MSEC_PER_SEC; > > - } else { > > + } else > > +#endif > > timeout_msecs = 300 * MSEC_PER_SEC; /* 5 min */ > > - } > > > > return timeout_msecs; > > } > > @@ -106,6 +108,7 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context) > > > > try_catch->catch(try_catch->context); > > } > > +EXPORT_SYMBOL_GPL(kunit_try_catch_run); > > > > void kunit_try_catch_init(struct kunit_try_catch *try_catch, > > struct kunit *test, > > @@ -116,3 +119,4 @@ void kunit_try_catch_init(struct kunit_try_catch *try_catch, > > try_catch->try = try; > > try_catch->catch = catch; > > } > > +EXPORT_SYMBOL_GPL(kunit_try_catch_init); > > This code should also probably be hidden from outside of kunit/. > Sure, will do. Thanks again for the review! Alan
Sorry for the delayed reply. I will be on vacation until Wednesday, October 16th. On Wed, Oct 9, 2019 at 9:36 AM Alan Maguire <alan.maguire@oracle.com> wrote: > > On Tue, 8 Oct 2019, Brendan Higgins wrote: > > > On Tue, Oct 08, 2019 at 03:55:44PM +0100, Alan Maguire wrote: [...] > > > diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c > > > index e6d17aa..e4f3a97 100644 > > > --- a/lib/kunit/string-stream.c > > > +++ b/lib/kunit/string-stream.c > > > @@ -100,6 +100,7 @@ int string_stream_vadd(struct string_stream *stream, > > > > > > return 0; > > > } > > > +EXPORT_SYMBOL_GPL(string_stream_vadd); > > > > Is this actually needed by anything other than lib/kunit/test.c right > > now? Maybe we should move the include file into the kunit/ directory to > > hide these so no one else can use them. > > > > I tried this, and it's the right answer I think but it exposes > a problem with symbol visibility when kunit is compiled as a module. > More on this below... > > > > int string_stream_add(struct string_stream *stream, const char *fmt, ...) > > > { > > > @@ -112,6 +113,7 @@ int string_stream_add(struct string_stream *stream, const char *fmt, ...) > > > > > > return result; > > > } > > > +EXPORT_SYMBOL_GPL(string_stream_add); > > [...] > > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > > > index c83c0fa..e7896f1 100644 > > > --- a/lib/kunit/test.c > > > +++ b/lib/kunit/test.c > > [...] > > > @@ -50,6 +51,7 @@ static unsigned long kunit_test_timeout(void) > > > * For more background on this topic, see: > > > * https://mike-bland.com/2011/11/01/small-medium-large.html > > > */ > > > +#ifndef MODULE > > > > Why is this block of code "ifndef MODULE"? > > > > Symbol visibility is the problem again; sysctl_hung_task_timeout_secs > isn't exported so when kunit is a module it can't find the symbol. > > I think I saw Kees mentioned something about symbol lookup too; in KTF > Knut solved this by defining ktf_find_symbol(). I'd suggest we may need a > kunit_find_symbol() with a function signature I thought we were just talking about exposing symbols for linking outside of a compilation unit (static vs. not static); nevertheless, I think you are right that it is relevant here. Kees, thoughts? > void *kunit_find_symbol(const char *modname, const char *symbol_name); > > ...which does a [module_]kallsyms_lookup_sym(). > > If the above makes sense I can look at adding it as a patch (and adding > a test of it of course!). What do you think? So that won't work if you are trying to link against a symbol not in a module, right? Also, it won't work against a static symbol, right? Even so, I think it is pretty wonky to expect users to either a) export any symbol name to be tested, or b) have to access them via kunit_find_symbol. I think it is fine to have some tests that cannot be compiled as modules, if there is no other user friendly way to make this work in those cases. Thoughts?
On Fri, 11 Oct 2019, Brendan Higgins wrote: > Sorry for the delayed reply. I will be on vacation until Wednesday, > October 16th. > > On Wed, Oct 9, 2019 at 9:36 AM Alan Maguire <alan.maguire@oracle.com> wrote: > > > > On Tue, 8 Oct 2019, Brendan Higgins wrote: > > > > > On Tue, Oct 08, 2019 at 03:55:44PM +0100, Alan Maguire wrote: > [...] > > > > diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c > > > > index e6d17aa..e4f3a97 100644 > > > > --- a/lib/kunit/string-stream.c > > > > +++ b/lib/kunit/string-stream.c > > > > @@ -100,6 +100,7 @@ int string_stream_vadd(struct string_stream *stream, > > > > > > > > return 0; > > > > } > > > > +EXPORT_SYMBOL_GPL(string_stream_vadd); > > > > > > Is this actually needed by anything other than lib/kunit/test.c right > > > now? Maybe we should move the include file into the kunit/ directory to > > > hide these so no one else can use them. > > > > > > > I tried this, and it's the right answer I think but it exposes > > a problem with symbol visibility when kunit is compiled as a module. > > More on this below... > > > > > > int string_stream_add(struct string_stream *stream, const char *fmt, ...) > > > > { > > > > @@ -112,6 +113,7 @@ int string_stream_add(struct string_stream *stream, const char *fmt, ...) > > > > > > > > return result; > > > > } > > > > +EXPORT_SYMBOL_GPL(string_stream_add); > > > [...] > > > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > > > > index c83c0fa..e7896f1 100644 > > > > --- a/lib/kunit/test.c > > > > +++ b/lib/kunit/test.c > > > [...] > > > > @@ -50,6 +51,7 @@ static unsigned long kunit_test_timeout(void) > > > > * For more background on this topic, see: > > > > * https://mike-bland.com/2011/11/01/small-medium-large.html > > > > */ > > > > +#ifndef MODULE > > > > > > Why is this block of code "ifndef MODULE"? > > > > > > > Symbol visibility is the problem again; sysctl_hung_task_timeout_secs > > isn't exported so when kunit is a module it can't find the symbol. > > > > I think I saw Kees mentioned something about symbol lookup too; in KTF > > Knut solved this by defining ktf_find_symbol(). I'd suggest we may need a > > kunit_find_symbol() with a function signature > > I thought we were just talking about exposing symbols for linking > outside of a compilation unit (static vs. not static); nevertheless, I > think you are right that it is relevant here. Kees, thoughts? > > > void *kunit_find_symbol(const char *modname, const char *symbol_name); > > > > ...which does a [module_]kallsyms_lookup_sym(). > > > > If the above makes sense I can look at adding it as a patch (and adding > > a test of it of course!). What do you think? > > So that won't work if you are trying to link against a symbol not in a > module, right? Also, it won't work against a static symbol, right? > Nope, works in both cases with the proviso that we need to use an alternative name for symbols when compiling built-in. For example in the case of the string-stream tests, we'd use a test init callback to initialize used symbols: static int string_stream_test_init(struct kunit *test) { _alloc_string_stream = kunit_find_symbol("alloc_string_stream"); _string_stream_add = kunit_find_symbol("string_stream_add"); _string_stream_get_string = kunit_find_symbol("string_stream_get_string"); _string_stream_is_empty = kunit_find_symbol("string_stream_is_empty"); if (IS_ERR(_alloc_string_stream) || IS_ERR(_string_stream_add) || IS_ERR(_string_stream_get_string) || IS_ERR(_string_stream_is_empty)) return EINVAL; return 0; } I've tested this when string-stream-test is compiled built-in and as a module. We can of course create a wrapper macro to handle these assignments. To illustrate further here's the test cases I'd propose adding to test-test.c with the changes. In the first case we're grabbing the "modules" variable from the kernel, and in the second we're grabbing a static symbol from the test-test.ko module (when it is compiled as a module): /* * Find non-exported kernel symbol; we use the modules list as a safe * choice that should always be present. */ static void kunit_find_symbol_kernel(struct kunit *test) { KUNIT_ASSERT_NOT_ERR_OR_NULL(test, kunit_find_symbol("modules")); } #ifdef MODULE /* * If we are compiled as a module, use this module for lookup. */ static void kunit_find_symbol_module(struct kunit *test) { KUNIT_ASSERT_NOT_ERR_OR_NULL(test, kunit_find_symbol("kunit_find_symbol_kernel")); } #endif > Even so, I think it is pretty wonky to expect users to either a) > export any symbol name to be tested, Absolutely not, I'd never advocate that. Nothing should need to change in the component under test simply to facilitate testing, especially if there's a way the test framework can work around it. > or b) have to access them via > kunit_find_symbol. I think it is fine to have some tests that cannot > be compiled as modules, if there is no other user friendly way to make > this work in those cases. That's fine, and I agree in some cases it's unworkable, but there are going to be a lot of tristate componenets we'd like to test, and restricting testing of those by requiring CONFIG_FOO=y seems like a limitation too. In practice I've found symbol lookup isn't needed extensively for test development. For cases where the weight of symbol lookup is too heavy the tests can simply stay built-in - the non-exported nature of the symbols is probably suggesting something about the nature of the interface that makes that a more natural choice anyway. However for other cases I think there's value to having something like this feature. Of course there may be better ways to realize the functionality than what I'm proposing. Thanks! Alan
On Fri, Oct 11, 2019 at 11:25:33AM +0100, Alan Maguire wrote: > On Fri, 11 Oct 2019, Brendan Higgins wrote: > > > Sorry for the delayed reply. I will be on vacation until Wednesday, > > October 16th. > > > > On Wed, Oct 9, 2019 at 9:36 AM Alan Maguire <alan.maguire@oracle.com> wrote: > > > > > > On Tue, 8 Oct 2019, Brendan Higgins wrote: > > > > > > > On Tue, Oct 08, 2019 at 03:55:44PM +0100, Alan Maguire wrote: > > [...] > > > > > diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c > > > > > index e6d17aa..e4f3a97 100644 > > > > > --- a/lib/kunit/string-stream.c > > > > > +++ b/lib/kunit/string-stream.c > > > > > @@ -100,6 +100,7 @@ int string_stream_vadd(struct string_stream *stream, > > > > > > > > > > return 0; > > > > > } > > > > > +EXPORT_SYMBOL_GPL(string_stream_vadd); > > > > > > > > Is this actually needed by anything other than lib/kunit/test.c right > > > > now? Maybe we should move the include file into the kunit/ directory to > > > > hide these so no one else can use them. > > > > > > > > > > I tried this, and it's the right answer I think but it exposes > > > a problem with symbol visibility when kunit is compiled as a module. > > > More on this below... > > > > > > > > int string_stream_add(struct string_stream *stream, const char *fmt, ...) > > > > > { > > > > > @@ -112,6 +113,7 @@ int string_stream_add(struct string_stream *stream, const char *fmt, ...) > > > > > > > > > > return result; > > > > > } > > > > > +EXPORT_SYMBOL_GPL(string_stream_add); > > > > [...] > > > > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > > > > > index c83c0fa..e7896f1 100644 > > > > > --- a/lib/kunit/test.c > > > > > +++ b/lib/kunit/test.c > > > > [...] > > > > > @@ -50,6 +51,7 @@ static unsigned long kunit_test_timeout(void) > > > > > * For more background on this topic, see: > > > > > * https://mike-bland.com/2011/11/01/small-medium-large.html > > > > > */ > > > > > +#ifndef MODULE > > > > > > > > Why is this block of code "ifndef MODULE"? > > > > > > > > > > Symbol visibility is the problem again; sysctl_hung_task_timeout_secs > > > isn't exported so when kunit is a module it can't find the symbol. > > > > > > I think I saw Kees mentioned something about symbol lookup too; in KTF > > > Knut solved this by defining ktf_find_symbol(). I'd suggest we may need a > > > kunit_find_symbol() with a function signature Based on what you said below, I think the kunit_find_symbol() may have value for writing tests; however, I do not think it is the right way to handle resources needed by test.c. I think exporting the symbols in this case is the lesser of the two evils. I am still suprised that you need to export a symbol that is getting compiled into and is only used by the kunit module. In fact, I think I found an example in the kernel where someone else managed this. Checkout stp_policy_node_priv(). Looks like the symbol is used here[1] and is defined here[2]. You can see here[3] and here[4] that the files end up in the same module. Do you mind taking a look why it works for stm, but not here? > > I thought we were just talking about exposing symbols for linking > > outside of a compilation unit (static vs. not static); nevertheless, I > > think you are right that it is relevant here. Kees, thoughts? > > > > > void *kunit_find_symbol(const char *modname, const char *symbol_name); > > > > > > ...which does a [module_]kallsyms_lookup_sym(). > > > > > > If the above makes sense I can look at adding it as a patch (and adding > > > a test of it of course!). What do you think? > > > > So that won't work if you are trying to link against a symbol not in a > > module, right? Also, it won't work against a static symbol, right? > > > > Nope, works in both cases with the proviso that we need to use an Nifty! That sounds great! > alternative name for symbols when compiling built-in. For example Can you elaborate on "need[ing] to use an alternative name"? > in the case of the string-stream tests, we'd use a test init callback > to initialize used symbols: > > static int string_stream_test_init(struct kunit *test) > { > _alloc_string_stream = kunit_find_symbol("alloc_string_stream"); > _string_stream_add = kunit_find_symbol("string_stream_add"); > _string_stream_get_string = kunit_find_symbol("string_stream_get_string"); > _string_stream_is_empty = kunit_find_symbol("string_stream_is_empty"); > if (IS_ERR(_alloc_string_stream) || > IS_ERR(_string_stream_add) || > IS_ERR(_string_stream_get_string) || > IS_ERR(_string_stream_is_empty)) > return EINVAL; > return 0; > } > > I've tested this when string-stream-test is compiled built-in and as a > module. We can of course create a wrapper macro to handle these > assignments. I've got mixed feelings on this. On one hand, that has the potential to solve a lot of problems with visibility and modules in a way that doesn't immediately cause code under test to change in undesirable ways. On the other hand, I feel that this has the potential to be really prone to breakage. It would be much nicer if the compiler could tell you that your symbol changed rather than having to wait until you run the test. Just having the test tell you that a symbol doesn't exist anymore would be mildly annoying, but having the signature of the symbol change could get downright frustrating using this method. > To illustrate further here's the test cases I'd propose adding to > test-test.c with the changes. > > In the first case we're grabbing the "modules" variable from the kernel, > and in the second we're grabbing a static symbol from the test-test.ko > module (when it is compiled as a module): > > /* > * Find non-exported kernel symbol; we use the modules list as a safe > * choice that should always be present. > */ > static void kunit_find_symbol_kernel(struct kunit *test) > { > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, kunit_find_symbol("modules")); > } > > #ifdef MODULE > /* > * If we are compiled as a module, use this module for lookup. > */ > static void kunit_find_symbol_module(struct kunit *test) > { > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, > kunit_find_symbol("kunit_find_symbol_kernel")); > } > #endif > > > > Even so, I think it is pretty wonky to expect users to either a) > > export any symbol name to be tested, > > Absolutely not, I'd never advocate that. Nothing should need to change in Cool. Looks like we are on the same page then :-) > the component under test simply to facilitate testing, especially if > there's a way the test framework can work around it. I would say it depends. I think it is fine to ask people to write code in such a manner that makes it more testable. From my experience, this generally leads to better quality code. Nevertheless, I agree from the perspective that suddenly having to export symbol names for no good reason is not something we should make users do. > > or b) have to access them via > > kunit_find_symbol. I think it is fine to have some tests that cannot > > be compiled as modules, if there is no other user friendly way to make > > this work in those cases. > > That's fine, and I agree in some cases it's unworkable, but there are > going to be a lot of tristate componenets we'd like to test, and > restricting testing of those by requiring CONFIG_FOO=y seems like a > limitation too. In practice I've found symbol lookup isn't needed > extensively for test development. For cases where the weight of symbol > lookup is too heavy the tests can simply stay built-in - the non-exported > nature of the symbols is probably suggesting something about the nature of > the interface that makes that a more natural choice anyway. However for > other cases I think there's value to having something like this feature. I think that makes sense. So, I don't think the symbol lookup is needed for this patchset. I think all the symbols that you use should *probably* be exported at worse since they are used by the core kunit library stuff. Nevertheless, I have a test coming down the pipeline for which this could be a potential solution. I will CC you on the test when I send it. > Of course there may be better ways to realize the functionality than what > I'm proposing. Cheers! [1] https://elixir.bootlin.com/linux/v5.3.6/source/drivers/hwtracing/stm/core.c#L322 [2] https://elixir.bootlin.com/linux/v5.3.6/source/drivers/hwtracing/stm/policy.c#L40 [3] https://elixir.bootlin.com/linux/v5.3.6/source/drivers/hwtracing/stm/Makefile#L4 [4] https://elixir.bootlin.com/linux/v5.3.6/source/drivers/hwtracing/stm/Kconfig#L3
On Wed, 16 Oct 2019, Brendan Higgins wrote: > On Fri, Oct 11, 2019 at 11:25:33AM +0100, Alan Maguire wrote: > > On Fri, 11 Oct 2019, Brendan Higgins wrote: > > > > > Sorry for the delayed reply. I will be on vacation until Wednesday, > > > October 16th. > > > > > > On Wed, Oct 9, 2019 at 9:36 AM Alan Maguire <alan.maguire@oracle.com> wrote: > > > > > > > > On Tue, 8 Oct 2019, Brendan Higgins wrote: > > > > > > > > > On Tue, Oct 08, 2019 at 03:55:44PM +0100, Alan Maguire wrote: > > > [...] > > > > > > diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c > > > > > > index e6d17aa..e4f3a97 100644 > > > > > > --- a/lib/kunit/string-stream.c > > > > > > +++ b/lib/kunit/string-stream.c > > > > > > @@ -100,6 +100,7 @@ int string_stream_vadd(struct string_stream *stream, > > > > > > > > > > > > return 0; > > > > > > } > > > > > > +EXPORT_SYMBOL_GPL(string_stream_vadd); > > > > > > > > > > Is this actually needed by anything other than lib/kunit/test.c right > > > > > now? Maybe we should move the include file into the kunit/ directory to > > > > > hide these so no one else can use them. > > > > > > > > > > > > > I tried this, and it's the right answer I think but it exposes > > > > a problem with symbol visibility when kunit is compiled as a module. > > > > More on this below... > > > > > > > > > > int string_stream_add(struct string_stream *stream, const char *fmt, ...) > > > > > > { > > > > > > @@ -112,6 +113,7 @@ int string_stream_add(struct string_stream *stream, const char *fmt, ...) > > > > > > > > > > > > return result; > > > > > > } > > > > > > +EXPORT_SYMBOL_GPL(string_stream_add); > > > > > [...] > > > > > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > > > > > > index c83c0fa..e7896f1 100644 > > > > > > --- a/lib/kunit/test.c > > > > > > +++ b/lib/kunit/test.c > > > > > [...] > > > > > > @@ -50,6 +51,7 @@ static unsigned long kunit_test_timeout(void) > > > > > > * For more background on this topic, see: > > > > > > * https://mike-bland.com/2011/11/01/small-medium-large.html > > > > > > */ > > > > > > +#ifndef MODULE > > > > > > > > > > Why is this block of code "ifndef MODULE"? > > > > > > > > > > > > > Symbol visibility is the problem again; sysctl_hung_task_timeout_secs > > > > isn't exported so when kunit is a module it can't find the symbol. > > > > > > > > I think I saw Kees mentioned something about symbol lookup too; in KTF > > > > Knut solved this by defining ktf_find_symbol(). I'd suggest we may need a > > > > kunit_find_symbol() with a function signature > > Based on what you said below, I think the kunit_find_symbol() may have > value for writing tests; however, I do not think it is the right way to > handle resources needed by test.c. I think exporting the symbols in this > case is the lesser of the two evils. > The only symbol we need in core kunit from the kernel when compiling kunit as a module is sysctl_hung_task_timeout_secs; it's a core kernel symbol. There's no issue with symbols within kunit in this case. I've come up with a new way to handle variables and functions in the v3 patch set 3 [1] I've sent out. While not being perfect, it attempts to satisfy some of the requirements you describe below. It will generate compiler errors if there is a mismatch between local symbol definition and the target symbol type. Symbol variable definitions are handled such that the same symbol name can be used; see try-catch.c in patch 5 where we assign sysctl_hung_task_timeout_secs. Unfortunately the same scheme won't work for functions. The reason for this is we've already #included a definiton of the function, so if we attempt to redefine that same name as a function pointer we get a compile-time that we are redefining the symbol. As a consequence the approach I took is for us to define a local function pointer and it gets assigned either to - the results of kunit_find_symbol() (module case) - the function itself (builtin case) The latter will trigger a compile-time error if our local definition is out of sync. > I am still suprised that you need to export a symbol that is getting > compiled into and is only used by the kunit module. see above - kunit needs a non-exported global kernel symbol (sysctl_hung_task_timeout_secs). > In fact, I think I > found an example in the kernel where someone else managed this. Checkout > stp_policy_node_priv(). Looks like the symbol is used here[1] and is > defined here[2]. You can see here[3] and here[4] that the files end up > in the same module. Do you mind taking a look why it works for stm, but > not here? > > > > I thought we were just talking about exposing symbols for linking > > > outside of a compilation unit (static vs. not static); nevertheless, I > > > think you are right that it is relevant here. Kees, thoughts? > > > > > > > void *kunit_find_symbol(const char *modname, const char *symbol_name); > > > > > > > > ...which does a [module_]kallsyms_lookup_sym(). > > > > > > > > If the above makes sense I can look at adding it as a patch (and adding > > > > a test of it of course!). What do you think? > > > > > > So that won't work if you are trying to link against a symbol not in a > > > module, right? Also, it won't work against a static symbol, right? > > > > > > > Nope, works in both cases with the proviso that we need to use an > > Nifty! That sounds great! > > > alternative name for symbols when compiling built-in. For example > > Can you elaborate on "need[ing] to use an alternative name"? > See above and patch 4 in the v3 patchset. > > in the case of the string-stream tests, we'd use a test init callback > > to initialize used symbols: > > > > static int string_stream_test_init(struct kunit *test) > > { > > _alloc_string_stream = kunit_find_symbol("alloc_string_stream"); > > _string_stream_add = kunit_find_symbol("string_stream_add"); > > _string_stream_get_string = kunit_find_symbol("string_stream_get_string"); > > _string_stream_is_empty = kunit_find_symbol("string_stream_is_empty"); > > if (IS_ERR(_alloc_string_stream) || > > IS_ERR(_string_stream_add) || > > IS_ERR(_string_stream_get_string) || > > IS_ERR(_string_stream_is_empty)) > > return EINVAL; > > return 0; > > } > > > > I've tested this when string-stream-test is compiled built-in and as a > > module. We can of course create a wrapper macro to handle these > > assignments. > > I've got mixed feelings on this. On one hand, that has the potential to > solve a lot of problems with visibility and modules in a way that > doesn't immediately cause code under test to change in undesirable ways. > On the other hand, I feel that this has the potential to be really prone > to breakage. It would be much nicer if the compiler could tell you that > your symbol changed rather than having to wait until you run the test. > Just having the test tell you that a symbol doesn't exist anymore would > be mildly annoying, but having the signature of the symbol change could > get downright frustrating using this method. > See above; if compiled as builtin compiler errors will be generated. Thanks! Alan [1] https://lkml.org/lkml/2019/10/17/801
On Thu, Oct 17, 2019 at 07:32:18PM +0100, Alan Maguire wrote: > kunit needs a non-exported global kernel symbol > (sysctl_hung_task_timeout_secs). Sounds like a perfect use case for the new symbol namespaces [0]. We wouldn't want random drivers importing this namespace, but for kunit it would seem reasonable. [0] https://lwn.net/Articles/798254/ Luis
On Fri, Oct 18, 2019 at 5:21 AM Luis Chamberlain <mcgrof@kernel.org> wrote: > > On Thu, Oct 17, 2019 at 07:32:18PM +0100, Alan Maguire wrote: > > kunit needs a non-exported global kernel symbol > > (sysctl_hung_task_timeout_secs). > > Sounds like a perfect use case for the new symbol namespaces [0]. We > wouldn't want random drivers importing this namespace, but for kunit it > would seem reasonable. > > [0] https://lwn.net/Articles/798254/ Sounds good to me.
diff --git a/include/kunit/test.h b/include/kunit/test.h index dba4830..9fc6c1b 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -12,6 +12,7 @@ #include <kunit/assert.h> #include <kunit/try-catch.h> #include <linux/kernel.h> +#include <linux/module.h> #include <linux/slab.h> #include <linux/types.h> @@ -204,24 +205,39 @@ struct kunit { * Registers @suite with the test framework. See &struct kunit_suite for * more information. * - * NOTE: Currently KUnit tests are all run as late_initcalls; this means + * When builtin, KUnit tests are all run as late_initcalls; this means * that they cannot test anything where tests must run at a different init * phase. One significant restriction resulting from this is that KUnit * cannot reliably test anything that is initialize in the late_init phase; * another is that KUnit is useless to test things that need to be run in * an earlier init phase. * + * An alternative is to build the tests as a module. Because modules + * do not support multiple late_initcall()s, we need to initialize an + * array of suites for a module. + * * TODO(brendanhiggins@google.com): Don't run all KUnit tests as * late_initcalls. I have some future work planned to dispatch all KUnit * tests from the same place, and at the very least to do so after * everything else is definitely initialized. */ -#define kunit_test_suite(suite) \ - static int kunit_suite_init##suite(void) \ - { \ - return kunit_run_tests(&suite); \ - } \ - late_initcall(kunit_suite_init##suite) +#define kunit_test_suites(...) \ + static struct kunit_suite *suites[] = { __VA_ARGS__, NULL}; \ + static int kunit_test_suites_init(void) \ + { \ + unsigned int i; \ + for (i = 0; suites[i] != NULL; i++) \ + kunit_run_tests(suites[i]); \ + return 0; \ + } \ + late_initcall(kunit_test_suites_init); \ + static void __exit kunit_test_suites_exit(void) \ + { \ + return; \ + } \ + module_exit(kunit_test_suites_exit) + +#define kunit_test_suite(suite) kunit_test_suites(suite) /* * Like kunit_alloc_resource() below, but returns the struct kunit_resource diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c index 2a63241..15161c5 100644 --- a/kernel/sysctl-test.c +++ b/kernel/sysctl-test.c @@ -389,4 +389,8 @@ static void sysctl_test_api_dointvec_write_single_greater_int_max( .test_cases = sysctl_test_cases, }; -kunit_test_suite(sysctl_test_suite); +kunit_test_suite(&sysctl_test_suite); + +#ifdef MODULE +MODULE_LICENSE("GPL"); +#endif /* MODULE */ diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index a3017a5..f9f411a6 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -1951,10 +1951,10 @@ config TEST_SYSCTL If unsure, say N. config SYSCTL_KUNIT_TEST - bool "KUnit test for sysctl" + tristate "KUnit test for sysctl" depends on KUNIT help - This builds the proc sysctl unit test, which runs on boot. + This builds the proc sysctl unit test, which runs on boot/module load. Tests the API contract and implementation correctness of sysctl. For more information on KUnit and unit tests in general please refer to the KUnit documentation in Documentation/dev-tools/kunit/. diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig index af37016..9ebd5e6 100644 --- a/lib/kunit/Kconfig +++ b/lib/kunit/Kconfig @@ -15,7 +15,7 @@ menuconfig KUNIT if KUNIT config KUNIT_TEST - bool "KUnit test for KUnit" + tristate "KUnit test for KUnit" help Enables the unit tests for the KUnit test framework. These tests test the KUnit test framework itself; the tests are both written using @@ -24,7 +24,7 @@ config KUNIT_TEST expected. config KUNIT_EXAMPLE_TEST - bool "Example test for KUnit" + tristate "Example test for KUnit" help Enables an example unit test that illustrates some of the basic features of KUnit. This test only exists to help new users understand diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c index 86013d4..92eba2a 100644 --- a/lib/kunit/assert.c +++ b/lib/kunit/assert.c @@ -24,6 +24,7 @@ void kunit_base_assert_format(const struct kunit_assert *assert, string_stream_add(stream, "%s FAILED at %s:%d\n", expect_or_assert, assert->file, assert->line); } +EXPORT_SYMBOL_GPL(kunit_base_assert_format); void kunit_assert_print_msg(const struct kunit_assert *assert, struct string_stream *stream) @@ -31,6 +32,7 @@ void kunit_assert_print_msg(const struct kunit_assert *assert, if (assert->message.fmt) string_stream_add(stream, "\n%pV", &assert->message); } +EXPORT_SYMBOL_GPL(kunit_assert_print_msg); void kunit_fail_assert_format(const struct kunit_assert *assert, struct string_stream *stream) @@ -38,6 +40,7 @@ void kunit_fail_assert_format(const struct kunit_assert *assert, kunit_base_assert_format(assert, stream); string_stream_add(stream, "%pV", &assert->message); } +EXPORT_SYMBOL_GPL(kunit_fail_assert_format); void kunit_unary_assert_format(const struct kunit_assert *assert, struct string_stream *stream) @@ -56,6 +59,7 @@ void kunit_unary_assert_format(const struct kunit_assert *assert, unary_assert->condition); kunit_assert_print_msg(assert, stream); } +EXPORT_SYMBOL_GPL(kunit_unary_assert_format); void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert, struct string_stream *stream) @@ -76,6 +80,7 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert, } kunit_assert_print_msg(assert, stream); } +EXPORT_SYMBOL_GPL(kunit_ptr_not_err_assert_format); void kunit_binary_assert_format(const struct kunit_assert *assert, struct string_stream *stream) @@ -97,6 +102,7 @@ void kunit_binary_assert_format(const struct kunit_assert *assert, binary_assert->right_value); kunit_assert_print_msg(assert, stream); } +EXPORT_SYMBOL_GPL(kunit_binary_assert_format); void kunit_binary_ptr_assert_format(const struct kunit_assert *assert, struct string_stream *stream) @@ -118,6 +124,7 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert, binary_assert->right_value); kunit_assert_print_msg(assert, stream); } +EXPORT_SYMBOL_GPL(kunit_binary_ptr_assert_format); void kunit_binary_str_assert_format(const struct kunit_assert *assert, struct string_stream *stream) @@ -139,3 +146,4 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert, binary_assert->right_value); kunit_assert_print_msg(assert, stream); } +EXPORT_SYMBOL_GPL(kunit_binary_str_assert_format); diff --git a/lib/kunit/example-test.c b/lib/kunit/example-test.c index f64a829..6c6a408 100644 --- a/lib/kunit/example-test.c +++ b/lib/kunit/example-test.c @@ -85,4 +85,8 @@ static int example_test_init(struct kunit *test) * This registers the above test suite telling KUnit that this is a suite of * tests that need to be run. */ -kunit_test_suite(example_test_suite); +kunit_test_suite(&example_test_suite); + +#ifdef MODULE +MODULE_LICENSE("GPL"); +#endif /* MODULE */ diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c index 76cc05e..7a3e7a0 100644 --- a/lib/kunit/string-stream-test.c +++ b/lib/kunit/string-stream-test.c @@ -45,8 +45,13 @@ static void string_stream_test_get_string(struct kunit *test) {} }; -static struct kunit_suite string_stream_test_suite = { +struct kunit_suite string_stream_test_suite = { .name = "string-stream-test", .test_cases = string_stream_test_cases }; -kunit_test_suite(string_stream_test_suite); + +kunit_test_suite(&string_stream_test_suite); + +#ifdef MODULE +MODULE_LICENSE("GPL"); +#endif /* MODULE */ diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c index e6d17aa..e4f3a97 100644 --- a/lib/kunit/string-stream.c +++ b/lib/kunit/string-stream.c @@ -100,6 +100,7 @@ int string_stream_vadd(struct string_stream *stream, return 0; } +EXPORT_SYMBOL_GPL(string_stream_vadd); int string_stream_add(struct string_stream *stream, const char *fmt, ...) { @@ -112,6 +113,7 @@ int string_stream_add(struct string_stream *stream, const char *fmt, ...) return result; } +EXPORT_SYMBOL_GPL(string_stream_add); static void string_stream_clear(struct string_stream *stream) { @@ -145,6 +147,7 @@ char *string_stream_get_string(struct string_stream *stream) return buf; } +EXPORT_SYMBOL_GPL(string_stream_get_string); int string_stream_append(struct string_stream *stream, struct string_stream *other) @@ -158,11 +161,13 @@ int string_stream_append(struct string_stream *stream, return string_stream_add(stream, other_content); } +EXPORT_SYMBOL_GPL(string_stream_append); bool string_stream_is_empty(struct string_stream *stream) { return list_empty(&stream->fragments); } +EXPORT_SYMBOL_GPL(string_stream_is_empty); struct string_stream_alloc_context { struct kunit *test; @@ -207,6 +212,7 @@ struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp) gfp, &context); } +EXPORT_SYMBOL_GPL(alloc_string_stream); int string_stream_destroy(struct string_stream *stream) { @@ -215,3 +221,4 @@ int string_stream_destroy(struct string_stream *stream) string_stream_free, stream); } +EXPORT_SYMBOL_GPL(string_stream_destroy); diff --git a/lib/kunit/test-test.c b/lib/kunit/test-test.c index 5ebe059..b5fdbe3 100644 --- a/lib/kunit/test-test.c +++ b/lib/kunit/test-test.c @@ -100,7 +100,6 @@ static int kunit_try_catch_test_init(struct kunit *test) .init = kunit_try_catch_test_init, .test_cases = kunit_try_catch_test_cases, }; -kunit_test_suite(kunit_try_catch_test_suite); /* * Context for testing test managed resources @@ -328,4 +327,9 @@ static void kunit_resource_test_exit(struct kunit *test) .exit = kunit_resource_test_exit, .test_cases = kunit_resource_test_cases, }; -kunit_test_suite(kunit_resource_test_suite); +kunit_test_suites(&kunit_resource_test_suite, + &kunit_try_catch_test_suite); + +#ifdef MODULE +MODULE_LICENSE("GPL"); +#endif /* MODULE */ diff --git a/lib/kunit/test.c b/lib/kunit/test.c index c83c0fa..e7896f1 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -171,6 +171,7 @@ void kunit_do_assertion(struct kunit *test, if (assert->type == KUNIT_ASSERTION) kunit_abort(test); } +EXPORT_SYMBOL_GPL(kunit_do_assertion); void kunit_init_test(struct kunit *test, const char *name) { @@ -179,6 +180,7 @@ void kunit_init_test(struct kunit *test, const char *name) test->name = name; test->success = true; } +EXPORT_SYMBOL_GPL(kunit_init_test); /* * Initializes and runs test case. Does not clean up or do post validations. @@ -317,6 +319,7 @@ int kunit_run_tests(struct kunit_suite *suite) return 0; } +EXPORT_SYMBOL_GPL(kunit_run_tests); struct kunit_resource *kunit_alloc_and_get_resource(struct kunit *test, kunit_resource_init_t init, @@ -342,6 +345,7 @@ struct kunit_resource *kunit_alloc_and_get_resource(struct kunit *test, return res; } +EXPORT_SYMBOL_GPL(kunit_alloc_and_get_resource); static void kunit_resource_free(struct kunit *test, struct kunit_resource *res) { @@ -400,6 +404,7 @@ int kunit_resource_destroy(struct kunit *test, kunit_resource_free(test, resource); return 0; } +EXPORT_SYMBOL_GPL(kunit_resource_destroy); struct kunit_kmalloc_params { size_t size; @@ -435,6 +440,7 @@ void *kunit_kmalloc(struct kunit *test, size_t size, gfp_t gfp) gfp, ¶ms); } +EXPORT_SYMBOL_GPL(kunit_kmalloc); void kunit_kfree(struct kunit *test, const void *ptr) { @@ -447,6 +453,7 @@ void kunit_kfree(struct kunit *test, const void *ptr) WARN_ON(rc); } +EXPORT_SYMBOL_GPL(kunit_kfree); void kunit_cleanup(struct kunit *test) { @@ -476,3 +483,4 @@ void kunit_cleanup(struct kunit *test) kunit_resource_free(test, resource); } } +EXPORT_SYMBOL_GPL(kunit_cleanup); diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c index 55686839..a288012 100644 --- a/lib/kunit/try-catch.c +++ b/lib/kunit/try-catch.c @@ -19,6 +19,7 @@ void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch) try_catch->try_result = -EFAULT; complete_and_exit(try_catch->try_completion, -EFAULT); } +EXPORT_SYMBOL_GPL(kunit_try_catch_throw); static int kunit_generic_run_threadfn_adapter(void *data) { @@ -50,6 +51,7 @@ static unsigned long kunit_test_timeout(void) * For more background on this topic, see: * https://mike-bland.com/2011/11/01/small-medium-large.html */ +#ifndef MODULE if (sysctl_hung_task_timeout_secs) { /* * If sysctl_hung_task is active, just set the timeout to some @@ -60,9 +62,9 @@ static unsigned long kunit_test_timeout(void) */ timeout_msecs = (sysctl_hung_task_timeout_secs - 1) * MSEC_PER_SEC; - } else { + } else +#endif timeout_msecs = 300 * MSEC_PER_SEC; /* 5 min */ - } return timeout_msecs; } @@ -106,6 +108,7 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context) try_catch->catch(try_catch->context); } +EXPORT_SYMBOL_GPL(kunit_try_catch_run); void kunit_try_catch_init(struct kunit_try_catch *try_catch, struct kunit *test, @@ -116,3 +119,4 @@ void kunit_try_catch_init(struct kunit_try_catch *try_catch, try_catch->try = try; try_catch->catch = catch; } +EXPORT_SYMBOL_GPL(kunit_try_catch_init);