On 17/08/20 12:36 pm, Rasmus Villemoes wrote: > On 17/08/2020 06.30, Arpitha Raghunandan wrote: >> Converts test lib/test_printf.c to KUnit. >> More information about KUnit can be found at >> https://www.kernel.org/doc/html/latest/dev-tools/kunit/index.html. >> KUnit provides a common framework for unit tests in the kernel. > > So I can continue to build a kernel with some appropriate CONFIG set to > y, boot it under virt-me, run dmesg and see if I broke printf? That's > what I do now, and I don't want to have to start using some enterprisy > framework. > Yes, the test can be run on boot up. More information about this can be found here: https://www.kernel.org/doc/html/latest/dev-tools/kunit/start.html#running-tests-without-the-kunit-wrapper. >> diff --git a/lib/test_printf.c b/lib/printf_kunit.c >> similarity index 45% >> rename from lib/test_printf.c >> rename to lib/printf_kunit.c >> index 7ac87f18a10f..68ac5f9b8d28 100644 >> --- a/lib/test_printf.c >> +++ b/lib/printf_kunit.c >> @@ -5,6 +5,7 @@ >> >> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >> >> +#include <kunit/test.h> >> #include <linux/init.h> >> #include <linux/kernel.h> >> #include <linux/module.h> >> @@ -30,79 +31,61 @@ >> #define PAD_SIZE 16 >> #define FILL_CHAR '$' >> >> -static unsigned total_tests __initdata; >> -static unsigned failed_tests __initdata; >> -static char *test_buffer __initdata; >> -static char *alloced_buffer __initdata; >> +static char *test_buffer; >> +static char *alloced_buffer; >> >> -static int __printf(4, 0) __init >> -do_test(int bufsize, const char *expect, int elen, >> +static void __printf(5, 0) >> +do_test(struct kunit *kunittest, int bufsize, const char *expect, int elen, >> const char *fmt, va_list ap) >> { >> va_list aq; >> int ret, written; >> >> - total_tests++; >> - >> memset(alloced_buffer, FILL_CHAR, BUF_SIZE + 2*PAD_SIZE); >> va_copy(aq, ap); >> ret = vsnprintf(test_buffer, bufsize, fmt, aq); >> va_end(aq); >> >> - if (ret != elen) { >> - pr_warn("vsnprintf(buf, %d, \"%s\", ...) returned %d, expected %d\n", >> + KUNIT_EXPECT_EQ_MSG(kunittest, ret, elen, >> + "vsnprintf(buf, %d, \"%s\", ...) returned %d, expected %d\n", >> bufsize, fmt, ret, elen); >> - return 1; >> - } > > > IIRC, some of these early returns are required to ensure the following > checks do not fail (as in, potentially crash the kernel) simply because > they go off into the weeds. Please double-check that they are all safe > to continue to perform (though, another reason I might have put them in > is to simply avoid lots of useless collateral). > These are safe to perform. I will check once again though. > >> - if (memchr_inv(alloced_buffer, FILL_CHAR, PAD_SIZE)) { >> + KUNIT_EXPECT_EQ_MSG(kunittest, memchr_inv(alloced_buffer, FILL_CHAR, PAD_SIZE), NULL, > >> - if (memchr_inv(test_buffer, FILL_CHAR, BUF_SIZE + PAD_SIZE)) { >> + KUNIT_EXPECT_FALSE_MSG(kunittest, > >> - if (memchr_inv(test_buffer + written + 1, FILL_CHAR, BUF_SIZE + PAD_SIZE - (written + 1))) { >> + KUNIT_EXPECT_FALSE_MSG(kunittest, >> + memchr_inv(test_buffer + written + 1, FILL_CHAR, BUF_SIZE + PAD_SIZE - (written + 1)) > > Why the inconsistency in what a memchr_inv != NULL check gets converted to? > Oh my bad. I will make this consistent. > >> >> -static void __printf(3, 4) __init >> -__test(const char *expect, int elen, const char *fmt, ...) >> +static void __printf(4, 5) >> +__test(struct kunit *kunittest, const char *expect, int elen, const char *fmt, ...) >> { >> va_list ap; >> int rand; >> char *p; >> >> - if (elen >= BUF_SIZE) { >> - pr_err("error in test suite: expected output length %d too long. Format was '%s'.\n", >> - elen, fmt); >> - failed_tests++; >> - return; >> - } >> + KUNIT_EXPECT_LT_MSG(kunittest, elen, BUF_SIZE, >> + "error in test suite: expected output length %d too long. Format was '%s'.\n", >> + elen, fmt); > > And it's ok to continue with the tests when the test suite itself is > buggy because? [*] > >> va_start(ap, fmt); >> >> @@ -112,49 +95,46 @@ __test(const char *expect, int elen, const char *fmt, ...) >> * enough and 0), and then we also test that kvasprintf would >> * be able to print it as expected. >> */ >> - failed_tests += do_test(BUF_SIZE, expect, elen, fmt, ap); >> + do_test(kunittest, BUF_SIZE, expect, elen, fmt, ap); >> rand = 1 + prandom_u32_max(elen+1); >> /* Since elen < BUF_SIZE, we have 1 <= rand <= BUF_SIZE. */ >> - failed_tests += do_test(rand, expect, elen, fmt, ap); > > [*] Certainly this invariant gets violated, so we (may) provide do_test > with a buffer size larger than, well, BUF_SIZE. > >> >> -#define test(expect, fmt, ...) \ >> - __test(expect, strlen(expect), fmt, ##__VA_ARGS__) >> +#define test(kunittest, expect, fmt, ...) \ >> + __test(kunittest, expect, strlen(expect), fmt, ##__VA_ARGS__) >> >> -static void __init >> -test_basic(void) >> +static void >> +test_basic(struct kunit *kunittest) >> { >> /* Work around annoying "warning: zero-length gnu_printf format string". */ >> char nul = '\0'; >> >> - test("", &nul); >> - test("100%", "100%%"); >> - test("xxx%yyy", "xxx%cyyy", '%'); >> - __test("xxx\0yyy", 7, "xxx%cyyy", '\0'); >> + test(kunittest, "", &nul); >> + test(kunittest, "100%", "100%%"); >> + test(kunittest, "xxx%yyy", "xxx%cyyy", '%'); >> + __test(kunittest, "xxx\0yyy", 7, "xxx%cyyy", '\0'); > > Am I reading this right that all this is simply to prepend kunittest to > the arguments? How about just redefining the test macro so it > automagically does that instead of all this churn? The few cases that > use __test may need to be handled specially. > >> + >> +static void selftest(struct kunit *kunittest) >> { >> alloced_buffer = kmalloc(BUF_SIZE + 2*PAD_SIZE, GFP_KERNEL); >> if (!alloced_buffer) >> return; >> test_buffer = alloced_buffer + PAD_SIZE; >> >> - test_basic(); >> - test_number(); >> - test_string(); >> - test_pointer(); >> + test_basic(kunittest); >> + test_number(kunittest); >> + test_string(kunittest); >> + test_pointer(kunittest); >> >> kfree(alloced_buffer); >> } > > Even better, since the whole thing still relies on the static variables > test_buffer and alloced_buffer, why not just stash the struct kunit* > that the framework passes in a file-scope static and avoid even more > churn? Then only the newly introduce KUNIT_CHECK_* macros need to refer > to it, and none of the existing code (or future cases) needs that piece > of boilerplate. > Yes, using file-scope static will be better. I will make this change. > BTW, does the framework have some kind of logic that ensures nobody runs > the printf suite twice in parallel? > Brendan would have a better idea about this. But, it wouldn't be possible at boot up because KUnit only dispatches each test once. The other way for a KUnit test to be executed currently is as a module, and a module can only be loaded once until it is unloaded. Thanks for the review.
