Skip to contentSkip to conversation

lib: Convert test_printf.c to KUnit

PATCH Superseded Arpitha Raghunandan 6 participants 13 messages Aug 17, 2020 – Oct 13, 2020

Subsystems (2)
Kselftest Library code

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Arpitha Raghunandan

author
#0 lore raw

PATCH 1/1 lib: Convert test_printf.c to KUnit

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.

Signed-off-by Arpitha Raghunandan <[email protected]>
---
 lib/Kconfig.debug                     |   7 +-
 lib/Makefile                          |   2 +-
 lib/{test_printf.c => printf_kunit.c} | 498 +++++++++++++-------------
 3 files changed, 246 insertions(+), 261 deletions(-)
 rename lib/{test_printf.c => printf_kunit.c} (45%)
lib/Kconfig.debug +4 -3
index e068c3c7189a..7454258cf57a 100644
@@ -2027,9 +2027,6 @@ config TEST_STRSCPY
 config TEST_KSTRTOX
 	tristate "Test kstrto*() family of functions at runtime"
 
-config TEST_PRINTF
-	tristate "Test printf() family of functions at runtime"
-
 config TEST_BITMAP
 	tristate "Test bitmap_*() family of functions at runtime"
 	help

@@ -2247,6 +2244,10 @@ config BITS_TEST
 
 	  If unsure, say N.
 
+config PRINTF_KUNIT_TEST
+	tristate "KUnit tests for printf() family of functions at runtime"
+	depends on KUNIT
+
 config TEST_UDELAY
 	tristate "udelay test driver"
 	help
lib/Makefile +1 -1
index e290fc5707ea..68e5b32a185f 100644
@@ -77,7 +77,6 @@ obj-$(CONFIG_TEST_SORT) += test_sort.o
 obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
 obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
 obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
-obj-$(CONFIG_TEST_PRINTF) += test_printf.o
 obj-$(CONFIG_TEST_BITMAP) += test_bitmap.o
 obj-$(CONFIG_TEST_STRSCPY) += test_strscpy.o
 obj-$(CONFIG_TEST_BITFIELD) += test_bitfield.o

@@ -343,3 +342,4 @@ obj-$(CONFIG_PLDMFW) += pldmfw/
 obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
 obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
 obj-$(CONFIG_BITS_TEST) += test_bits.o
+obj-$(CONFIG_PRINTF_KUNIT_TEST) += printf_kunit.o
lib/printf_kunit.c renamed from lib/test_printf.c +241 -257

818 lines not on this page · raw message

Signature · 1 line
2.25.1
#1 lore raw
Quoted 4 lines · Arpitha Raghunandan

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.

Quoted 39 lines
lib/printf_kunit.c renamed from lib/test_printf.c +5 -8
similarity index 45%
rename from lib/test_printf.c
rename to lib/printf_kunit.c
index 7ac87f18a10f..68ac5f9b8d28 100644
@@ -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);

lib/printf_kunit.c line 56

Quoted 10 lines
lib/printf_kunit.c renamed from lib/test_printf.c +2 -4
 	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).

lib/printf_kunit.c

Quoted 9 lines
lib/printf_kunit.c renamed from lib/test_printf.c +4 -3
-	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?

Quoted 9 lines
lib/printf_kunit.c renamed from lib/test_printf.c +2 -2
-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;

lib/printf_kunit.c line 88

Quoted 10 lines
lib/printf_kunit.c renamed from lib/test_printf.c +3 -6
 
-	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? [*]

lib/printf_kunit.c __test(const char *expect, int elen, const char *fmt, ...) · line 118

Quoted 11 lines
lib/printf_kunit.c renamed from lib/test_printf.c +1 -2
 	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.

Quoted 12 lines
lib/printf_kunit.c renamed from lib/test_printf.c +4 -4
-#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". */

lib/printf_kunit.c __test(const char *expect, int elen, const char *fmt, ...) · line 126

Quoted 10 lines
lib/printf_kunit.c renamed from lib/test_printf.c +4 -4
 	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.

Quoted 9 lines
lib/printf_kunit.c renamed from lib/test_printf.c +2 -1
+
+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();

lib/printf_kunit.c static void __init fwnode_pointer(void) · line 685

Quoted 10 lines
lib/printf_kunit.c renamed from lib/test_printf.c +4 -3
-	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.

BTW, does the framework have some kind of logic that ensures nobody runs the printf suite twice in parallel?

Rasmus

author
#2 lore raw
Quoted 11 lines · Rasmus Villemoes
Quoted 4 lines · Arpitha Raghunandan

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.

Quoted 42 lines
Quoted 42 lines
lib/printf_kunit.c renamed from lib/test_printf.c +5 -8
similarity index 45%
rename from lib/test_printf.c
rename to lib/printf_kunit.c
index 7ac87f18a10f..68ac5f9b8d28 100644
@@ -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);
Quoted 15 lines

