Message-ID: <2e1d1e40-b448-9389-d7d2-93841af60a88@gmail.com>
Date:   Fri, 21 Aug 2020 10:24:36 +0530
From:   Arpitha Raghunandan <98.arpi@...il.com>
To:     Rasmus Villemoes <linux@...musvillemoes.dk>,
        brendanhiggins@...gle.com, skhan@...uxfoundation.org,
        andriy.shevchenko@...ux.intel.com, pmladek@...e.com,
        rostedt@...dmis.org, sergey.senozhatsky@...il.com
Cc:     kunit-dev@...glegroups.com, linux-kselftest@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        linux-kernel-mentees@...ts.linuxfoundation.org
Subject: Re: [PATCH] lib: Convert test_printf.c to KUnit

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.

Powered by blists - more mailing lists