linux-kernel - Re: [PATCH 4/6] kunit: factor out kunit_base_assert_format() call into kunit

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: <CAFd5g47r8aQBWPtt6ffHokqqN2sMi10p1Q5QA3xGVLTVDQh98Q@mail.gmail.com>
Date:   Mon, 10 Jan 2022 17:31:58 -0500
From:   Brendan Higgins <brendanhiggins@...gle.com>
To:     Daniel Latypov <dlatypov@...gle.com>
Cc:     davidgow@...gle.com, linux-kernel@...r.kernel.org,
        kunit-dev@...glegroups.com, linux-kselftest@...r.kernel.org,
        skhan@...uxfoundation.org, torvalds@...ux-foundation.org
Subject: Re: [PATCH 4/6] kunit: factor out kunit_base_assert_format() call
 into kunit_fail()

On Fri, Jan 7, 2022 at 8:23 PM Daniel Latypov <dlatypov@...gle.com> wrote:
>
> We call this function first thing for all the assertion `format()`
> functions.
> This is the part that prints the file and line number and assertion type
> (EXPECTATION, ASSERTION).
>
> Having it as part of the format functions lets us have the flexibility
> to not print that information (or print it differently) for new
> assertion types, but I think this we don't need that.

nit: drop the "this".

> And in the future, we'd like to consider factoring that data (file,
> line#, type) out of the kunit_assert struct and into a `static`
> variable, as Linus suggested [1], so we'd need to extract it anyways.
>
> [1] https://groups.google.com/g/kunit-dev/c/i3fZXgvBrfA/m/VULQg1z6BAAJ
>
> Signed-off-by: Daniel Latypov <dlatypov@...gle.com>
> ---
>  lib/kunit/assert.c | 6 ------
>  lib/kunit/test.c   | 1 +
>  2 files changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
> index b972bda61c0c..4d9a1295efc7 100644
> --- a/lib/kunit/assert.c
> +++ b/lib/kunit/assert.c
> @@ -40,7 +40,6 @@ EXPORT_SYMBOL_GPL(kunit_assert_print_msg);
>  void kunit_fail_assert_format(const struct kunit_assert *assert,
>                               struct string_stream *stream)
>  {
> -       kunit_base_assert_format(assert, stream);
>         string_stream_add(stream, "%pV", &assert->message);
>  }
>  EXPORT_SYMBOL_GPL(kunit_fail_assert_format);
> @@ -52,7 +51,6 @@ void kunit_unary_assert_format(const struct kunit_assert *assert,
>
>         unary_assert = container_of(assert, struct kunit_unary_assert, assert);
>
> -       kunit_base_assert_format(assert, stream);
>         if (unary_assert->expected_true)
>                 string_stream_add(stream,
>                                   KUNIT_SUBTEST_INDENT "Expected %s to be true, but is false\n",
> @@ -73,7 +71,6 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
>         ptr_assert = container_of(assert, struct kunit_ptr_not_err_assert,
>                                   assert);
>
> -       kunit_base_assert_format(assert, stream);
>         if (!ptr_assert->value) {
>                 string_stream_add(stream,
>                                   KUNIT_SUBTEST_INDENT "Expected %s is not null, but is\n",
> @@ -119,7 +116,6 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
>         binary_assert = container_of(assert, struct kunit_binary_assert,
>                                      assert);
>
> -       kunit_base_assert_format(assert, stream);
>         string_stream_add(stream,
>                           KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
>                           binary_assert->left_text,
> @@ -147,7 +143,6 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
>         binary_assert = container_of(assert, struct kunit_binary_ptr_assert,
>                                      assert);
>
> -       kunit_base_assert_format(assert, stream);
>         string_stream_add(stream,
>                           KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
>                           binary_assert->left_text,
> @@ -187,7 +182,6 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
>         binary_assert = container_of(assert, struct kunit_binary_str_assert,
>                                      assert);
>
> -       kunit_base_assert_format(assert, stream);
>         string_stream_add(stream,
>                           KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
>                           binary_assert->left_text,
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 5ad671745483..735c1b67d843 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -255,6 +255,7 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
>                 return;
>         }
>
> +       kunit_base_assert_format(assert, stream);

I think my thinking in having this function called by the other assert
functions was to take advantage of inheritance. I was treating
kunit_base_assert_format as the parent method that other methods were
inheriting from, so I wanted to have them inherit some of the common
behavior by calling the original function.

If you decide to make this change, I think it would be a good idea to
change the name of kunit_base_assert_format to not mislead to this
effect.

>         assert->format(assert, stream);
>
>         kunit_print_string_stream(test, stream);
> --
> 2.34.1.575.g55b058a8bb-goog
>