Message-ID: <CAJ-ks9nFSzRXFauavzSWhvhr2Rou7qqkWi_LZ=4e1Tyr4_bn3g@mail.gmail.com>
Date: Thu, 6 Mar 2025 09:25:43 -0500
From: Tamir Duberstein <tamird@...il.com>
To: Petr Mladek <pmladek@...e.com>
Cc: Arpitha Raghunandan <98.arpi@...il.com>, David Gow <davidgow@...gle.com>, 
	Steven Rostedt <rostedt@...dmis.org>, Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 
	Rasmus Villemoes <linux@...musvillemoes.dk>, Sergey Senozhatsky <senozhatsky@...omium.org>, 
	Andrew Morton <akpm@...ux-foundation.org>, Shuah Khan <shuah@...nel.org>, 
	Jonathan Corbet <corbet@....net>, Geert Uytterhoeven <geert@...ux-m68k.org>, 
	Madhavan Srinivasan <maddy@...ux.ibm.com>, Michael Ellerman <mpe@...erman.id.au>, 
	Nicholas Piggin <npiggin@...il.com>, Christophe Leroy <christophe.leroy@...roup.eu>, 
	Naveen N Rao <naveen@...nel.org>, Brendan Higgins <brendan.higgins@...ux.dev>, 
	linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org, 
	linux-doc@...r.kernel.org, linux-m68k@...ts.linux-m68k.org, 
	linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v5 1/3] printf: convert self-test to KUnit

On Thu, Mar 6, 2025 at 7:25 AM Petr Mladek <pmladek@...e.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@...il.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@...omium.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!

Powered by blists - more mailing lists