On Thu, Mar 6, 2025 at 7:25 AM Petr Mladek <pmladek@xxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxx>
> > 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!
