On Fri 2025-02-21 15:34:31, Tamir Duberstein wrote:
> Move all tests into `printf_test_cases`. This gives us nicer output in
> the event of a failure.
>
> Combine `plain_format` and `plain_hash` into `hash_pointer` since
> they're testing the same scenario.
>
> Signed-off-by: Tamir Duberstein <tamird@xxxxxxxxx>
> ---
> lib/tests/printf_kunit.c | 331 +++++++++++++++++------------------------------
> 1 file changed, 121 insertions(+), 210 deletions(-)
>
> diff --git a/lib/tests/printf_kunit.c b/lib/tests/printf_kunit.c
> index 287bbfb61148..013df6f6dd49 100644
> --- a/lib/tests/printf_kunit.c
> +++ b/lib/tests/printf_kunit.c
> @@ -38,13 +38,8 @@ static unsigned int total_tests;
> static char *test_buffer;
> static char *alloced_buffer;
>
> -static struct kunit *kunittest;
> -
> -#define tc_fail(fmt, ...) \
> - KUNIT_FAIL(kunittest, fmt, ##__VA_ARGS__)
> -
> -static void __printf(4, 0)
> -do_test(int bufsize, const char *expect, int elen,
> +static void __printf(5, 0)
> +do_test(struct kunit *kunittest, int bufsize, const char *expect, int elen,
> const char *fmt, va_list ap)
> {
> va_list aq;
> @@ -58,59 +53,64 @@ do_test(int bufsize, const char *expect, int elen,
[...]
>
> if (memcmp(test_buffer, expect, written)) {
> - tc_fail("vsnprintf(buf, %d, \"%s\", ...) wrote '%s', expected '%.*s'",
> - bufsize, fmt, test_buffer, written, expect);
> + KUNIT_FAIL(kunittest, "vsnprintf(buf, %d, \"%s\", ...) wrote '%s', expected '%.*s'",
> + bufsize, fmt, test_buffer, written, expect);
> return;
> }
> }
>
> -static void __printf(3, 4)
> -__test(const char *expect, int elen, const char *fmt, ...)
> +static void __printf(4, 0)
This should be:
static void __printf(4, 5)
The 2nd parameter is zero when the variable list of parameters is
passed using va_list.
> +__test(struct kunit *kunittest, const char *expect, int elen, const char *fmt, ...)
> {
> va_list ap;
> int rand;
> char *p;
> @@ -247,89 +225,44 @@ plain_format(void)
> #define ZEROS ""
> #define ONES ""
>
> -static int
> -plain_format(void)
> -{
> - /* Format is implicitly tested for 32 bit machines by plain_hash() */
> - return 0;
> -}
> -
> #endif /* BITS_PER_LONG == 64 */
>
> -static int
> -plain_hash_to_buffer(const void *p, char *buf, size_t len)
> +static void
> +plain_hash_to_buffer(struct kunit *kunittest, const void *p, char *buf, size_t len)
> {
> - int nchars;
> -
> - nchars = snprintf(buf, len, "%p", p);
> -
> - if (nchars != PTR_WIDTH)
> - return -1;
> + KUNIT_ASSERT_EQ(kunittest, snprintf(buf, len, "%p", p), PTR_WIDTH);
>
> if (strncmp(buf, PTR_VAL_NO_CRNG, PTR_WIDTH) == 0) {
> kunit_warn(kunittest, "crng possibly not yet initialized. plain 'p' buffer contains \"%s\"",
> PTR_VAL_NO_CRNG);
> - return 0;
> }
> -
> - return 0;
> }
>
> -static int
> -plain_hash(void)
> -{
> - char buf[PLAIN_BUF_SIZE];
> - int ret;
> -
> - ret = plain_hash_to_buffer(PTR, buf, PLAIN_BUF_SIZE);
> - if (ret)
> - return ret;
> -
> - if (strncmp(buf, PTR_STR, PTR_WIDTH) == 0)
> - return -1;
> -
> - return 0;
> -}
> -
> -/*
> - * We can't use test() to test %p because we don't know what output to expect
> - * after an address is hashed.
> - */
> static void
> -plain(void)
> +hash_pointer(struct kunit *kunittest)
> {
> - int err;
> + if (no_hash_pointers)
> + kunit_skip(kunittest, "hash pointers disabled");
>
> - if (no_hash_pointers) {
> - kunit_warn(kunittest, "skipping plain 'p' tests");
> - return;
> - }
> + char buf[PLAIN_BUF_SIZE];
>
> - err = plain_hash();
> - if (err) {
> - tc_fail("plain 'p' does not appear to be hashed");
> - return;
> - }
> + plain_hash_to_buffer(kunittest, PTR, buf, PLAIN_BUF_SIZE);
>
> - err = plain_format();
> - if (err) {
> - tc_fail("hashing plain 'p' has unexpected format");
> - }
> + /*
> + * We can't use test() to test %p because we don't know what output to expect
> + * after an address is hashed.
> + */
The code does not longer print a reasonable error message on failure.
I would extend the comment to make it easier to understand the
meaning. Also I would use the imperative style. Something like:
/*
* The hash of %p is unpredictable, therefore test() cannot be used.
* Instead, verify that the first 32 bits are zeros on a 64-bit system,
* and confirm the non-hashed value is not printed.
*/
> +
> + KUNIT_EXPECT_MEMEQ(kunittest, buf, ZEROS, strlen(ZEROS));
> + KUNIT_EXPECT_MEMNEQ(kunittest, buf+strlen(ZEROS), PTR_STR, PTR_WIDTH);
This looks wrong. It should be either:
KUNIT_EXPECT_MEMNEQ(kunittest, buf, PTR_STR, PTR_WIDTH);
or
KUNIT_EXPECT_MEMNEQ(kunittest,
buf + strlen(ZEROS),
PTR_STR + strlen(ZEROS),
PTR_WIDTH - strlen(ZEROS));
I would use the 1st variant. It is easier and it works the same way
as the original check.
Anyway, it is a great clean up of the pointer tests. I have wanted to do it
since a long time but I never found time.
> }
>
> static void
> -test_hashed(const char *fmt, const void *p)
> +test_hashed(struct kunit *kunittest, const char *fmt, const void *p)
> {
> char buf[PLAIN_BUF_SIZE];
> - int ret;
>
> - /*
> - * No need to increase failed test counter since this is assumed
> - * to be called after plain().
> - */
> - ret = plain_hash_to_buffer(p, buf, PLAIN_BUF_SIZE);
> - if (ret)
> - return;
> + plain_hash_to_buffer(kunittest, p, buf, PLAIN_BUF_SIZE);
>
> test(buf, fmt, p);
> }
> @@ -739,11 +664,9 @@ flags(void)
> (unsigned long) gfp);
> gfp |= __GFP_HIGH;
> test(cmp_buffer, "%pGg", &gfp);
> -
> - kfree(cmp_buffer);
I belive that the kfree() should stay. Otherwise, the test leaks memory
in every run.
> }
>
> -static void fwnode_pointer(void)
> +static void fwnode_pointer(struct kunit *kunittest)
> {
> const struct software_node first = { .name = "first" };
> const struct software_node second = { .name = "second", .parent = &first };
Otherwise, it looks good to me.
Best Regards,
Petr