lib/printf_kunit.c line 56

Quoted 7 lines
lib/printf_kunit.c renamed from lib/test_printf.c +2 -4
-	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.

Quoted 4 lines
Quoted 3 lines
lib/printf_kunit.c renamed from lib/test_printf.c +1 -1
-	if (memchr_inv(alloced_buffer, FILL_CHAR, PAD_SIZE)) {
+	KUNIT_EXPECT_EQ_MSG(kunittest, memchr_inv(alloced_buffer, FILL_CHAR, PAD_SIZE), NULL,
Quoted 9 lines

lib/printf_kunit.c

Quoted 6 lines
lib/printf_kunit.c renamed from lib/test_printf.c +3 -2
-		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.

Quoted 84 lines
Quoted 10 lines
lib/printf_kunit.c renamed from lib/test_printf.c +2 -2
-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;

lib/printf_kunit.c line 88

Quoted 10 lines
lib/printf_kunit.c renamed from lib/test_printf.c +3 -6
 
-	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? [*]

lib/printf_kunit.c __test(const char *expect, int elen, const char *fmt, ...) · line 118

Quoted 11 lines
lib/printf_kunit.c renamed from lib/test_printf.c +1 -2
 	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.

Quoted 12 lines
lib/printf_kunit.c renamed from lib/test_printf.c +4 -4
-#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". */

lib/printf_kunit.c __test(const char *expect, int elen, const char *fmt, ...) · line 126

Quoted 10 lines
lib/printf_kunit.c renamed from lib/test_printf.c +4 -4
 	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.

Quoted 17 lines
lib/printf_kunit.c renamed from lib/test_printf.c +6 -4
+
+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);
Quoted 10 lines

lib/printf_kunit.c static void __init fwnode_pointer(void) · line 685

Quoted 2 lines
lib/printf_kunit.c renamed from lib/test_printf.c +0 -0
 	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.

Quoted 3 lines

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.

#3 lore raw
In reply to Rasmus Villemoes
Quoted 10 lines · Rasmus Villemoes
Quoted 4 lines · Arpitha Raghunandan

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.

I had the same concern. I have tried it. It compiled a module called printf_kunit.c.

#> modprobe printf_kunit

produced the following in dmesg:

[   60.931175] printf_kunit: module verification failed: signature and/or required key missing - tainting kernel
[   60.942209] TAP version 14
[   60.945197]     # Subtest: printf-kunit-test
[   60.945200]     1..1
[   60.951092]     ok 1 - selftest
[   60.953414] ok 1 - printf-kunit-test

I could live with the above. Then I tried to break a test by the following change:

lib/printf_kunit.c +1 -1
index 68ac5f9b8d28..1689dadd70a3 100644
@@ -395,7 +395,7 @@ ip4(struct kunit *kunittest)
        sa.sin_port = cpu_to_be16(12345);
        sa.sin_addr.s_addr = cpu_to_be32(0x7f000001);
 
-       test(kunittest, "127.000.000.001|127.0.0.1", "%pi4|%pI4", &sa.sin_addr, &sa.sin_addr);
+       test(kunittest, "127-000.000.001|127.0.0.1", "%pi4|%pI4", &sa.sin_addr, &sa.sin_addr);
        test(kunittest, "127.000.000.001|127.0.0.1", "%piS|%pIS", &sa, &sa);
        sa.sin_addr.s_addr = cpu_to_be32(0x01020304);
        test(kunittest, "001.002.003.004:12345|1.2.3.4:12345", "%piSp|%pISp", &sa, &sa);

It produced::

[   56.786858] TAP version 14
[   56.787493]     # Subtest: printf-kunit-test
[   56.787494]     1..1
[   56.788612]     # selftest: EXPECTATION FAILED at lib/printf_kunit.c:76
                   Expected memcmp(test_buffer, expect, written) == 0, but
                       memcmp(test_buffer, expect, written) == 1
                       0 == 0
               vsnprintf(buf, 256, "%pi4|%pI4", ...) wrote '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
[   56.795433]     # selftest: EXPECTATION FAILED at lib/printf_kunit.c:76
                   Expected memcmp(test_buffer, expect, written) == 0, but
                       memcmp(test_buffer, expect, written) == 1
                       0 == 0
               vsnprintf(buf, 20, "%pi4|%pI4", ...) wrote '127.000.000.001|127', expected '127-000.000.001|127'
[   56.800909]     # selftest: EXPECTATION FAILED at lib/printf_kunit.c:108
                   Expected memcmp(p, expect, elen+1) == 0, but
                       memcmp(p, expect, elen+1) == 1
                       0 == 0
               kvasprintf(..., "%pi4|%pI4", ...) returned '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
[   56.806497]     not ok 1 - selftest
[   56.806497] not ok 1 - printf-kunit-test

while the original code would have written the following error messages:

[   95.859225] test_printf: loaded.
[   95.860031] test_printf: vsnprintf(buf, 256, "%pi4|%pI4", ...) wrote '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
[   95.862630] test_printf: vsnprintf(buf, 6, "%pi4|%pI4", ...) wrote '127.0', expected '127-0'
[   95.864118] test_printf: kvasprintf(..., "%pi4|%pI4", ...) returned '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
[   95.866589] test_printf: failed 3 out of 388 tests

Even the error output is acceptable for me. I am just curious why the 2nd failure is different:

+ original code: vsnprintf(buf, 6, "%pi4|%pI4", ...) wrote '127.0', expected '127-0'
+ kunit code: vsnprintf(buf, 20, "%pi4|%pI4", ...) wrote '127.000.000.001|127', expected '127-000.000.001|127'

I am also a bit scared by the following note at
https://www.kernel.org/doc/html/latest/dev-tools/kunit/start.html#running-tests-without-the-kunit-wrapper

"KUnit is not designed for use in a production system, and it’s possible that tests may reduce the stability or security of the system."

What does it mean thay it might reduce stability or security? Is it because the tests might cause problems?
Or because the kunit framework modifies functionality of the running system all the time?

Best Regards,
Petr

PS: I still have to look at the code. Anwyay, the comments from Rasmus make sense.
_______________________________________________
Linux-kernel-mentees mailing list [email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

#4 lore raw
Quoted 7 lines · Petr Mladek
Quoted 6 lines · Rasmus Villemoes
Quoted 4 lines · Arpitha Raghunandan

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.

Quoted 6 lines
Quoted 4 lines

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.

I had the same concern. I have tried it.

Thanks for doing that and reporting the results.

Quoted 54 lines

#> modprobe printf_kunit

produced the following in dmesg:

[   60.931175] printf_kunit: module verification failed: signature and/or required key missing - tainting kernel
[   60.942209] TAP version 14
[   60.945197]     # Subtest: printf-kunit-test
[   60.945200]     1..1
[   60.951092]     ok 1 - selftest
[   60.953414] ok 1 - printf-kunit-test

I could live with the above. Then I tried to break a test by the following change:

lib/printf_kunit.c +1 -1
index 68ac5f9b8d28..1689dadd70a3 100644
@@ -395,7 +395,7 @@ ip4(struct kunit *kunittest)
        sa.sin_port = cpu_to_be16(12345);
        sa.sin_addr.s_addr = cpu_to_be32(0x7f000001);
 
-       test(kunittest, "127.000.000.001|127.0.0.1", "%pi4|%pI4", &sa.sin_addr, &sa.sin_addr);
+       test(kunittest, "127-000.000.001|127.0.0.1", "%pi4|%pI4", &sa.sin_addr, &sa.sin_addr);
        test(kunittest, "127.000.000.001|127.0.0.1", "%piS|%pIS", &sa, &sa);
        sa.sin_addr.s_addr = cpu_to_be32(0x01020304);
        test(kunittest, "001.002.003.004:12345|1.2.3.4:12345", "%piSp|%pISp", &sa, &sa);

It produced::

[   56.786858] TAP version 14
[   56.787493]     # Subtest: printf-kunit-test
[   56.787494]     1..1
[   56.788612]     # selftest: EXPECTATION FAILED at lib/printf_kunit.c:76
                   Expected memcmp(test_buffer, expect, written) == 0, but
                       memcmp(test_buffer, expect, written) == 1
                       0 == 0
               vsnprintf(buf, 256, "%pi4|%pI4", ...) wrote '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
[   56.795433]     # selftest: EXPECTATION FAILED at lib/printf_kunit.c:76
                   Expected memcmp(test_buffer, expect, written) == 0, but
                       memcmp(test_buffer, expect, written) == 1
                       0 == 0
               vsnprintf(buf, 20, "%pi4|%pI4", ...) wrote '127.000.000.001|127', expected '127-000.000.001|127'
[   56.800909]     # selftest: EXPECTATION FAILED at lib/printf_kunit.c:108
                   Expected memcmp(p, expect, elen+1) == 0, but
                       memcmp(p, expect, elen+1) == 1
                       0 == 0
               kvasprintf(..., "%pi4|%pI4", ...) returned '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
[   56.806497]     not ok 1 - selftest
[   56.806497] not ok 1 - printf-kunit-test

while the original code would have written the following error messages:

Quoted 8 lines
[   95.859225] test_printf: loaded.
[   95.860031] test_printf: vsnprintf(buf, 256, "%pi4|%pI4", ...) wrote '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
[   95.862630] test_printf: vsnprintf(buf, 6, "%pi4|%pI4", ...) wrote '127.0', expected '127-0'
[   95.864118] test_printf: kvasprintf(..., "%pi4|%pI4", ...) returned '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
[   95.866589] test_printf: failed 3 out of 388 tests

Even the error output is acceptable for me.

Urgh. Yeah, perhaps, but the original is much more readable; it really doesn't matter that a memcmp() fails to compare equal to 0, that's merely how we figured out that the output was wrong.

I am just curious why

Quoted 4 lines

the 2nd failure is different:

+ original code: vsnprintf(buf, 6, "%pi4|%pI4", ...) wrote '127.0', expected '127-0'
+ kunit code: vsnprintf(buf, 20, "%pi4|%pI4", ...) wrote '127.000.000.001|127', expected '127-000.000.001|127'

That's by design. If you read the code, there's a comment that says we do every test case four times: With a buffer that is large enough to do the whole output, with a 0 size buffer (that's essential to allowing kasprintf to know how much to allocate), with kvasprintf - but also with a buffer size that's guaranteed to ensure the output gets truncated somewhere. And that size is chosen randomly - I guess one could test every single buffer size between 0 and elen+1, but that's overkill.

Now I should probably have made the tests deterministic in the sense of getting a random seed for a PRNG, printing that seed and allowing a module parameter to set the seed in order to repeat the exact same tests. But so far I haven't really seen any bugs caught by test_printf which would have been easier to fix with that.

The reason I added that "chop it off somewhere randomly" was, IIRC, due to some %p extensions that behaved rather weirdly depending on whether there was enough room left or not, but I fixed those bugs before creating test_printf (and they were in turn the reason for creating test_printf). See for example 41416f2330, where %pE at the beginning of the format string would work ok, but if anything preceded it and the buffer was too small we'd crash.

Quoted 4 lines

I am also a bit scared by the following note at
https://www.kernel.org/doc/html/latest/dev-tools/kunit/start.html#running-tests-without-the-kunit-wrapper

Quoted 8 lines

"KUnit is not designed for use in a production system, and it’s possible that tests may reduce the stability or security of the system."

What does it mean thay it might reduce stability or security? Is it because the tests might cause problems?
Or because the kunit framework modifies functionality of the running system all the time?

Hm, yeah, that sounds a little frightening.

Rasmus
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

#5 lore raw

On Fri, Aug 21, 2020 at 5:19 AM Rasmus Villemoes
<[email protected]> wrote:

Sorry about the late reply. I saw activity on this before and thought it was under control. I only saw the unresolved state now looking through patchwork.

Quoted 78 lines · Petr Mladek
Quoted 7 lines · Rasmus Villemoes
Quoted 6 lines · Arpitha Raghunandan
Quoted 4 lines

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.

Quoted 6 lines
Quoted 4 lines

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.

I had the same concern. I have tried it.

Thanks for doing that and reporting the results.

Quoted 61 lines

#> modprobe printf_kunit

produced the following in dmesg:

[   60.931175] printf_kunit: module verification failed: signature and/or required key missing - tainting kernel
[   60.942209] TAP version 14
[   60.945197]     # Subtest: printf-kunit-test
[   60.945200]     1..1
[   60.951092]     ok 1 - selftest
[   60.953414] ok 1 - printf-kunit-test

I could live with the above. Then I tried to break a test by the following change:

lib/printf_kunit.c +1 -1
index 68ac5f9b8d28..1689dadd70a3 100644
@@ -395,7 +395,7 @@ ip4(struct kunit *kunittest)
        sa.sin_port = cpu_to_be16(12345);
        sa.sin_addr.s_addr = cpu_to_be32(0x7f000001);
 
-       test(kunittest, "127.000.000.001|127.0.0.1", "%pi4|%pI4", &sa.sin_addr, &sa.sin_addr);
+       test(kunittest, "127-000.000.001|127.0.0.1", "%pi4|%pI4", &sa.sin_addr, &sa.sin_addr);
        test(kunittest, "127.000.000.001|127.0.0.1", "%piS|%pIS", &sa, &sa);
        sa.sin_addr.s_addr = cpu_to_be32(0x01020304);
        test(kunittest, "001.002.003.004:12345|1.2.3.4:12345", "%piSp|%pISp", &sa, &sa);

It produced::

[   56.786858] TAP version 14
[   56.787493]     # Subtest: printf-kunit-test
[   56.787494]     1..1
[   56.788612]     # selftest: EXPECTATION FAILED at lib/printf_kunit.c:76
                   Expected memcmp(test_buffer, expect, written) == 0, but
                       memcmp(test_buffer, expect, written) == 1
                       0 == 0
               vsnprintf(buf, 256, "%pi4|%pI4", ...) wrote '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
[   56.795433]     # selftest: EXPECTATION FAILED at lib/printf_kunit.c:76
                   Expected memcmp(test_buffer, expect, written) == 0, but
                       memcmp(test_buffer, expect, written) == 1
                       0 == 0
               vsnprintf(buf, 20, "%pi4|%pI4", ...) wrote '127.000.000.001|127', expected '127-000.000.001|127'
[   56.800909]     # selftest: EXPECTATION FAILED at lib/printf_kunit.c:108
                   Expected memcmp(p, expect, elen+1) == 0, but
                       memcmp(p, expect, elen+1) == 1
                       0 == 0
               kvasprintf(..., "%pi4|%pI4", ...) returned '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
[   56.806497]     not ok 1 - selftest
[   56.806497] not ok 1 - printf-kunit-test

while the original code would have written the following error messages:

[   95.859225] test_printf: loaded.
[   95.860031] test_printf: vsnprintf(buf, 256, "%pi4|%pI4", ...) wrote '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
[   95.862630] test_printf: vsnprintf(buf, 6, "%pi4|%pI4", ...) wrote '127.0', expected '127-0'
[   95.864118] test_printf: kvasprintf(..., "%pi4|%pI4", ...) returned '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
[   95.866589] test_printf: failed 3 out of 388 tests
Quoted 5 lines
Quoted 1 line

Even the error output is acceptable for me.

Urgh. Yeah, perhaps, but the original is much more readable; it really doesn't matter that a memcmp() fails to compare equal to 0, that's merely how we figured out that the output was wrong.

We can go back to the original error reporting format, just as long as you don't mind the "ok" lines interspersed throughout (this is part of an attempt to standardize around the KTAP reporting format[1].

Quoted 20 lines

I am just curious why

Quoted 4 lines

the 2nd failure is different:

+ original code: vsnprintf(buf, 6, "%pi4|%pI4", ...) wrote '127.0', expected '127-0'
+ kunit code: vsnprintf(buf, 20, "%pi4|%pI4", ...) wrote '127.000.000.001|127', expected '127-000.000.001|127'

That's by design. If you read the code, there's a comment that says we do every test case four times: With a buffer that is large enough to do the whole output, with a 0 size buffer (that's essential to allowing kasprintf to know how much to allocate), with kvasprintf - but also with a buffer size that's guaranteed to ensure the output gets truncated somewhere. And that size is chosen randomly - I guess one could test every single buffer size between 0 and elen+1, but that's overkill.

Now I should probably have made the tests deterministic in the sense of getting a random seed for a PRNG, printing that seed and allowing a module parameter to set the seed in order to repeat the exact same tests. But so far I haven't really seen any bugs caught by test_printf which would have been easier to fix with that.

Quoted 20 lines

The reason I added that "chop it off somewhere randomly" was, IIRC, due to some %p extensions that behaved rather weirdly depending on whether there was enough room left or not, but I fixed those bugs before creating test_printf (and they were in turn the reason for creating test_printf). See for example 41416f2330, where %pE at the beginning of the format string would work ok, but if anything preceded it and the buffer was too small we'd crash.

Quoted 12 lines

I am also a bit scared by the following note at
https://www.kernel.org/doc/html/latest/dev-tools/kunit/start.html#running-tests-without-the-kunit-wrapper

"KUnit is not designed for use in a production system, and it’s possible that tests may reduce the stability or security of the system."

What does it mean thay it might reduce stability or security? Is it because the tests might cause problems?
Or because the kunit framework modifies functionality of the running system all the time?

Oh yeah, that's just because we are afraid that tests might cause problems. KUnit by itself does nothing to affect the stability or security of the system.

#6 lore raw
Quoted 132 lines · Brendan Higgins

On Fri, Aug 21, 2020 at 5:19 AM Rasmus Villemoes
<[email protected]> wrote:

Sorry about the late reply. I saw activity on this before and thought it was under control. I only saw the unresolved state now looking through patchwork.

Quoted 78 lines · Petr Mladek
Quoted 7 lines · Rasmus Villemoes
Quoted 6 lines · Arpitha Raghunandan
Quoted 4 lines

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.

Quoted 6 lines
Quoted 4 lines

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.

I had the same concern. I have tried it.

Thanks for doing that and reporting the results.

Quoted 61 lines

#> modprobe printf_kunit

produced the following in dmesg:

[   60.931175] printf_kunit: module verification failed: signature and/or required key missing - tainting kernel
[   60.942209] TAP version 14
[   60.945197]     # Subtest: printf-kunit-test
[   60.945200]     1..1
[   60.951092]     ok 1 - selftest
[   60.953414] ok 1 - printf-kunit-test

I could live with the above. Then I tried to break a test by the following change:

lib/printf_kunit.c +1 -1
index 68ac5f9b8d28..1689dadd70a3 100644
@@ -395,7 +395,7 @@ ip4(struct kunit *kunittest)
        sa.sin_port = cpu_to_be16(12345);
        sa.sin_addr.s_addr = cpu_to_be32(0x7f000001);
 
-       test(kunittest, "127.000.000.001|127.0.0.1", "%pi4|%pI4", &sa.sin_addr, &sa.sin_addr);
+       test(kunittest, "127-000.000.001|127.0.0.1", "%pi4|%pI4", &sa.sin_addr, &sa.sin_addr);
        test(kunittest, "127.000.000.001|127.0.0.1", "%piS|%pIS", &sa, &sa);
        sa.sin_addr.s_addr = cpu_to_be32(0x01020304);
        test(kunittest, "001.002.003.004:12345|1.2.3.4:12345", "%piSp|%pISp", &sa, &sa);

It produced::

[   56.786858] TAP version 14
[   56.787493]     # Subtest: printf-kunit-test
[   56.787494]     1..1
[   56.788612]     # selftest: EXPECTATION FAILED at lib/printf_kunit.c:76
                   Expected memcmp(test_buffer, expect, written) == 0, but
                       memcmp(test_buffer, expect, written) == 1
                       0 == 0
               vsnprintf(buf, 256, "%pi4|%pI4", ...) wrote '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
[   56.795433]     # selftest: EXPECTATION FAILED at lib/printf_kunit.c:76
                   Expected memcmp(test_buffer, expect, written) == 0, but
                       memcmp(test_buffer, expect, written) == 1
                       0 == 0
               vsnprintf(buf, 20, "%pi4|%pI4", ...) wrote '127.000.000.001|127', expected '127-000.000.001|127'
[   56.800909]     # selftest: EXPECTATION FAILED at lib/printf_kunit.c:108
                   Expected memcmp(p, expect, elen+1) == 0, but
                       memcmp(p, expect, elen+1) == 1
                       0 == 0
               kvasprintf(..., "%pi4|%pI4", ...) returned '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
[   56.806497]     not ok 1 - selftest
[   56.806497] not ok 1 - printf-kunit-test

while the original code would have written the following error messages:

[   95.859225] test_printf: loaded.
[   95.860031] test_printf: vsnprintf(buf, 256, "%pi4|%pI4", ...) wrote '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
[   95.862630] test_printf: vsnprintf(buf, 6, "%pi4|%pI4", ...) wrote '127.0', expected '127-0'
[   95.864118] test_printf: kvasprintf(..., "%pi4|%pI4", ...) returned '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
[   95.866589] test_printf: failed 3 out of 388 tests
Quoted 5 lines
Quoted 1 line

Even the error output is acceptable for me.

Urgh. Yeah, perhaps, but the original is much more readable; it really doesn't matter that a memcmp() fails to compare equal to 0, that's merely how we figured out that the output was wrong.

We can go back to the original error reporting format, just as long as you don't mind the "ok" lines interspersed throughout (this is part of an attempt to standardize around the KTAP reporting format[1].

Quoted 36 lines

I am just curious why

Quoted 4 lines

the 2nd failure is different:

+ original code: vsnprintf(buf, 6, "%pi4|%pI4", ...) wrote '127.0', expected '127-0'
+ kunit code: vsnprintf(buf, 20, "%pi4|%pI4", ...) wrote '127.000.000.001|127', expected '127-000.000.001|127'

That's by design. If you read the code, there's a comment that says we do every test case four times: With a buffer that is large enough to do the whole output, with a 0 size buffer (that's essential to allowing kasprintf to know how much to allocate), with kvasprintf - but also with a buffer size that's guaranteed to ensure the output gets truncated somewhere. And that size is chosen randomly - I guess one could test every single buffer size between 0 and elen+1, but that's overkill.

Now I should probably have made the tests deterministic in the sense of getting a random seed for a PRNG, printing that seed and allowing a module parameter to set the seed in order to repeat the exact same tests. But so far I haven't really seen any bugs caught by test_printf which would have been easier to fix with that.

The reason I added that "chop it off somewhere randomly" was, IIRC, due to some %p extensions that behaved rather weirdly depending on whether there was enough room left or not, but I fixed those bugs before creating test_printf (and they were in turn the reason for creating test_printf). See for example 41416f2330, where %pE at the beginning of the format string would work ok, but if anything preceded it and the buffer was too small we'd crash.

Quoted 8 lines

I am also a bit scared by the following note at
https://www.kernel.org/doc/html/latest/dev-tools/kunit/start.html#running-tests-without-the-kunit-wrapper

"KUnit is not designed for use in a production system, and it’s possible that tests may reduce the stability or security of the system."

Quoted 8 lines
Quoted 4 lines
Quoted 4 lines

What does it mean thay it might reduce stability or security? Is it because the tests might cause problems?
Or because the kunit framework modifies functionality of the running system all the time?

Oh yeah, that's just because we are afraid that tests might cause problems. KUnit by itself does nothing to affect the stability or security of the system.

And I forgot the link to KTAP[1]. I am really batting a thousand here.

[1] https://lore.kernel.org/linux-kselftest/CY4PR13MB1175B804E31E502221BC8163FD830@CY4PR13MB1175.namprd13.prod.outlook.com/

#7 lore raw
In reply to Petr Mladek
Quoted 7 lines · Petr Mladek
Quoted 6 lines · Rasmus Villemoes
Quoted 4 lines · Arpitha Raghunandan

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.

Quoted 6 lines
Quoted 4 lines

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.

I had the same concern. I have tried it.

Which raises an obvious question: did the people who convert this test this themselves? Looks like a janitor work in the area without understanding the area good enough.

Probably I will NAK all those patches from now on, until it will be good commit messages and cover of risen aspects, including reference to before and after outcome for passed and failed test cases.

Brendan, I guess the ball now on your side to prove this is good activity.

Signature · 8 lines
With Best Regards,
Andy Shevchenko


_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
#8 lore raw
Quoted 8 lines · Andy Shevchenko
Quoted 7 lines · Petr Mladek
Quoted 6 lines · Rasmus Villemoes
Quoted 4 lines · Arpitha Raghunandan

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.

Quoted 6 lines
Quoted 6 lines
Quoted 4 lines

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.

I had the same concern. I have tried it.

Sorry you feel that way. Do you have any suggestions on how we can make it seem less enterprisy? Seems like there are people here who are not a fan of the output format, so of which we can fix here, some of which is part of KTAP[1].

Quoted 3 lines

Which raises an obvious question: did the people who convert this test this themselves? Looks like a janitor work in the area without understanding the area good enough.

Looks to me like Arpitha ran it, but you are right, we don't have a lot of familiarity with this area; we were treating it as "janitor work" as you say.

Our intention was just to take some existing tests and as non-invasively as possible, get them to report using a common format, and maybe even get some of the tests to follow a common pattern.

Quoted 3 lines

Probably I will NAK all those patches from now on, until it will be good commit messages and cover of risen aspects, including reference to before and after outcome for passed and failed test cases.

Fair enough, hopefully we can address these issues in the next revision.

One issue though, with the "before and after outcome" you are referencing; are you referring to the issue that Petr pointed out in how they are inconsistent:

   + original code: vsnprintf(buf, 6, "%pi4|%pI4", ...) wrote '127.0', expected '127-0'
   + kunit code: vsnprintf(buf, 20, "%pi4|%pI4", ...) wrote '127.000.000.001|127', expected '127-000.000.001|127'

(I think Rasmus addressed this.) Or are your referring to something else?

Quoted 1 line

Brendan, I guess the ball now on your side to prove this is good activity.

And I see that we are off to a great start! :-)

In all seriousness, I am really sorry about this. I kind of bungled this up trying to go after too many of these conversions at once.

Arpitha, can you get this follow up patch out?

[1] https://lore.kernel.org/linux-kselftest/CY4PR13MB1175B804E31E502221BC8163FD830@CY4PR13MB1175.namprd13.prod.outlook.com/
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

author
#9 lore raw
Quoted 49 lines · Brendan Higgins
Quoted 8 lines · Andy Shevchenko
Quoted 7 lines · Petr Mladek
Quoted 6 lines · Rasmus Villemoes
Quoted 4 lines · Arpitha Raghunandan

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.

Quoted 6 lines
Quoted 6 lines
Quoted 4 lines

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.

I had the same concern. I have tried it.

Sorry you feel that way. Do you have any suggestions on how we can make it seem less enterprisy? Seems like there are people here who are not a fan of the output format, so of which we can fix here, some of which is part of KTAP[1].

Quoted 3 lines

Which raises an obvious question: did the people who convert this test this themselves? Looks like a janitor work in the area without understanding the area good enough.

Looks to me like Arpitha ran it, but you are right, we don't have a lot of familiarity with this area; we were treating it as "janitor work" as you say.

Our intention was just to take some existing tests and as non-invasively as possible, get them to report using a common format, and maybe even get some of the tests to follow a common pattern.

Quoted 3 lines

Probably I will NAK all those patches from now on, until it will be good commit messages and cover of risen aspects, including reference to before and after outcome for passed and failed test cases.

Fair enough, hopefully we can address these issues in the next revision.

One issue though, with the "before and after outcome" you are referencing; are you referring to the issue that Petr pointed out in how they are inconsistent:

   + original code: vsnprintf(buf, 6, "%pi4|%pI4", ...) wrote '127.0', expected '127-0'
   + kunit code: vsnprintf(buf, 20, "%pi4|%pI4", ...) wrote '127.000.000.001|127', expected '127-000.000.001|127'

(I think Rasmus addressed this.) Or are your referring to something else?

Quoted 9 lines
Quoted 1 line

Brendan, I guess the ball now on your side to prove this is good activity.

And I see that we are off to a great start! :-)

In all seriousness, I am really sorry about this. I kind of bungled this up trying to go after too many of these conversions at once.

Arpitha, can you get this follow up patch out?

Yes, I will work on this.

Quoted 2 lines

[1] https://lore.kernel.org/linux-kselftest/CY4PR13MB1175B804E31E502221BC8163FD830@CY4PR13MB1175.namprd13.prod.outlook.com/

#10 lore raw
In reply to Brendan Higgins
Quoted 14 lines · Brendan Higgins
Quoted 13 lines · Andy Shevchenko
Quoted 12 lines · Petr Mladek
Quoted 10 lines · Rasmus Villemoes
Quoted 4 lines · Arpitha Raghunandan

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.

Quoted 6 lines
Quoted 1 line
Quoted 1 line

I had the same concern. I have tried it.

Sorry you feel that way. Do you have any suggestions on how we can make it seem less enterprisy? Seems like there are people here who are not a fan of the output format, so of which we can fix here, some of which is part of KTAP[1].

I'm fine with machine-readable TAP, but I most defintely also want human-readable, which means all the excessive and pointless lines need to go away.

Quoted 22 lines
Quoted 3 lines

Which raises an obvious question: did the people who convert this test this themselves? Looks like a janitor work in the area without understanding the area good enough.

Looks to me like Arpitha ran it, but you are right, we don't have a lot of familiarity with this area; we were treating it as "janitor work" as you say.

Our intention was just to take some existing tests and as non-invasively as possible, get them to report using a common format, and maybe even get some of the tests to follow a common pattern.

Quoted 3 lines

Probably I will NAK all those patches from now on, until it will be good commit messages and cover of risen aspects, including reference to before and after outcome for passed and failed test cases.

Fair enough, hopefully we can address these issues in the next revision.

One issue though, with the "before and after outcome" you are referencing; are you referring to the issue that Petr pointed out in how they are inconsistent:

Quoted 5 lines

   + original code: vsnprintf(buf, 6, "%pi4|%pI4", ...) wrote '127.0', expected '127-0'
   + kunit code: vsnprintf(buf, 20, "%pi4|%pI4", ...) wrote '127.000.000.001|127', expected '127-000.000.001|127'

(I think Rasmus addressed this.) Or are your referring to something else?

Yeah, that change is fine and expected, can we stop bringing that up.

It's all the explicit "memcmp() == 0 failed" gunk at least I am concerned with. If you can get rid of that (basically, stop stringifying the code, that's completely irrelevant) and just get the messages from the test itself that explains what went wrong. I'm fine with interspersing that with a few TAP-readable lines. But things like

Expected memcmp(test_buffer, expect, written) == 0, but

                       memcmp(test_buffer, expect, written) == 1
                       0 == 0

are utterly useless. We're not _testing_ memcmp, we're _using_ it to know if vsprintf() did as we expected. So just mechanically changing "memcmp() == 0" into "SOME_MACRO(memcmp(), 0)" is never going to work, at least when SOME_MACRO does the stringify and ends up producing the above. But if you can end up producing

[   56.795433]     # selftest: EXPECTATION FAILED at lib/printf_kunit.c:76
               vsnprintf(buf, 20, "%pi4|%pI4", ...) wrote

'127.000.000.001|127', expected '127-000.000.001|127'

that's fine; that's basically just prepending an EXPECTATION FAILED line to the existing output.

So doing it properly would probably be either

- change the existing pr_warn()s to use some KUNIT macro that generates whatever extra info is needed by TAP, in addition to the current human-readable message, and/or
- just add a few lines of TAP-suitable FAIL/PASS lines here and there

but let me repeat that the control flow (early returns) in do_test() cannot be modified.

Rasmus

build bot
#11 lore raw
In reply to Arpitha Raghunandan
Thank you for the patch! Yet something to improve:

Hi Arpitha,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.9-rc1 next-20200817]
[cannot apply to pmladek/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Arpitha-Raghunandan/lib-Convert-test_printf-c-to-KUnit/20200817-123341
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 9123e3a74ec7b934a4a099e98af6a61c2f80bbf5
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.3.0

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "__delay" undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

build bot
#12 lore raw
In reply to Arpitha Raghunandan
Thank you for the patch! Perhaps something to improve:

Hi Arpitha,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.9-rc1 next-20200817]
[cannot apply to pmladek/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Arpitha-Raghunandan/lib-Convert-test_printf-c-to-KUnit/20200817-123341
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 9123e3a74ec7b934a4a099e98af6a61c2f80bbf5
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

>> WARNING: modpost: lib/printf_kunit.o(.text+0x4dc): Section mismatch in reference from the function dentry() to the variable .init.data:test_dentry

The function dentry() references
the variable __initdata test_dentry.
This is often because dentry lacks a __initdata annotation or the annotation of test_dentry is wrong. --

>> WARNING: modpost: lib/printf_kunit.o(.text+0x680): Section mismatch in reference from the function dentry() to the (unknown reference) .init.data:(unknown)

The function dentry() references
the (unknown reference) __initdata (unknown). This is often because dentry lacks a __initdata annotation or the annotation of (unknown) is wrong.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org