| Message ID | 20250214-scanf-kunit-convert-v8-4-5ea50f95f83c@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scanf: convert self-test to KUnit | expand |
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@google.com> > Signed-off-by: Tamir Duberstein <tamird@gmail.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.
On Wed, Mar 05, 2025 at 04:01:48PM +0100, Petr Mladek wrote: > On Fri 2025-02-14 11:20:01, Tamir Duberstein wrote: ... > > #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. +1. > 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? Header inclusions is a pain point to me in the kernel. Esp. misuse of kernel.h or other headers to behave like a "proxy". If no-one even asked for a cleanup it's always good to follow IWYU principle as you mentioned below. > 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. +1.
On Wed, Mar 5, 2025 at 10:55 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Mar 05, 2025 at 10:25:51AM -0500, Tamir Duberstein wrote: > > On Wed, Mar 5, 2025 at 10:01 AM Petr Mladek <pmladek@suse.com> wrote: > > > On Fri 2025-02-14 11:20:01, Tamir Duberstein wrote: > > ... > > > > > #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. > > > > Yeah, I don't feel strongly. I'll just restore all the includes. > > It will be blind approach. Please, try to look at them closely and include what > you use (IWYU principle). I don't think anybody uses kernel.h here, for > example. > > -- > With Best Regards, > Andy Shevchenko I think I'm getting conflicting instructions here. IWYU is indeed what I did: bitops, kernel, overflow, printk are all unused; string is used only for sprintf, so I made that replacement. However Petr said "Did anyone request the clean up, please?" which implies to me an aversion to unwanted cleanup. So, which is it please?
On Wed, Mar 05, 2025 at 10:57:47AM -0500, Tamir Duberstein wrote: > On Wed, Mar 5, 2025 at 10:55 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Mar 05, 2025 at 10:25:51AM -0500, Tamir Duberstein wrote: > > > On Wed, Mar 5, 2025 at 10:01 AM Petr Mladek <pmladek@suse.com> wrote: > > > > On Fri 2025-02-14 11:20:01, Tamir Duberstein wrote: ... > > > > > #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. > > > > > > Yeah, I don't feel strongly. I'll just restore all the includes. > > > > It will be blind approach. Please, try to look at them closely and include what > > you use (IWYU principle). I don't think anybody uses kernel.h here, for > > example. > > I think I'm getting conflicting instructions here. IWYU is indeed what > I did: bitops, kernel, overflow, printk are all unused; string is used > only for sprintf, so I made that replacement. > > However Petr said "Did anyone request the clean up, please?" which > implies to me an aversion to unwanted cleanup. So, which is it please? I believe he asks the background of the change. And if it made in a separate patch it would be clearer to begin with (e.g., Suggested-by tag). But I don't know how you deducted that it's unwanted. With a separate patch we may discuss and see if it's wanted or not. In any case I would like to see such a patch.
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 @@ -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); \ } \ } \ } while (0) @@ -435,8 +430,11 @@ static void numbers_list_hh(struct kunit *test, const char *delim) numbers_list_8(signed char, "0x%hhx", delim, "hhi", check_char); } -static void numbers_list(struct kunit *test, const char *delim) +static void numbers_list(struct kunit *test) { + const char * const *param = test->param_value; + const char *delim = *param; + numbers_list_ll(test, delim); numbers_list_l(test, delim); numbers_list_d(test, delim); @@ -507,8 +505,11 @@ static void numbers_list_field_width_hh(struct kunit *test, const char *delim) * List of numbers separated by delim. Each field width specifier is the * maximum possible digits for the given type and base. */ -static void numbers_list_field_width_typemax(struct kunit *test, const char *delim) +static void numbers_list_field_width_typemax(struct kunit *test) { + const char * const *param = test->param_value; + const char *delim = *param; + numbers_list_field_width_ll(test, delim); numbers_list_field_width_l(test, delim); numbers_list_field_width_d(test, delim); @@ -570,8 +571,11 @@ static void numbers_list_field_width_val_hh(struct kunit *test, const char *deli * List of numbers separated by delim. Each field width specifier is the * exact length of the corresponding value digits in the string being scanned. */ -static void numbers_list_field_width_val_width(struct kunit *test, const char *delim) +static void numbers_list_field_width_val_width(struct kunit *test) { + const char * const *param = test->param_value; + const char *delim = *param; + numbers_list_field_width_val_ll(test, delim); numbers_list_field_width_val_l(test, delim); numbers_list_field_width_val_d(test, delim); @@ -587,7 +591,12 @@ static void numbers_list_field_width_val_width(struct kunit *test, const char *d */ static void numbers_slice(struct kunit *test) { - numbers_list_field_width_val_width(test, ""); + const char *delim = ""; + + KUNIT_ASSERT_PTR_EQ(test, test->param_value, NULL); + test->param_value = &delim; + + numbers_list_field_width_val_width(test); } #define test_number_prefix(T, str, scan_fmt, expect0, expect1, n_args, fn) \ @@ -738,62 +747,60 @@ static const char * const number_delimiters[] = { " ", ":", ",", "-", "/", }; -static void test_numbers(struct kunit *test) +static void number_delimiter_param_desc(const char * const *param, + char *desc) { - int i; + snprintf(desc, KUNIT_PARAM_DESC_SIZE, "\"%s\"", *param); +} - /* String containing only one number. */ - numbers_simple(test); +KUNIT_ARRAY_PARAM(number_delimiters, number_delimiters, number_delimiter_param_desc); +static struct kunit_case scanf_test_cases[] = { + KUNIT_CASE(numbers_simple), /* String with multiple numbers separated by delimiter. */ - for (i = 0; i < ARRAY_SIZE(number_delimiters); i++) { - numbers_list(test, number_delimiters[i]); - - /* Field width may be longer than actual field digits. */ - numbers_list_field_width_typemax(test, number_delimiters[i]); - - /* Each field width exactly length of actual field digits. */ - numbers_list_field_width_val_width(test, number_delimiters[i]); - } - + KUNIT_CASE_PARAM(numbers_list, number_delimiters_gen_params), + /* Field width may be longer than actual field digits. */ + KUNIT_CASE_PARAM(numbers_list_field_width_typemax, number_delimiters_gen_params), + /* Each field width exactly length of actual field digits. */ + KUNIT_CASE_PARAM(numbers_list_field_width_val_width, number_delimiters_gen_params), /* Slice continuous sequence of digits using field widths. */ - numbers_slice(test); + KUNIT_CASE(numbers_slice), + KUNIT_CASE(numbers_prefix_overflow), - numbers_prefix_overflow(test); -} + KUNIT_CASE(test_simple_strtoull), + KUNIT_CASE(test_simple_strtoll), + KUNIT_CASE(test_simple_strtoul), + KUNIT_CASE(test_simple_strtol), + {} +}; -static void scanf_test(struct kunit *test) +static int scanf_suite_init(struct kunit_suite *suite) { test_buffer = kmalloc(BUF_SIZE, GFP_KERNEL); if (!test_buffer) - return; + return -ENOMEM; fmt_buffer = kmalloc(BUF_SIZE, GFP_KERNEL); if (!fmt_buffer) { kfree(test_buffer); - return; + return -ENOMEM; } prandom_seed_state(&rnd_state, 3141592653589793238ULL); - test_numbers(test); - - test_simple_strtoull(test); - test_simple_strtoll(test); - test_simple_strtoul(test); - test_simple_strtol(test); + return 0; +} +static void scanf_suite_exit(struct kunit_suite *suite) +{ kfree(fmt_buffer); kfree(test_buffer); } -static struct kunit_case scanf_test_cases[] = { - KUNIT_CASE(scanf_test), - {} -}; - static struct kunit_suite scanf_test_suite = { .name = "scanf", + .suite_init = scanf_suite_init, + .suite_exit = scanf_suite_exit, .test_cases = scanf_test_cases, };