linux-kernel - Re: [PATCH v4 03/10] kunit: string-stream: Add cases for adding empty strings to a string

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: <CABVgOS=j0RJ0i64yQ3tz_e+ZeCS+Pfj0_GZy02UWJnV9TePgTA@mail.gmail.com>
Date:   Tue, 15 Aug 2023 17:16:11 +0800
From:   David Gow <davidgow@...gle.com>
To:     Richard Fitzgerald <rf@...nsource.cirrus.com>
Cc:     brendan.higgins@...ux.dev, rmoar@...gle.com,
        linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com,
        linux-kernel@...r.kernel.org, patches@...nsource.cirrus.com
Subject: Re: [PATCH v4 03/10] kunit: string-stream: Add cases for adding empty
 strings to a string_stream

On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald
<rf@...nsource.cirrus.com> wrote:
>
> Adds string_stream_append_empty_string_test() to test that adding an
> empty string to a string_stream doesn't create a new empty fragment.
>
> Signed-off-by: Richard Fitzgerald <rf@...nsource.cirrus.com>
> ---

Looks good. One minor note below (not worth resending the series to
fix by itself, though).

If you wanted to combine this with the previous patch, that'd be fine
too. (I don't mind either way.)

Reviewed-by: David Gow <davidgow@...gle.com>

Cheers,
-- David

>  lib/kunit/string-stream-test.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
> index 1d46d5f06d2a..efe13e3322b5 100644
> --- a/lib/kunit/string-stream-test.c
> +++ b/lib/kunit/string-stream-test.c
> @@ -206,11 +206,32 @@ static void string_stream_append_test(struct kunit *test)
>         KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream_1), stream_2_content);
>  }
>
> +/* Adding an empty string should not create a fragment. */
> +static void string_stream_append_empty_string_test(struct kunit *test)
> +{
> +       struct string_stream *stream;
> +
> +       stream = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
> +
> +       /* Formatted empty string */
> +       string_stream_add(stream, "%s", "");
> +       KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
> +       KUNIT_EXPECT_TRUE(test, list_empty(&stream->fragments));
> +
> +       /* Adding an empty string to a non-empty stream */
> +       string_stream_add(stream, "Add this line");
> +       string_stream_add(stream, "%s", "");
> +       KUNIT_EXPECT_EQ(test, list_count_nodes(&stream->fragments), 1);

While this is fine, I do wonder whether the more future-proof thing to
do would be to check that the number of fragments hasn't changed after
adding the empty string, rather than that it's definitely 1.

(In practice, even with a fancier allocation strategy, I can't imagine
us splitting "Add this line" into multiple fragments, but I think it's
slightly clearer that what we're testing is that the empty string
doesn't increase it.)


> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream), "Add this line");
> +}
> +
>  static struct kunit_case string_stream_test_cases[] = {
>         KUNIT_CASE(string_stream_init_test),
>         KUNIT_CASE(string_stream_line_add_test),
>         KUNIT_CASE(string_stream_variable_length_line_test),
>         KUNIT_CASE(string_stream_append_test),
> +       KUNIT_CASE(string_stream_append_empty_string_test),
>         {}
>  };
>
> --
> 2.30.2
>

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4003 bytes)