linux-kernel - Re: [PATCH v3 3/4] kunit: Don't crash if no parameters are generated

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: <CAGS_qxp5KLKqFOBT2zyjw3R5fqcVnzFjTVTnqxKt=X9ZJ-oREQ@mail.gmail.com>
Date:   Thu, 28 Oct 2021 15:21:11 -0700
From:   Daniel Latypov <dlatypov@...gle.com>
To:     David Gow <davidgow@...gle.com>
Cc:     Brendan Higgins <brendanhiggins@...gle.com>,
        Rae Moar <rmr167@...il.com>,
        Shuah Khan <skhan@...uxfoundation.org>,
        kunit-dev@...glegroups.com, linux-kselftest@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/4] kunit: Don't crash if no parameters are generated

On Wed, Oct 27, 2021 at 11:42 PM David Gow <davidgow@...gle.com> wrote:
>
> It's possible that a parameterised test could end up with zero
> parameters. At the moment, the test function will nevertheless be called
> with NULL as the parameter. Instead, don't try to run the test code, and
> just mark the test as SKIPped.
>
> Reported-by: Daniel Latypov <dlatypov@...gle.com>
> Signed-off-by: David Gow <davidgow@...gle.com>

Reviewed-by: Daniel Latypov <dlatypov@...gle.com>

There's a small bug in this patch noted below, we just need to move
the "# Subtest" change into the child patch.
If/when we make that change, I have an optional suggestion about
flipping the if/else branch.

But other than that, this looks good to me.

> ---
>
> Changes since v2:
> https://lore.kernel.org/linux-kselftest/20211027013702.2039566-3-davidgow@google.com/
> - Rework to not share the loop between the parameterised and
>   non-parameterised test cases.
>   - Suggested by Daniel Latypov.
>   - Avoids using a magic non-zero pointer value.
>
>  lib/kunit/test.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 3bd741e50a2d..dfe1127aacfd 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -508,12 +508,12 @@ int kunit_run_tests(struct kunit_suite *suite)
>                         /* Get initial param. */
>                         param_desc[0] = '\0';
>                         test.param_value = test_case->generate_params(NULL, param_desc);
> -               }
> +                       kunit_log(KERN_INFO, &test, KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT
> +                                 "# Subtest: %s", test_case->name);

It looks like this change accidentally made its way into this patch as
opposed to the child.

This commit itself gives me a traffic light (red/yellow/green statuses):

[ERROR] Test inode_test_xtimestamp_decoding: 0 tests run!
====== [NO TESTS RUN] inode_test_xtimestamp_decoding =======
================= [PASSED] ext4_inode_test =================
============================================================

The problem is the output becomes this:
    # Subtest: ext4_inode_test
    1..1
        # Subtest: inode_test_xtimestamp_decoding
    # inode_test_xtimestamp_decoding: ok 1 - 1901-12-13 Lower bound of
32bit < 0 timestamp, no extra bits
   ...

>
> -               do {
> -                       kunit_run_case_catch_errors(suite, test_case, &test);
> +                       while (test.param_value) {
> +                               kunit_run_case_catch_errors(suite, test_case, &test);
>
> -                       if (test_case->generate_params) {
>                                 if (param_desc[0] == '\0') {
>                                         snprintf(param_desc, sizeof(param_desc),
>                                                  "param-%d", test.param_index);
> @@ -530,11 +530,15 @@ int kunit_run_tests(struct kunit_suite *suite)
>                                 param_desc[0] = '\0';
>                                 test.param_value = test_case->generate_params(test.param_value, param_desc);
>                                 test.param_index++;
> -                       }
>
> +                               kunit_update_stats(&param_stats, test.status);
> +                       }
> +               } else {

I have a very slight preference for having the order of these branches swapped.
i.e.

if (!test_case->generate_params) {
  /* Non-parameterised test */
} else { ... }

I prefer this because I think it's more readable for a few reasons:
* I like having the "normal" branch come first. This is likely the
code path a reader would care more about.
* I prefer having the shorter branch come first. It makes it easier to
read it through and see "oh, so this branch is just that one but with
XYZ"

> +                       /* Non-parameterised test. */
> +                       kunit_run_case_catch_errors(suite, test_case, &test);
>                         kunit_update_stats(&param_stats, test.status);
> +               }
>
> -               } while (test.param_value);
>
>                 kunit_print_test_stats(&test, param_stats);
>
> --
> 2.33.0.1079.g6e70778dc9-goog
>