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-te....
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.