Re: [PATCH v2 1/2] printf: convert self-test to KUnit

On Fri, Feb 07 2025, Tamir Duberstein tamird@gmail.com wrote:

Convert the printf() self-test to a KUnit test.

In the interest of keeping the patch reasonably-sized this doesn't refactor the tests into proper parameterized tests - it's all one big test case.

Acked-by: Geert Uytterhoeven geert@linux-m68k.org # m68k Signed-off-by: Tamir Duberstein tamird@gmail.com

---

Documentation/core-api/printk-formats.rst | 2 +- MAINTAINERS | 2 +- arch/m68k/configs/amiga_defconfig | 1 - arch/m68k/configs/apollo_defconfig | 1 - arch/m68k/configs/atari_defconfig | 1 - arch/m68k/configs/bvme6000_defconfig | 1 - arch/m68k/configs/hp300_defconfig | 1 - arch/m68k/configs/mac_defconfig | 1 - arch/m68k/configs/multi_defconfig | 1 - arch/m68k/configs/mvme147_defconfig | 1 - arch/m68k/configs/mvme16x_defconfig | 1 - arch/m68k/configs/q40_defconfig | 1 - arch/m68k/configs/sun3_defconfig | 1 - arch/m68k/configs/sun3x_defconfig | 1 - arch/powerpc/configs/ppc64_defconfig | 1 - lib/Kconfig.debug | 20 +++- lib/Makefile | 2 +- lib/{test_printf.c => printf_kunit.c} | 164 +++++++++++++++--------------- tools/testing/selftests/lib/config | 1 - tools/testing/selftests/lib/printf.sh | 4 - 20 files changed, 104 insertions(+), 104 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst index ecccc0473da9..0d9461bd6964 100644 --- a/Documentation/core-api/printk-formats.rst +++ b/Documentation/core-api/printk-formats.rst @@ -661,7 +661,7 @@ Do *not* use it from C. Thanks ====== -If you add other %p extensions, please extend <lib/test_printf.c> with +If you add other %p extensions, please extend <lib/printf_kunit.c> with one or more test cases, if at all feasible. Thank you for your cooperation and attention. diff --git a/MAINTAINERS b/MAINTAINERS index 896a307fa065..57e366d356bd 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -25368,7 +25368,7 @@ R: Sergey Senozhatsky senozhatsky@chromium.org S: Maintained T: git git://git.kernel.org/pub/scm/linux/kernel/git/printk/linux.git F: Documentation/core-api/printk-formats.rst -F: lib/test_printf.c +F: lib/printf_kunit.c F: lib/test_scanf.c F: lib/vsprintf.c diff --git a/arch/m68k/configs/amiga_defconfig b/arch/m68k/configs/amiga_defconfig index dbf2ea561c85..e8c5e08fb6ec 100644 --- a/arch/m68k/configs/amiga_defconfig +++ b/arch/m68k/configs/amiga_defconfig @@ -622,7 +622,6 @@ CONFIG_ATOMIC64_SELFTEST=m CONFIG_ASYNC_RAID6_TEST=m CONFIG_TEST_HEXDUMP=m CONFIG_TEST_KSTRTOX=m -CONFIG_TEST_PRINTF=m CONFIG_TEST_SCANF=m CONFIG_TEST_BITMAP=m CONFIG_TEST_UUID=m

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.

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.

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.

Rasmus