lkml.org 
[lkml]   [2020]   [Aug]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] lib: Convert test_printf.c to KUnit
From
Date
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.

\
 
 \ /
  Last update: 2020-08-21 06:55    [W:0.196 / U:0.200 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site