| Message ID | 20250211-scanf-kunit-convert-v7-1-c057f0a3d9d8@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | scanf: convert self-test to KUnit | expand |
On Tue, Feb 11, 2025 at 10:13:37AM -0500, Tamir Duberstein wrote: > The test already prints the same information on failure; remove > redundant pr_debug() logs. ... > #define _check_numbers_template(arg_fmt, expect, str, fmt, n_args, ap) \ > do { \ > - pr_debug("\"%s\", \"%s\" ->\n", str, fmt); \ What *if* the n_args == 0 here? > for (; n_args > 0; n_args--, expect++) { \ > typeof(*expect) got = *va_arg(ap, typeof(expect)); \ > - pr_debug("\t" arg_fmt "\n", got); \ > if (got != *expect) { \ > pr_warn("vsscanf(\"%s\", \"%s\", ...) expected " arg_fmt " got " arg_fmt "\n", \ > str, fmt, *expect, got); \ > @@ -689,7 +687,6 @@ do { \ > total_tests++; \ > len = snprintf(test_buffer, BUF_SIZE, gen_fmt, expect); \ > got = (fn)(test_buffer, &endp, base); \ > - pr_debug(#fn "(\"%s\", %d) -> " gen_fmt "\n", test_buffer, base, got); \ > if (got != (expect)) { \ > fail = true; \ > pr_warn(#fn "(\"%s\", %d): got " gen_fmt " expected " gen_fmt "\n", \
On Tue, Feb 11, 2025 at 10:42 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Feb 11, 2025 at 10:13:37AM -0500, Tamir Duberstein wrote: > > The test already prints the same information on failure; remove > > redundant pr_debug() logs. > > ... > > > #define _check_numbers_template(arg_fmt, expect, str, fmt, n_args, ap) \ > > do { \ > > - pr_debug("\"%s\", \"%s\" ->\n", str, fmt); \ > > What *if* the n_args == 0 here? Then there's no assertion in this block, so the test cannot possibly fail here. > > for (; n_args > 0; n_args--, expect++) { \ > > typeof(*expect) got = *va_arg(ap, typeof(expect)); \ > > - pr_debug("\t" arg_fmt "\n", got); \ > > if (got != *expect) { \ > > pr_warn("vsscanf(\"%s\", \"%s\", ...) expected " arg_fmt " got " arg_fmt "\n", \ > > str, fmt, *expect, got); \ > > @@ -689,7 +687,6 @@ do { \ > > total_tests++; \ > > len = snprintf(test_buffer, BUF_SIZE, gen_fmt, expect); \ > > got = (fn)(test_buffer, &endp, base); \ > > - pr_debug(#fn "(\"%s\", %d) -> " gen_fmt "\n", test_buffer, base, got); \ > > if (got != (expect)) { \ > > fail = true; \ > > pr_warn(#fn "(\"%s\", %d): got " gen_fmt " expected " gen_fmt "\n", \ > > -- > With Best Regards, > Andy Shevchenko > >
On Tue, Feb 11, 2025 at 10:50:33AM -0500, Tamir Duberstein wrote: > On Tue, Feb 11, 2025 at 10:42 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Feb 11, 2025 at 10:13:37AM -0500, Tamir Duberstein wrote: > > > The test already prints the same information on failure; remove > > > redundant pr_debug() logs. ... > > > - pr_debug("\"%s\", \"%s\" ->\n", str, fmt); \ > > > > What *if* the n_args == 0 here? > > Then there's no assertion in this block, so the test cannot possibly fail here. Correct, but I'm talking about this in a scope of the removed debug print. I.o.w. how would we even know that this was the case? (I'm not objecting removal, what I want from you is to have a descriptive and explanatory commit message that's answers to "why is this needed?" and "why is it safe to do?")
On Tue, Feb 11, 2025 at 10:58 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Feb 11, 2025 at 10:50:33AM -0500, Tamir Duberstein wrote: > > On Tue, Feb 11, 2025 at 10:42 AM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Tue, Feb 11, 2025 at 10:13:37AM -0500, Tamir Duberstein wrote: > > > > The test already prints the same information on failure; remove > > > > redundant pr_debug() logs. > > ... > > > > > - pr_debug("\"%s\", \"%s\" ->\n", str, fmt); \ > > > > > > What *if* the n_args == 0 here? > > > > Then there's no assertion in this block, so the test cannot possibly fail here. > > Correct, but I'm talking about this in a scope of the removed debug print. > I.o.w. how would we even know that this was the case? > > (I'm not objecting removal, what I want from you is to have a descriptive and > explanatory commit message that's answers to "why is this needed?" and "why is > it safe to do?") The true answer to "why is this needed" is Petr requested it in https://lore.kernel.org/all/Z6s2eqh0jkYHntUL@pathway.suse.cz/ (again, lore is having issues): On Tue, Feb 11, 2025 at 6:37 AM Petr Mladek <pmladek@suse.com> wrote: > > [...] > > But when thinking more about it. I think that even pr_debug() is not > the right solution. > > IMHO, we really want to print these details only when the test fails. > > Best Regards, > Petr The commit message already answers "why is it safe to do": > The test already prints the same information on failure; remove > redundant pr_debug() logs. Perhaps what you're asking for is an assertion to be added if n_args == 0? I think that would make sense. Does it belong in this series? Tamir
On Tue, Feb 11, 2025 at 11:02:59AM -0500, Tamir Duberstein wrote: > On Tue, Feb 11, 2025 at 10:58 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Feb 11, 2025 at 10:50:33AM -0500, Tamir Duberstein wrote: > > > On Tue, Feb 11, 2025 at 10:42 AM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Tue, Feb 11, 2025 at 10:13:37AM -0500, Tamir Duberstein wrote: > > > > > The test already prints the same information on failure; remove > > > > > redundant pr_debug() logs. ... > > > > > - pr_debug("\"%s\", \"%s\" ->\n", str, fmt); \ > > > > > > > > What *if* the n_args == 0 here? > > > > > > Then there's no assertion in this block, so the test cannot possibly fail here. > > > > Correct, but I'm talking about this in a scope of the removed debug print. > > I.o.w. how would we even know that this was the case? > > > > (I'm not objecting removal, what I want from you is to have a descriptive and > > explanatory commit message that's answers to "why is this needed?" and "why is > > it safe to do?") > > The true answer to "why is this needed" is Petr requested it in > https://lore.kernel.org/all/Z6s2eqh0jkYHntUL@pathway.suse.cz/ (again, > lore is having issues): > > On Tue, Feb 11, 2025 at 6:37 AM Petr Mladek <pmladek@suse.com> wrote: [...] > > But when thinking more about it. I think that even pr_debug() is not > > the right solution. > > > > IMHO, we really want to print these details only when the test fails. > > > > Best Regards, > > Petr > > The commit message already answers "why is it safe to do": Not really. It answers that "why is it safe to do when test case fails?". > > The test already prints the same information on failure; remove > > redundant pr_debug() logs. > > Perhaps what you're asking for is an assertion to be added if n_args > == 0? I think that would make sense. Does it belong in this series? I don't know if it's possible case. I don't know if we need an assertion. Please, research.
On Tue, Feb 11, 2025 at 12:16 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Feb 11, 2025 at 11:02:59AM -0500, Tamir Duberstein wrote: > > On Tue, Feb 11, 2025 at 10:58 AM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Tue, Feb 11, 2025 at 10:50:33AM -0500, Tamir Duberstein wrote: > > > > On Tue, Feb 11, 2025 at 10:42 AM Andy Shevchenko > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > On Tue, Feb 11, 2025 at 10:13:37AM -0500, Tamir Duberstein wrote: > > > > > > The test already prints the same information on failure; remove > > > > > > redundant pr_debug() logs. > > ... > > > > > > > - pr_debug("\"%s\", \"%s\" ->\n", str, fmt); \ > > > > > > > > > > What *if* the n_args == 0 here? > > > > > > > > Then there's no assertion in this block, so the test cannot possibly fail here. > > > > > > Correct, but I'm talking about this in a scope of the removed debug print. > > > I.o.w. how would we even know that this was the case? > > > > > > (I'm not objecting removal, what I want from you is to have a descriptive and > > > explanatory commit message that's answers to "why is this needed?" and "why is > > > it safe to do?") > > > > The true answer to "why is this needed" is Petr requested it in > > https://lore.kernel.org/all/Z6s2eqh0jkYHntUL@pathway.suse.cz/ (again, > > lore is having issues): > > > > On Tue, Feb 11, 2025 at 6:37 AM Petr Mladek <pmladek@suse.com> wrote: > > [...] > > > > But when thinking more about it. I think that even pr_debug() is not > > > the right solution. > > > > > > IMHO, we really want to print these details only when the test fails. > > > > > > Best Regards, > > > Petr > > > > The commit message already answers "why is it safe to do": > > Not really. It answers that "why is it safe to do when test case fails?". > > > > The test already prints the same information on failure; remove > > > redundant pr_debug() logs. > > > > Perhaps what you're asking for is an assertion to be added if n_args > > == 0? I think that would make sense. Does it belong in this series? > > I don't know if it's possible case. I don't know if we need an assertion. > Please, research. Such an assertion is not necessary. `_check_numbers_template` is called from `check_{ull,ll,ulong,long,uint,int,ushort,short,uchar,char}` which are in turn called from `_test`: > if (ret != n_args) { > KUNIT_FAIL(test, "vsscanf(\"%s\", \"%s\", ...) returned %d expected %d", string, fmt, ret, n_args); > } else { > (*fn)(test, check_data, string, fmt, n_args, ap); // <-- `fn` is `check_*`. > } So `n_args` comes from the test expectation, and it's already checked against the return value of `vsscanf`.
diff --git a/lib/test_scanf.c b/lib/test_scanf.c index 44f8508c9d88..07444a852fd4 100644 --- a/lib/test_scanf.c +++ b/lib/test_scanf.c @@ -62,10 +62,8 @@ _test(check_fn fn, const void *check_data, const char *string, const char *fmt, #define _check_numbers_template(arg_fmt, expect, str, fmt, n_args, ap) \ do { \ - pr_debug("\"%s\", \"%s\" ->\n", str, fmt); \ for (; n_args > 0; n_args--, expect++) { \ typeof(*expect) got = *va_arg(ap, typeof(expect)); \ - pr_debug("\t" arg_fmt "\n", got); \ if (got != *expect) { \ pr_warn("vsscanf(\"%s\", \"%s\", ...) expected " arg_fmt " got " arg_fmt "\n", \ str, fmt, *expect, got); \ @@ -689,7 +687,6 @@ do { \ total_tests++; \ len = snprintf(test_buffer, BUF_SIZE, gen_fmt, expect); \ got = (fn)(test_buffer, &endp, base); \ - pr_debug(#fn "(\"%s\", %d) -> " gen_fmt "\n", test_buffer, base, got); \ if (got != (expect)) { \ fail = true; \ pr_warn(#fn "(\"%s\", %d): got " gen_fmt " expected " gen_fmt "\n", \
The test already prints the same information on failure; remove redundant pr_debug() logs. Signed-off-by: Tamir Duberstein <tamird@gmail.com> --- lib/test_scanf.c | 3 --- 1 file changed, 3 deletions(-)