linux-kernel - Re: [PATCH v8 4/4] scanf: break kunit into test cases

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z8hnXIrMV0ct1YR6@pathway.suse.cz>
Date: Wed, 5 Mar 2025 16:01:48 +0100
From: Petr Mladek <pmladek@...e.com>
To: Tamir Duberstein <tamird@...il.com>
Cc: 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>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v8 4/4] scanf: break kunit into test cases

On Fri 2025-02-14 11:20:01, Tamir Duberstein wrote:
> Use `suite_init` and move some tests into `scanf_test_cases`. This
> gives us nicer output in the event of a failure.
> 
> Reviewed-by: David Gow <davidgow@...gle.com>
> Signed-off-by: Tamir Duberstein <tamird@...il.com>
> ---
>  lib/tests/scanf_kunit.c | 95 ++++++++++++++++++++++++++-----------------------
>  1 file changed, 51 insertions(+), 44 deletions(-)
> 
> diff --git a/lib/tests/scanf_kunit.c b/lib/tests/scanf_kunit.c
> index 3bbad9ebe437..fa215a7db366 100644
> --- a/lib/tests/scanf_kunit.c
> +++ b/lib/tests/scanf_kunit.c
> @@ -4,14 +4,10 @@
>   */
>  
>  #include <kunit/test.h>
> -#include <linux/bitops.h>
> -#include <linux/kernel.h>
>  #include <linux/module.h>
> -#include <linux/overflow.h>
> -#include <linux/printk.h>
>  #include <linux/prandom.h>
>  #include <linux/slab.h>
> -#include <linux/string.h>
> +#include <linux/sprintf.h>
>  
>  #define BUF_SIZE 1024

It would make more sense to do this clean up in the 3rd patch
where some code was replaced by the kunit macros.

Also I am not sure about the choice. It might make sense to remove
<include/printk.h> because the pr_*() calls were removed.
But what about the others? Did anyone request the clean up, please?

I do not want to open a bike shadding because different people
have different opinion.

I would personally prefer to keep the explicit includes when the
related API is still used. It helps to optimize nested includes
in the header files which helps to speedup build. AFAIK, there
are people working in this optimization and they might need
to revert this change.

> @@ -50,10 +46,9 @@ do {										\
>  	for (; n_args > 0; n_args--, expect++) {				\
>  		typeof(*expect) got = *va_arg(ap, typeof(expect));		\
>  		if (got != *expect) {						\
> -			KUNIT_FAIL(test,					\
> -				   "%s:%d: vsscanf(\"%s\", \"%s\", ...) expected " arg_fmt " got " arg_fmt, \
> -				   file, line, str, fmt, *expect, got);		\
> -			return;							\
> +			KUNIT_FAIL_AND_ABORT(test,				\
> +					     "%s:%d: vsscanf(\"%s\", \"%s\", ...) expected " arg_fmt " got " arg_fmt, \
> +					     file, line, str, fmt, *expect, got); \

I am just curious. Is there any particular reason why
KUNIT_FAIL() is replaced with KUNIT_FAIL_AND_ABORT()?

Did the move of some tests into KUNIT_CASE() increased the number of
reported errors?

Why is _ABORT() variant used in _check_numbers_template() and not in _test()?

I do not have strong opinion. The change just looks a bit ad-hoc and
inconsistent.

>  		}								\
>  	}									\
>  } while (0)

Otherwise, the change looks good to me.

Best Regards,
Petr

PS: I suggest to wait at least one or two days with the respin. Other
    reviewers might want to add their own opinion.