On Mon, Feb 10, 2025 at 8:01 AM Rasmus Villemoes <li...@rasmusvillemoes.dk> wrote: > > On Fri, Feb 07 2025, Tamir Duberstein <tam...@gmail.com> wrote: > > [...] > > If/when you do re-roll a v3, can you split the defconfig changes off to > a separate patch? It's a little annoying to scroll through all those > mechanical one-liner diffs to get to the actual changes.
Yep. I'll split them into one for m68k and another for powerpc. Geert, I'll move your Acked-by to the m68k patch. > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > > index 1af972a92d06..4834cac1eb8f 100644 > > --- a/lib/Kconfig.debug > > +++ b/lib/Kconfig.debug > > @@ -2427,6 +2427,23 @@ config ASYNC_RAID6_TEST > > config TEST_HEXDUMP > > tristate "Test functions located in the hexdump module at runtime" > > > > +config PRINTF_KUNIT_TEST > > + tristate "KUnit test printf() family of functions at runtime" if > > !KUNIT_ALL_TESTS > > + depends on KUNIT > > + default KUNIT_ALL_TESTS > > + help > > + Enable this option to test the printf functions at boot. > > + > > Well, not necessarily at boot unless built-in? > > > + KUnit tests run during boot and output the results to the debug log > > + in TAP format (http://testanything.org/). Only useful for kernel > > devs > > + running the KUnit test harness, and not intended for inclusion into > > a > > + production build. > > + > > + For more information on KUnit and unit tests in general please refer > > + to the KUnit documentation in Documentation/dev-tools/kunit/. > > + > > + If unsure, say N. > > + > > I see that this is copy-pasted from other kunit config items, but not > all of them have all this boilerplate, and I don't think it's useful > (and, the mentions of "at boot" and "during boot" are actively > misleading). Other kunit config items are shorter and more to the point, > e.g. > > Enable this to turn on 'list_sort()' function test. This test is > executed only once during system boot (so affects only boot time), > or at module load time. > > If unsure, say N. > Very good point. Truthfully this boilerplate is a result of checkpatch.pl nagging about this paragraph being a certain length. I'll drop it and just write "Enable this option to test the printf functions at runtime.". > > > > obj-$(CONFIG_TEST_BITMAP) += test_bitmap.o > > diff --git a/lib/test_printf.c b/lib/printf_kunit.c > > similarity index 89% > > rename from lib/test_printf.c > > rename to lib/printf_kunit.c > > index 59dbe4f9a4cb..fe6f4df92dd5 100644 > > --- a/lib/test_printf.c > > +++ b/lib/printf_kunit.c > > @@ -5,7 +5,7 @@ > > > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > > > -#include <linux/init.h> > > +#include <kunit/test.h> > > #include <linux/kernel.h> > > #include <linux/module.h> > > #include <linux/printk.h> > > @@ -25,8 +25,6 @@ > > > > #include <linux/property.h> > > > > -#include "../tools/testing/selftests/kselftest_module.h" > > - > > #define BUF_SIZE 256 > > #define PAD_SIZE 16 > > #define FILL_CHAR '$' > > @@ -37,72 +35,71 @@ > > block \ > > __diag_pop(); > > > > -KSTM_MODULE_GLOBALS(); > > +static char *test_buffer; > > +static char *alloced_buffer; > > + > > +static struct kunit *kunittest; > > > > -static char *test_buffer __initdata; > > -static char *alloced_buffer __initdata; > > +#define tc_fail(fmt, ...) \ > > + KUNIT_FAIL(kunittest, fmt, ##__VA_ARGS__) > > > > -static int __printf(4, 0) __init > > +static void __printf(4, 0) > > do_test(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", > > + tc_fail("vsnprintf(buf, %d, \"%s\", ...) returned %d, > > expected %d\n", > > bufsize, fmt, ret, elen); > > - return 1; > > + return; > > } > > > > if (memchr_inv(alloced_buffer, FILL_CHAR, PAD_SIZE)) { > > - pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote before > > buffer\n", bufsize, fmt); > > - return 1; > > + tc_fail("vsnprintf(buf, %d, \"%s\", ...) wrote before > > buffer\n", bufsize, fmt); > > + return; > > } > > > > if (!bufsize) { > > if (memchr_inv(test_buffer, FILL_CHAR, BUF_SIZE + PAD_SIZE)) { > > - pr_warn("vsnprintf(buf, 0, \"%s\", ...) wrote to > > buffer\n", > > + tc_fail("vsnprintf(buf, 0, \"%s\", ...) wrote to > > buffer\n", > > fmt); > > - return 1; > > } > > - return 0; > > + return; > > } > > > > written = min(bufsize-1, elen); > > if (test_buffer[written]) { > > - pr_warn("vsnprintf(buf, %d, \"%s\", ...) did not > > nul-terminate buffer\n", > > + tc_fail("vsnprintf(buf, %d, \"%s\", ...) did not > > nul-terminate buffer\n", > > bufsize, fmt); > > - return 1; > > + return; > > } > > > > if (memchr_inv(test_buffer + written + 1, FILL_CHAR, bufsize - > > (written + 1))) { > > - pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote beyond the > > nul-terminator\n", > > + tc_fail("vsnprintf(buf, %d, \"%s\", ...) wrote beyond the > > nul-terminator\n", > > bufsize, fmt); > > - return 1; > > + return; > > } > > > > if (memchr_inv(test_buffer + bufsize, FILL_CHAR, BUF_SIZE + PAD_SIZE > > - bufsize)) { > > - pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote beyond > > buffer\n", bufsize, fmt); > > - return 1; > > + tc_fail("vsnprintf(buf, %d, \"%s\", ...) wrote beyond > > buffer\n", bufsize, fmt); > > + return; > > } > > > > if (memcmp(test_buffer, expect, written)) { > > - pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote '%s', expected > > '%.*s'\n", > > + tc_fail("vsnprintf(buf, %d, \"%s\", ...) wrote '%s', expected > > '%.*s'\n", > > bufsize, fmt, test_buffer, written, expect); > > - return 1; > > + return; > > } > > - return 0; > > } > > > > -static void __printf(3, 4) __init > > +static void __printf(3, 4) > > __test(const char *expect, int elen, const char *fmt, ...) > > { > > va_list ap; > > @@ -110,9 +107,8 @@ __test(const char *expect, int elen, const char *fmt, > > ...) > > 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++; > > + tc_fail("error in test suite: expected output length %d too > > long. Format was '%s'.\n", > > + elen, fmt); > > return; > > } > > > > @@ -124,19 +120,17 @@ __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(BUF_SIZE, expect, elen, fmt, ap); > > rand = get_random_u32_inclusive(1, 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(rand, expect, elen, fmt, ap); > > + do_test(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", > > + tc_fail("kvasprintf(..., \"%s\", ...) returned '%s', > > expected '%s'\n", > > fmt, p, expect); > > - failed_tests++; > > } > > kfree(p); > > } > > So reading through this, is there really any reason to drop those > existing total_tests and failed_tests tallies? Since you do need to keep > the control flow the same, leaving the return types and "return > 1"/"return 0" and their uses in updating failed_tests would also reduce > the patch size quite significantly. > > So they no longer come from some KSTM_MODULE_GLOBALS(), and thus need to > be explicitly declared in this module, but that's exactly how it was > originally until someone decided to lift that from the the printf suite > to kstm. > > Yes, they'd need to be explicitly initialized to 0 during kunit_init > since they are no longer use-once __initdata variable, and some > kunit_exit logic would need to "manually" report them. If I understand your concern correctly, you only really care about `total_tests`, right? I think it's slightly unidiomatic to count tests outside the framework this way but it's not a hill I'll die on. Just to elaborate on why I think this unidiomatic: the KUnit test runner script that produces human-readable output doesn't emit logs unless something fails. In the success case, you'd get (before the next patch that breaks this into many tests): [09:34:15] ==================== printf (1 subtest) ==================== [09:34:15] [PASSED] printf_test [09:34:15] ===================== [PASSED] printf ====================== [09:34:15] ============================================================ [09:34:15] Testing complete. Ran 1 tests: passed: 1 but the raw output does contain the tally: KTAP version 1 1..1 KTAP version 1 # Subtest: printf # module: printf_kunit 1..1 ok 1 printf_test # printf: ran 448 tests ok 1 printf So assuming you're OK with this compromise, I'll go ahead and spin v3. Thanks for the review!
