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 98.arpi@gmail.com --- 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%)
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index e068c3c7189a..7454258cf57a 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -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 diff --git a/lib/Makefile b/lib/Makefile index e290fc5707ea..68e5b32a185f 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -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 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; - }
- if (memchr_inv(alloced_buffer, FILL_CHAR, PAD_SIZE)) { - pr_warn("vsnprintf(buf, %d, "%s", ...) wrote before buffer\n", bufsize, fmt); - return 1; - } + KUNIT_EXPECT_EQ_MSG(kunittest, memchr_inv(alloced_buffer, FILL_CHAR, PAD_SIZE), NULL, + "vsnprintf(buf, %d, "%s", ...) wrote before buffer\n", + bufsize, fmt);
if (!bufsize) { - if (memchr_inv(test_buffer, FILL_CHAR, BUF_SIZE + PAD_SIZE)) { - pr_warn("vsnprintf(buf, 0, "%s", ...) wrote to buffer\n", - fmt); - return 1; - } - return 0; + KUNIT_EXPECT_FALSE_MSG(kunittest, + memchr_inv(test_buffer, FILL_CHAR, BUF_SIZE + PAD_SIZE), + "vsnprintf(buf, 0, "%s", ...) wrote to buffer\n", fmt); + return; }
written = min(bufsize-1, elen); - if (test_buffer[written]) { - pr_warn("vsnprintf(buf, %d, "%s", ...) did not nul-terminate buffer\n", + KUNIT_EXPECT_FALSE_MSG(kunittest, test_buffer[written], + "vsnprintf(buf, %d, "%s", ...) did not nul-terminate buffer\n", bufsize, fmt); - return 1; - }
- if (memchr_inv(test_buffer + written + 1, FILL_CHAR, BUF_SIZE + PAD_SIZE - (written + 1))) { - pr_warn("vsnprintf(buf, %d, "%s", ...) wrote beyond the nul-terminator\n", + KUNIT_EXPECT_FALSE_MSG(kunittest, + memchr_inv(test_buffer + written + 1, FILL_CHAR, BUF_SIZE + PAD_SIZE - (written + 1)), + "vsnprintf(buf, %d, "%s", ...) wrote beyond the nul-terminator\n", bufsize, fmt); - return 1; - }
- if (memcmp(test_buffer, expect, written)) { - pr_warn("vsnprintf(buf, %d, "%s", ...) wrote '%s', expected '%.*s'\n", + KUNIT_EXPECT_EQ_MSG(kunittest, memcmp(test_buffer, expect, written), 0, + "vsnprintf(buf, %d, "%s", ...) wrote '%s', expected '%.*s'\n", bufsize, fmt, test_buffer, written, expect); - return 1; - } - return 0; }
-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);
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); - failed_tests += do_test(0, expect, elen, fmt, ap); + do_test(kunittest, rand, expect, elen, fmt, ap); + do_test(kunittest, 0, expect, elen, fmt, ap);
p = kvasprintf(GFP_KERNEL, fmt, ap); if (p) { - total_tests++; - if (memcmp(p, expect, elen+1)) { - pr_warn("kvasprintf(..., "%s", ...) returned '%s', expected '%s'\n", + KUNIT_EXPECT_EQ_MSG(kunittest, memcmp(p, expect, elen+1), 0, + "kvasprintf(..., "%s", ...) returned '%s', expected '%s'\n", fmt, p, expect); - failed_tests++; - } kfree(p); } va_end(ap); }
-#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'); }
-static void __init -test_number(void) +static void +test_number(struct kunit *kunittest) { - test("0x1234abcd ", "%#-12x", 0x1234abcd); - test(" 0x1234abcd", "%#12x", 0x1234abcd); - test("0|001| 12|+123| 1234|-123|-1234", "%d|%03d|%3d|%+d|% d|%+d|% d", 0, 1, 12, 123, 1234, -123, -1234); - test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1); - test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1); - test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627); + test(kunittest, "0x1234abcd ", "%#-12x", 0x1234abcd); + test(kunittest, " 0x1234abcd", "%#12x", 0x1234abcd); + test(kunittest, "0|001| 12|+123| 1234|-123|-1234", "%d|%03d|%3d|%+d|% d|%+d|% d", 0, 1, 12, 123, 1234, -123, -1234); + test(kunittest, "0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1); + test(kunittest, "0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1); + test(kunittest, "2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627); /* * POSIX/C99: »The result of converting zero with an explicit * precision of zero shall be no characters.« Hence the output @@ -163,7 +143,7 @@ test_number(void) * case. This test case simply documents the current * behaviour. */ - test("00|0|0|0|0", "%.2d|%.1d|%.0d|%.*d|%1.0d", 0, 0, 0, 0, 0, 0); + test(kunittest, "00|0|0|0|0", "%.2d|%.1d|%.0d|%.*d|%1.0d", 0, 0, 0, 0, 0, 0); #ifndef __CHAR_UNSIGNED__ { /* @@ -173,19 +153,19 @@ test_number(void) * with 0xff or cast to u8. */ char val = -16; - test("0xfffffff0|0xf0|0xf0", "%#02x|%#02x|%#02x", val, val & 0xff, (u8)val); + test(kunittest, "0xfffffff0|0xf0|0xf0", "%#02x|%#02x|%#02x", val, val & 0xff, (u8)val); } #endif }
-static void __init -test_string(void) +static void +test_string(struct kunit *kunittest) { - test("", "%s%.0s", "", "123"); - test("ABCD|abc|123", "%s|%.3s|%.*s", "ABCD", "abcdef", 3, "123456"); - test("1 | 2|3 | 4|5 ", "%-3s|%3s|%-*s|%*s|%*s", "1", "2", 3, "3", 3, "4", -3, "5"); - test("1234 ", "%-10.4s", "123456"); - test(" 1234", "%10.4s", "123456"); + test(kunittest, "", "%s%.0s", "", "123"); + test(kunittest, "ABCD|abc|123", "%s|%.3s|%.*s", "ABCD", "abcdef", 3, "123456"); + test(kunittest, "1 | 2|3 | 4|5 ", "%-3s|%3s|%-*s|%*s|%*s", "1", "2", 3, "3", 3, "4", -3, "5"); + test(kunittest, "1234 ", "%-10.4s", "123456"); + test(kunittest, " 1234", "%10.4s", "123456"); /* * POSIX and C99 say that a negative precision (which is only * possible to pass via a * argument) should be treated as if @@ -199,10 +179,10 @@ test_string(void) * anyone ever feel the need to follow the standards more * closely, this can be revisited. */ - test(" ", "%4.*s", -5, "123456"); - test("123456", "%.s", "123456"); - test("a||", "%.s|%.0s|%.*s", "a", "b", 0, "c"); - test("a | | ", "%-3.s|%-3.0s|%-3.*s", "a", "b", 0, "c"); + test(kunittest, " ", "%4.*s", -5, "123456"); + test(kunittest, "123456", "%.s", "123456"); + test(kunittest, "a||", "%.s|%.0s|%.*s", "a", "b", 0, "c"); + test(kunittest, "a | | ", "%-3.s|%-3.0s|%-3.*s", "a", "b", 0, "c"); }
#define PLAIN_BUF_SIZE 64 /* leave some space so we don't oops */ @@ -216,7 +196,7 @@ test_string(void) #define ZEROS "00000000" /* hex 32 zero bits */ #define ONES "ffffffff" /* hex 32 one bits */
-static int __init +static int plain_format(void) { char buf[PLAIN_BUF_SIZE]; @@ -248,7 +228,7 @@ plain_format(void) #define ZEROS "" #define ONES ""
-static int __init +static int plain_format(void) { /* Format is implicitly tested for 32 bit machines by plain_hash() */ @@ -257,7 +237,7 @@ plain_format(void)
#endif /* BITS_PER_LONG == 64 */
-static int __init +static int plain_hash_to_buffer(const void *p, char *buf, size_t len) { int nchars; @@ -276,7 +256,7 @@ plain_hash_to_buffer(const void *p, char *buf, size_t len) return 0; }
-static int __init +static int plain_hash(void) { char buf[PLAIN_BUF_SIZE]; @@ -296,27 +276,20 @@ plain_hash(void) * We can't use test() to test %p because we don't know what output to expect * after an address is hashed. */ -static void __init -plain(void) +static void +plain(struct kunit *kunittest) { int err;
err = plain_hash(); - if (err) { - pr_warn("plain 'p' does not appear to be hashed\n"); - failed_tests++; - return; - } + KUNIT_EXPECT_EQ_MSG(kunittest, err, 0, "plain 'p' does not appear to be hashed\n");
err = plain_format(); - if (err) { - pr_warn("hashing plain 'p' has unexpected format\n"); - failed_tests++; - } + KUNIT_EXPECT_EQ_MSG(kunittest, err, 0, "hashing plain 'p' has unexpected format\n"); }
-static void __init -test_hashed(const char *fmt, const void *p) +static void +test_hashed(struct kunit *kunittest, const char *fmt, const void *p) { char buf[PLAIN_BUF_SIZE]; int ret; @@ -329,92 +302,92 @@ test_hashed(const char *fmt, const void *p) if (ret) return;
- test(buf, fmt, p); + test(kunittest, buf, fmt, p); }
/* * NULL pointers aren't hashed. */ -static void __init -null_pointer(void) +static void +null_pointer(struct kunit *kunittest) { - test(ZEROS "00000000", "%p", NULL); - test(ZEROS "00000000", "%px", NULL); - test("(null)", "%pE", NULL); + test(kunittest, ZEROS "00000000", "%p", NULL); + test(kunittest, ZEROS "00000000", "%px", NULL); + test(kunittest, "(null)", "%pE", NULL); }
/* * Error pointers aren't hashed. */ -static void __init -error_pointer(void) +static void +error_pointer(struct kunit *kunittest) { - test(ONES "fffffff5", "%p", ERR_PTR(-11)); - test(ONES "fffffff5", "%px", ERR_PTR(-11)); - test("(efault)", "%pE", ERR_PTR(-11)); + test(kunittest, ONES "fffffff5", "%p", ERR_PTR(-11)); + test(kunittest, ONES "fffffff5", "%px", ERR_PTR(-11)); + test(kunittest, "(efault)", "%pE", ERR_PTR(-11)); }
#define PTR_INVALID ((void *)0x000000ab)
-static void __init -invalid_pointer(void) +static void +invalid_pointer(struct kunit *kunittest) { - test_hashed("%p", PTR_INVALID); - test(ZEROS "000000ab", "%px", PTR_INVALID); - test("(efault)", "%pE", PTR_INVALID); + test_hashed(kunittest, "%p", PTR_INVALID); + test(kunittest, ZEROS "000000ab", "%px", PTR_INVALID); + test(kunittest, "(efault)", "%pE", PTR_INVALID); }
-static void __init -symbol_ptr(void) +static void +symbol_ptr(struct kunit *kunittest) { }
-static void __init -kernel_ptr(void) +static void +kernel_ptr(struct kunit *kunittest) { /* We can't test this without access to kptr_restrict. */ }
-static void __init -struct_resource(void) +static void +struct_resource(struct kunit *kunittest) { }
-static void __init -addr(void) +static void +addr(struct kunit *kunittest) { }
-static void __init -escaped_str(void) +static void +escaped_str(struct kunit *kunittest) { }
-static void __init -hex_string(void) +static void +hex_string(struct kunit *kunittest) { const char buf[3] = {0xc0, 0xff, 0xee};
- test("c0 ff ee|c0:ff:ee|c0-ff-ee|c0ffee", + test(kunittest, "c0 ff ee|c0:ff:ee|c0-ff-ee|c0ffee", "%3ph|%3phC|%3phD|%3phN", buf, buf, buf, buf); - test("c0 ff ee|c0:ff:ee|c0-ff-ee|c0ffee", + test(kunittest, "c0 ff ee|c0:ff:ee|c0-ff-ee|c0ffee", "%*ph|%*phC|%*phD|%*phN", 3, buf, 3, buf, 3, buf, 3, buf); }
-static void __init -mac(void) +static void +mac(struct kunit *kunittest) { const u8 addr[6] = {0x2d, 0x48, 0xd6, 0xfc, 0x7a, 0x05};
- test("2d:48:d6:fc:7a:05", "%pM", addr); - test("05:7a:fc:d6:48:2d", "%pMR", addr); - test("2d-48-d6-fc-7a-05", "%pMF", addr); - test("2d48d6fc7a05", "%pm", addr); - test("057afcd6482d", "%pmR", addr); + test(kunittest, "2d:48:d6:fc:7a:05", "%pM", addr); + test(kunittest, "05:7a:fc:d6:48:2d", "%pMR", addr); + test(kunittest, "2d-48-d6-fc-7a-05", "%pMF", addr); + test(kunittest, "2d48d6fc7a05", "%pm", addr); + test(kunittest, "057afcd6482d", "%pmR", addr); }
-static void __init -ip4(void) +static void +ip4(struct kunit *kunittest) { struct sockaddr_in sa;
@@ -422,34 +395,34 @@ ip4(void) sa.sin_port = cpu_to_be16(12345); sa.sin_addr.s_addr = cpu_to_be32(0x7f000001);
- test("127.000.000.001|127.0.0.1", "%pi4|%pI4", &sa.sin_addr, &sa.sin_addr); - test("127.000.000.001|127.0.0.1", "%piS|%pIS", &sa, &sa); + 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("001.002.003.004:12345|1.2.3.4:12345", "%piSp|%pISp", &sa, &sa); + test(kunittest, "001.002.003.004:12345|1.2.3.4:12345", "%piSp|%pISp", &sa, &sa); }
-static void __init -ip6(void) +static void +ip6(struct kunit *kunittest) { }
-static void __init -ip(void) +static void +ip(struct kunit *kunittest) { - ip4(); - ip6(); + ip4(kunittest); + ip6(kunittest); }
-static void __init -uuid(void) +static void +uuid(struct kunit *kunittest) { const char uuid[16] = {0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8, 0x9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf};
- test("00010203-0405-0607-0809-0a0b0c0d0e0f", "%pUb", uuid); - test("00010203-0405-0607-0809-0A0B0C0D0E0F", "%pUB", uuid); - test("03020100-0504-0706-0809-0a0b0c0d0e0f", "%pUl", uuid); - test("03020100-0504-0706-0809-0A0B0C0D0E0F", "%pUL", uuid); + test(kunittest, "00010203-0405-0607-0809-0a0b0c0d0e0f", "%pUb", uuid); + test(kunittest, "00010203-0405-0607-0809-0A0B0C0D0E0F", "%pUB", uuid); + test(kunittest, "03020100-0504-0706-0809-0a0b0c0d0e0f", "%pUl", uuid); + test(kunittest, "03020100-0504-0706-0809-0A0B0C0D0E0F", "%pUL", uuid); }
static struct dentry test_dentry[4] __initdata = { @@ -467,34 +440,34 @@ static struct dentry test_dentry[4] __initdata = { .d_iname = "romeo" }, };
-static void __init -dentry(void) +static void +dentry(struct kunit *kunittest) { - test("foo", "%pd", &test_dentry[0]); - test("foo", "%pd2", &test_dentry[0]); + test(kunittest, "foo", "%pd", &test_dentry[0]); + test(kunittest, "foo", "%pd2", &test_dentry[0]);
- test("(null)", "%pd", NULL); - test("(efault)", "%pd", PTR_INVALID); - test("(null)", "%pD", NULL); - test("(efault)", "%pD", PTR_INVALID); + test(kunittest, "(null)", "%pd", NULL); + test(kunittest, "(efault)", "%pd", PTR_INVALID); + test(kunittest, "(null)", "%pD", NULL); + test(kunittest, "(efault)", "%pD", PTR_INVALID);
- test("romeo", "%pd", &test_dentry[3]); - test("alfa/romeo", "%pd2", &test_dentry[3]); - test("bravo/alfa/romeo", "%pd3", &test_dentry[3]); - test("/bravo/alfa/romeo", "%pd4", &test_dentry[3]); - test("/bravo/alfa", "%pd4", &test_dentry[2]); + test(kunittest, "romeo", "%pd", &test_dentry[3]); + test(kunittest, "alfa/romeo", "%pd2", &test_dentry[3]); + test(kunittest, "bravo/alfa/romeo", "%pd3", &test_dentry[3]); + test(kunittest, "/bravo/alfa/romeo", "%pd4", &test_dentry[3]); + test(kunittest, "/bravo/alfa", "%pd4", &test_dentry[2]);
- test("bravo/alfa |bravo/alfa ", "%-12pd2|%*pd2", &test_dentry[2], -12, &test_dentry[2]); - test(" bravo/alfa| bravo/alfa", "%12pd2|%*pd2", &test_dentry[2], 12, &test_dentry[2]); + test(kunittest, "bravo/alfa |bravo/alfa ", "%-12pd2|%*pd2", &test_dentry[2], -12, &test_dentry[2]); + test(kunittest, " bravo/alfa| bravo/alfa", "%12pd2|%*pd2", &test_dentry[2], 12, &test_dentry[2]); }
-static void __init -struct_va_format(void) +static void +struct_va_format(struct kunit *kunittest) { }
-static void __init -time_and_date(void) +static void +time_and_date(struct kunit *kunittest) { /* 1543210543 */ const struct rtc_time tm = { @@ -508,27 +481,27 @@ time_and_date(void) /* 2019-01-04T15:32:23 */ time64_t t = 1546615943;
- test("(%pt?)", "%pt", &tm); - test("2018-11-26T05:35:43", "%ptR", &tm); - test("0118-10-26T05:35:43", "%ptRr", &tm); - test("05:35:43|2018-11-26", "%ptRt|%ptRd", &tm, &tm); - test("05:35:43|0118-10-26", "%ptRtr|%ptRdr", &tm, &tm); - test("05:35:43|2018-11-26", "%ptRttr|%ptRdtr", &tm, &tm); - test("05:35:43 tr|2018-11-26 tr", "%ptRt tr|%ptRd tr", &tm, &tm); + test(kunittest, "(%pt?)", "%pt", &tm); + test(kunittest, "2018-11-26T05:35:43", "%ptR", &tm); + test(kunittest, "0118-10-26T05:35:43", "%ptRr", &tm); + test(kunittest, "05:35:43|2018-11-26", "%ptRt|%ptRd", &tm, &tm); + test(kunittest, "05:35:43|0118-10-26", "%ptRtr|%ptRdr", &tm, &tm); + test(kunittest, "05:35:43|2018-11-26", "%ptRttr|%ptRdtr", &tm, &tm); + test(kunittest, "05:35:43 tr|2018-11-26 tr", "%ptRt tr|%ptRd tr", &tm, &tm);
- test("2019-01-04T15:32:23", "%ptT", &t); - test("0119-00-04T15:32:23", "%ptTr", &t); - test("15:32:23|2019-01-04", "%ptTt|%ptTd", &t, &t); - test("15:32:23|0119-00-04", "%ptTtr|%ptTdr", &t, &t); + test(kunittest, "2019-01-04T15:32:23", "%ptT", &t); + test(kunittest, "0119-00-04T15:32:23", "%ptTr", &t); + test(kunittest, "15:32:23|2019-01-04", "%ptTt|%ptTd", &t, &t); + test(kunittest, "15:32:23|0119-00-04", "%ptTtr|%ptTdr", &t, &t); }
-static void __init -struct_clk(void) +static void +struct_clk(struct kunit *kunittest) { }
-static void __init -large_bitmap(void) +static void +large_bitmap(struct kunit *kunittest) { const int nbits = 1 << 16; unsigned long *bits = bitmap_zalloc(nbits, GFP_KERNEL); @@ -537,69 +510,69 @@ large_bitmap(void)
bitmap_set(bits, 1, 20); bitmap_set(bits, 60000, 15); - test("1-20,60000-60014", "%*pbl", nbits, bits); + test(kunittest, "1-20,60000-60014", "%*pbl", nbits, bits); bitmap_free(bits); }
-static void __init -bitmap(void) +static void +bitmap(struct kunit *kunittest) { DECLARE_BITMAP(bits, 20); const int primes[] = {2,3,5,7,11,13,17,19}; int i;
bitmap_zero(bits, 20); - test("00000|00000", "%20pb|%*pb", bits, 20, bits); - test("|", "%20pbl|%*pbl", bits, 20, bits); + test(kunittest, "00000|00000", "%20pb|%*pb", bits, 20, bits); + test(kunittest, "|", "%20pbl|%*pbl", bits, 20, bits);
for (i = 0; i < ARRAY_SIZE(primes); ++i) set_bit(primes[i], bits); - test("a28ac|a28ac", "%20pb|%*pb", bits, 20, bits); - test("2-3,5,7,11,13,17,19|2-3,5,7,11,13,17,19", "%20pbl|%*pbl", bits, 20, bits); + test(kunittest, "a28ac|a28ac", "%20pb|%*pb", bits, 20, bits); + test(kunittest, "2-3,5,7,11,13,17,19|2-3,5,7,11,13,17,19", "%20pbl|%*pbl", bits, 20, bits);
bitmap_fill(bits, 20); - test("fffff|fffff", "%20pb|%*pb", bits, 20, bits); - test("0-19|0-19", "%20pbl|%*pbl", bits, 20, bits); + test(kunittest, "fffff|fffff", "%20pb|%*pb", bits, 20, bits); + test(kunittest, "0-19|0-19", "%20pbl|%*pbl", bits, 20, bits);
- large_bitmap(); + large_bitmap(kunittest); }
-static void __init -netdev_features(void) +static void +netdev_features(struct kunit *kunittest) { }
-static void __init -flags(void) +static void +flags(struct kunit *kunittest) { unsigned long flags; gfp_t gfp; char *cmp_buffer;
flags = 0; - test("", "%pGp", &flags); + test(kunittest, "", "%pGp", &flags);
/* Page flags should filter the zone id */ flags = 1UL << NR_PAGEFLAGS; - test("", "%pGp", &flags); + test(kunittest, "", "%pGp", &flags);
flags |= 1UL << PG_uptodate | 1UL << PG_dirty | 1UL << PG_lru | 1UL << PG_active | 1UL << PG_swapbacked; - test("uptodate|dirty|lru|active|swapbacked", "%pGp", &flags); + test(kunittest, "uptodate|dirty|lru|active|swapbacked", "%pGp", &flags);
flags = VM_READ | VM_EXEC | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC | VM_DENYWRITE; - test("read|exec|mayread|maywrite|mayexec|denywrite", "%pGv", &flags); + test(kunittest, "read|exec|mayread|maywrite|mayexec|denywrite", "%pGv", &flags);
gfp = GFP_TRANSHUGE; - test("GFP_TRANSHUGE", "%pGg", &gfp); + test(kunittest, "GFP_TRANSHUGE", "%pGg", &gfp);
gfp = GFP_ATOMIC|__GFP_DMA; - test("GFP_ATOMIC|GFP_DMA", "%pGg", &gfp); + test(kunittest, "GFP_ATOMIC|GFP_DMA", "%pGg", &gfp);
gfp = __GFP_ATOMIC; - test("__GFP_ATOMIC", "%pGg", &gfp); + test(kunittest, "__GFP_ATOMIC", "%pGg", &gfp);
cmp_buffer = kmalloc(BUF_SIZE, GFP_KERNEL); if (!cmp_buffer) @@ -608,17 +581,17 @@ flags(void) /* Any flags not translated by the table should remain numeric */ gfp = ~__GFP_BITS_MASK; snprintf(cmp_buffer, BUF_SIZE, "%#lx", (unsigned long) gfp); - test(cmp_buffer, "%pGg", &gfp); + test(kunittest, cmp_buffer, "%pGg", &gfp);
snprintf(cmp_buffer, BUF_SIZE, "__GFP_ATOMIC|%#lx", (unsigned long) gfp); gfp |= __GFP_ATOMIC; - test(cmp_buffer, "%pGg", &gfp); + test(kunittest, cmp_buffer, "%pGg", &gfp);
kfree(cmp_buffer); }
-static void __init fwnode_pointer(void) +static void fwnode_pointer(struct kunit *kunittest) { const struct software_node softnodes[] = { { .name = "first", }, @@ -638,79 +611,90 @@ static void __init fwnode_pointer(void) return; }
- test(full_name_second, "%pfw", software_node_fwnode(&softnodes[1])); - test(full_name, "%pfw", software_node_fwnode(&softnodes[2])); - test(full_name, "%pfwf", software_node_fwnode(&softnodes[2])); - test(second_name, "%pfwP", software_node_fwnode(&softnodes[1])); - test(third_name, "%pfwP", software_node_fwnode(&softnodes[2])); + test(kunittest, full_name_second, "%pfw", software_node_fwnode(&softnodes[1])); + test(kunittest, full_name, "%pfw", software_node_fwnode(&softnodes[2])); + test(kunittest, full_name, "%pfwf", software_node_fwnode(&softnodes[2])); + test(kunittest, second_name, "%pfwP", software_node_fwnode(&softnodes[1])); + test(kunittest, third_name, "%pfwP", software_node_fwnode(&softnodes[2]));
software_node_unregister(&softnodes[2]); software_node_unregister(&softnodes[1]); software_node_unregister(&softnodes[0]); }
-static void __init -errptr(void) +static void +errptr(struct kunit *kunittest) { - test("-1234", "%pe", ERR_PTR(-1234)); + test(kunittest, "-1234", "%pe", ERR_PTR(-1234));
/* Check that %pe with a non-ERR_PTR gets treated as ordinary %p. */ BUILD_BUG_ON(IS_ERR(PTR)); - test_hashed("%pe", PTR); + test_hashed(kunittest, "%pe", PTR);
#ifdef CONFIG_SYMBOLIC_ERRNAME - test("(-ENOTSOCK)", "(%pe)", ERR_PTR(-ENOTSOCK)); - test("(-EAGAIN)", "(%pe)", ERR_PTR(-EAGAIN)); + test(kunittest, "(-ENOTSOCK)", "(%pe)", ERR_PTR(-ENOTSOCK)); + test(kunittest, "(-EAGAIN)", "(%pe)", ERR_PTR(-EAGAIN)); BUILD_BUG_ON(EAGAIN != EWOULDBLOCK); - test("(-EAGAIN)", "(%pe)", ERR_PTR(-EWOULDBLOCK)); - test("[-EIO ]", "[%-8pe]", ERR_PTR(-EIO)); - test("[ -EIO]", "[%8pe]", ERR_PTR(-EIO)); - test("-EPROBE_DEFER", "%pe", ERR_PTR(-EPROBE_DEFER)); + test(kunittest, "(-EAGAIN)", "(%pe)", ERR_PTR(-EWOULDBLOCK)); + test(kunittest, "[-EIO ]", "[%-8pe]", ERR_PTR(-EIO)); + test(kunittest, "[ -EIO]", "[%8pe]", ERR_PTR(-EIO)); + test(kunittest, "-EPROBE_DEFER", "%pe", ERR_PTR(-EPROBE_DEFER)); #endif }
-static void __init -test_pointer(void) -{ - plain(); - null_pointer(); - error_pointer(); - invalid_pointer(); - symbol_ptr(); - kernel_ptr(); - struct_resource(); - addr(); - escaped_str(); - hex_string(); - mac(); - ip(); - uuid(); - dentry(); - struct_va_format(); - time_and_date(); - struct_clk(); - bitmap(); - netdev_features(); - flags(); - errptr(); - fwnode_pointer(); -} - -static void __init selftest(void) +static void +test_pointer(struct kunit *kunittest) +{ + plain(kunittest); + null_pointer(kunittest); + error_pointer(kunittest); + invalid_pointer(kunittest); + symbol_ptr(kunittest); + kernel_ptr(kunittest); + struct_resource(kunittest); + addr(kunittest); + escaped_str(kunittest); + hex_string(kunittest); + mac(kunittest); + ip(kunittest); + uuid(kunittest); + dentry(kunittest); + struct_va_format(kunittest); + time_and_date(kunittest); + struct_clk(kunittest); + bitmap(kunittest); + netdev_features(kunittest); + flags(kunittest); + errptr(kunittest); + fwnode_pointer(kunittest); +} + +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); }
-KSTM_MODULE_LOADERS(test_printf); +static struct kunit_case printf_test_cases[] = { + KUNIT_CASE(selftest), + {} +}; + +static struct kunit_suite printf_test_suite = { + .name = "printf-kunit-test", + .test_cases = printf_test_cases, +}; + +kunit_test_suite(printf_test_suite); + MODULE_AUTHOR("Rasmus Villemoes linux@rasmusvillemoes.dk"); MODULE_LICENSE("GPL");
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.
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).
- 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?
-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.
BTW, does the framework have some kind of logic that ensures nobody runs the printf suite twice in parallel?
Rasmus
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.
On Mon 2020-08-17 09:06:32, 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.
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:
diff --git a/lib/printf_kunit.c b/lib/printf_kunit.c index 68ac5f9b8d28..1689dadd70a3 100644 --- a/lib/printf_kunit.c +++ b/lib/printf_kunit.c @@ -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-te...
"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.
On 21/08/2020 13.37, Petr Mladek wrote:
On Mon 2020-08-17 09:06:32, 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.
I had the same concern. I have tried it.
Thanks for doing that and reporting the results.
#> modprobe printf_kunitproduced 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:
diff --git a/lib/printf_kunit.c b/lib/printf_kunit.c index 68ac5f9b8d28..1689dadd70a3 100644 --- a/lib/printf_kunit.c +++ b/lib/printf_kunit.c @@ -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.
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
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.
I am also a bit scared by the following note at https://www.kernel.org/doc/html/latest/dev-tools/kunit/start.html#running-te...
"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
On Fri, Aug 21, 2020 at 5:19 AM Rasmus Villemoes linux@rasmusvillemoes.dk 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.
On 21/08/2020 13.37, Petr Mladek wrote:
On Mon 2020-08-17 09:06:32, 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.
I had the same concern. I have tried it.
Thanks for doing that and reporting the results.
#> modprobe printf_kunitproduced 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:
diff --git a/lib/printf_kunit.c b/lib/printf_kunit.c index 68ac5f9b8d28..1689dadd70a3 100644 --- a/lib/printf_kunit.c +++ b/lib/printf_kunit.c @@ -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.
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].
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'
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.
I am also a bit scared by the following note at https://www.kernel.org/doc/html/latest/dev-tools/kunit/start.html#running-te...
"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.
On Mon, Oct 12, 2020 at 1:13 PM Brendan Higgins brendanhiggins@google.com wrote:
On Fri, Aug 21, 2020 at 5:19 AM Rasmus Villemoes linux@rasmusvillemoes.dk 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.
On 21/08/2020 13.37, Petr Mladek wrote:
On Mon 2020-08-17 09:06:32, 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.
I had the same concern. I have tried it.
Thanks for doing that and reporting the results.
#> modprobe printf_kunitproduced 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:
diff --git a/lib/printf_kunit.c b/lib/printf_kunit.c index 68ac5f9b8d28..1689dadd70a3 100644 --- a/lib/printf_kunit.c +++ b/lib/printf_kunit.c @@ -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.
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].
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'
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.
I am also a bit scared by the following note at https://www.kernel.org/doc/html/latest/dev-tools/kunit/start.html#running-te...
"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.
And I forgot the link to KTAP[1]. I am really batting a thousand here.
[1] https://lore.kernel.org/linux-kselftest/CY4PR13MB1175B804E31E502221BC8163FD8...
On Fri, Aug 21, 2020 at 01:37:10PM +0200, Petr Mladek wrote:
On Mon 2020-08-17 09:06:32, 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.
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.
On Fri, Aug 21, 2020 at 03:28:49PM +0300, Andy Shevchenko wrote:
On Fri, Aug 21, 2020 at 01:37:10PM +0200, Petr Mladek wrote:
On Mon 2020-08-17 09:06:32, 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.
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].
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.
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?
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/CY4PR13MB1175B804E31E502221BC8163FD8...
On 13/10/20 2:16 am, Brendan Higgins wrote:
On Fri, Aug 21, 2020 at 03:28:49PM +0300, Andy Shevchenko wrote:
On Fri, Aug 21, 2020 at 01:37:10PM +0200, Petr Mladek wrote:
On Mon 2020-08-17 09:06:32, 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.
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].
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.
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?
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.
[1] https://lore.kernel.org/linux-kselftest/CY4PR13MB1175B804E31E502221BC8163FD8...
On 12/10/2020 22.46, Brendan Higgins wrote:
On Fri, Aug 21, 2020 at 03:28:49PM +0300, Andy Shevchenko wrote:
On Fri, Aug 21, 2020 at 01:37:10PM +0200, Petr Mladek wrote:
On Mon 2020-08-17 09:06:32, 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.
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.
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.
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?
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
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-tes... 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 lkp@intel.com
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@lists.01.org
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-tes... 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 lkp@intel.com
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@lists.01.org
linux-kselftest-mirror@lists.linaro.org
-
Andy Shevchenko -
Arpitha Raghunandan -
Brendan Higgins -
kernel test robot -
Petr Mladek -
Rasmus Villemoes