On Thu 2025-03-06 09:25:43, Tamir Duberstein wrote:
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.
--- a/lib/test_printf.c
+++ b/lib/tests/printf_kunit.c
@@ -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.
I see. It might have been useful when the patch removed the trailing '\n'.
But you are going to add it back. So there won't be any hidden change.
So I would prefer to go straight to KUNIT_FAIL().
@@ -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).
I see. KUNIT_ASSERT_NOT_NULL() can't be used in the .suite_exit() callback.
So I'd prefer to leave this as is.
I agree to leave this as is.
Best Regards,
Petr
