Re: [PATCH v5 1/3] printf: convert self-test to KUnit

On Thu, Mar 6, 2025 at 7:25 AM Petr Mladek pmladek@suse.com wrote:

On Fri 2025-02-21 15:34:30, Tamir Duberstein 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.

Signed-off-by: Tamir Duberstein tamird@gmail.com

Documentation/core-api/printk-formats.rst | 4 +- MAINTAINERS | 2 +- lib/Kconfig.debug | 12 +- lib/Makefile | 1 - lib/tests/Makefile | 1 + lib/{test_printf.c => tests/printf_kunit.c} | 188 +++++++++++++++------------- tools/testing/selftests/lib/config | 1 - tools/testing/selftests/lib/printf.sh | 4 - 8 files changed, 117 insertions(+), 96 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst index ecccc0473da9..4bdc394e86af 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 -one or more test cases, if at all feasible. +If you add other %p extensions, please extend <lib/tests/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 f076360ce3c6..b051ccf6b276 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -25510,8 +25510,8 @@ 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/test_scanf.c +F: lib/tests/printf_kunit.c F: lib/vsprintf.c

VT1211 HARDWARE MONITOR DRIVER diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 7ddbfdacf895..d2b15f633227 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -2436,6 +2436,15 @@ 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 runtime.
  

•   If unsure, say N.
  

config STRING_KUNIT_TEST tristate "KUnit test string functions at runtime" if !KUNIT_ALL_TESTS depends on KUNIT @@ -2449,9 +2458,6 @@ config STRING_HELPERS_KUNIT_TEST config TEST_KSTRTOX tristate "Test kstrto*() family of functions at runtime"

-config TEST_PRINTF

• tristate "Test printf() family of functions at runtime"
  

config TEST_SCANF tristate "Test scanf() family of functions at runtime"

diff --git a/lib/Makefile b/lib/Makefile index 961aef91d493..f31e6a3100ba 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -77,7 +77,6 @@ obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o obj-$(CONFIG_TEST_DYNAMIC_DEBUG) += test_dynamic_debug.o -obj-$(CONFIG_TEST_PRINTF) += test_printf.o obj-$(CONFIG_TEST_SCANF) += test_scanf.o

obj-$(CONFIG_TEST_BITMAP) += test_bitmap.o diff --git a/lib/tests/Makefile b/lib/tests/Makefile index 8961fbcff7a4..183c6a838a5d 100644 --- a/lib/tests/Makefile +++ b/lib/tests/Makefile @@ -30,6 +30,7 @@ obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o obj-$(CONFIG_MEMCPY_KUNIT_TEST) += memcpy_kunit.o CFLAGS_overflow_kunit.o = $(call cc-disable-warning, tautological-constant-out-of-range-compare) obj-$(CONFIG_OVERFLOW_KUNIT_TEST) += overflow_kunit.o +obj-$(CONFIG_PRINTF_KUNIT_TEST) += printf_kunit.o obj-$(CONFIG_SIPHASH_KUNIT_TEST) += siphash_kunit.o obj-$(CONFIG_SLUB_KUNIT_TEST) += slub_kunit.o obj-$(CONFIG_TEST_SORT) += test_sort.o diff --git a/lib/test_printf.c b/lib/tests/printf_kunit.c similarity index 87% rename from lib/test_printf.c rename to lib/tests/printf_kunit.c index 59dbe4f9a4cb..287bbfb61148 100644 --- a/lib/test_printf.c +++ b/lib/tests/printf_kunit.c @@ -3,9 +3,7 @@

• Test cases for printf facility.

*/

-#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 +23,6 @@

#include <linux/property.h>

-#include "../tools/testing/selftests/kselftest_module.h"

#define BUF_SIZE 256 #define PAD_SIZE 16 #define FILL_CHAR '$' @@ -37,12 +33,17 @@ block \ __diag_pop();

-KSTM_MODULE_GLOBALS(); +static unsigned int total_tests;

+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) { @@ -57,52 +58,50 @@ do_test(int bufsize, const char *expect, int elen, 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",
  

1. It looks a bit strange that the 1st patch replaces pr_warn() with tc_fail() which hides KUNIT_FAIL().

   And the 2nd patch replaces tc_fail() with KUNIT_FAIL().

   It looks like a non-necessary churn.

   It would be better to avoid the temporary "tc_fail" and swith to KUNIT_FAIL() already in this patch.

   I did not find any comment about this in the earier versions of the patchset.

   Is it just a result of the evolution of the patchset or is there any motivation for this?

The motivation was to keep the width of the macro the same in this first patch for ease of review, particularly in the 7 instances where the invocation wraps to a second line. If you prefer I go straight to KUNIT_FAIL, I can make that change.

2. What was the motivation to remove the trailing '\n', please?

   It actually makes a difference from the printk() POV. Messages without the trailing '\n' are _not_ flushed to the console until another message is added. The reason is that they might still be appended by pr_cont(). And printk() emits only complete lines to the console.

   In general, messages should include the trailing '\n' unless the code wants to append something later or the trailing '\n' is added by another layer of the code. It does not seem to be this case.

...

                  bufsize, fmt, ret, elen);

•         return 1;
  

•         return;
  }
  

[...]

I noticed in my testing that the trailing \n didn't change the test output, but I didn't know the details you shared about the trailing \n. I'll restore them, unless we jump straight to the KUNIT macros per the discussion above.

...

@@ -842,13 +836,15 @@ test_pointer(void) fourcc_pointer(); }

-static void __init selftest(void) +static void printf_test(struct kunit *test) { alloced_buffer = kmalloc(BUF_SIZE + 2*PAD_SIZE, GFP_KERNEL); if (!alloced_buffer) return;

I would use here:

    KUNIT_ASSERT_NOT_NULL(test, alloced_buffer);

And move the same change for the other kmalloc() location from the 2nd patch.

I didn't do that here because I was trying to keep this patch as small as possible, and I wrote that in the commit message.

As for using KUNIT_ASSERT_NOT_NULL here, that would have to change back to an error return in the 2nd patch because this code moves into `suite_init`, which is called with `struct kunit_suite` rather than `struct kunit_test`, and KUnit assertion macros do not work with the former (and for good reason, because failures in suite setup cannot be attributed to a particular test case).

So I'd prefer to leave this as is.

...

  test_buffer = alloced_buffer + PAD_SIZE;

• kunittest = test;
  

• test_basic();
  test_number();
  test_string();
  

Otherwise, it looks good to me.

Best Regards, Petr

Thank you for the review!