Message-ID: <CABVgOS=WoKEpPU=0f=mSfdx1g6AkEtx6QJTiNru1XSTev3sGaQ@mail.gmail.com>
Date:   Tue, 15 Aug 2023 17:16:42 +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 06/10] kunit: string-stream: Pass struct kunit to string_stream_get_string()

On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald
<rf@...nsource.cirrus.com> wrote:
>
> Pass a struct kunit* and gfp_t to string_stream_get_string(). Allocate
> the returned buffer using these instead of using the stream->test and
> stream->gfp.
>
> This is preparation for removing the dependence of string_stream on
> struct kunit, so that string_stream can be used for the debugfs log.
>
> Signed-off-by: Richard Fitzgerald <rf@...nsource.cirrus.com>
> ---

Makes sense to me.

I think that, if we weren't going to remove the struct kunit
dependency, we'd want to either keep a version of
string_stream_get_string() which uses it, or, e.g., fall back to it if
the struct passed in is NULL.

The other option is to have a version which doesn't manage the string
at all, so just takes a gfp and requires the caller to free it (or
register an action to do so). If we did that, we could get rid of the
struct kunit pointer totally (though it may be better to keep it and
have both versions).

So -- to switch to some stream-of-consciousness thoughts about this --
basically there are three possible variants of
string_stream_get_string():
- Current version: uses the stream's struct kunit. Useless if this is
NULL, but very convenient otherwise.
- This patch: manage the string using the struct kunit passed as an
argument. Still can't be used directly outside a test, but adds enough
flexibility to get _append to work.
- Totally unmanaged: the generated string is allocated separately, and
must be freed (directly or in a deferred action) by the caller. Works
well outside tests, but less convenient.

Personally, I feel that the design of string_stream is heading towards
the third option. By the end of this series, everything uses
_string_stream_concatenate_to_buf() anyway. There's only one call to
string_stream_get_string() outside of the logging / string_stream
tests, and all that does is log the results (and it has a fallback to
log each fragment separately if the allocation fails).

Then again, if this is only really being used in tests, then we can
probably just stick with string_stream_get_string() as-is, remove the
string_stream->test member totally, and if we need it, we can make
_string_stream_concatenate_to_buf() public, or add an unmanaged
version anyway.

So, after all that, I think this is probably good as-is. _Maybe_ we
could rename string_stream_get_string() to something like
string_stream_get_managed_string(), now that it's the only function
which is "managed" as a KUnit resource, but we can equally put that
off until we need to add an unmanaged version.

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

Cheers,
-- David


>  lib/kunit/string-stream-test.c | 26 +++++++++++++++-----------
>  lib/kunit/string-stream.c      |  8 ++++----
>  lib/kunit/string-stream.h      |  3 ++-
>  lib/kunit/test.c               |  2 +-
>  4 files changed, 22 insertions(+), 17 deletions(-)
>
> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
> index 46c2ac162fe8..8a30bb7d5fb7 100644
> --- a/lib/kunit/string-stream-test.c
> +++ b/lib/kunit/string-stream-test.c
> @@ -57,7 +57,7 @@ static void string_stream_line_add_test(struct kunit *test)
>         }
>         num_lines = i;
>
> -       concat_string = string_stream_get_string(stream);
> +       concat_string = string_stream_get_string(test, stream, GFP_KERNEL);
>         KUNIT_EXPECT_NOT_ERR_OR_NULL(test, concat_string);
>         KUNIT_EXPECT_EQ(test, strlen(concat_string), total_len);
>
> @@ -113,7 +113,7 @@ static void string_stream_variable_length_line_test(struct kunit *test)
>         }
>         num_lines = i;
>
> -       concat_string = string_stream_get_string(stream);
> +       concat_string = string_stream_get_string(test, stream, GFP_KERNEL);
>         KUNIT_EXPECT_NOT_ERR_OR_NULL(test, concat_string);
>         KUNIT_EXPECT_EQ(test, strlen(concat_string), total_len);
>
> @@ -165,17 +165,18 @@ static void string_stream_append_test(struct kunit *test)
>
>         /* Append content of empty stream to empty stream */
>         string_stream_append(stream_1, stream_2);
> -       KUNIT_EXPECT_EQ(test, strlen(string_stream_get_string(stream_1)), 0);
> +       KUNIT_EXPECT_EQ(test, strlen(string_stream_get_string(test, stream_1, GFP_KERNEL)), 0);
>
>         /* Add some data to stream_1 */
>         for (i = 0; i < ARRAY_SIZE(strings_1); ++i)
>                 string_stream_add(stream_1, "%s\n", strings_1[i]);
>
> -       original_content = string_stream_get_string(stream_1);
> +       original_content = string_stream_get_string(test, stream_1, GFP_KERNEL);
>
>         /* Append content of empty stream to non-empty stream */
>         string_stream_append(stream_1, stream_2);
> -       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream_1), original_content);
> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream_1, GFP_KERNEL),
> +                          original_content);
>
>         /* Add some data to stream_2 */
>         for (i = 0; i < ARRAY_SIZE(strings_2); ++i)
> @@ -188,14 +189,15 @@ static void string_stream_append_test(struct kunit *test)
>          * End result should be the original content of stream_1 plus
>          * the content of stream_2.
>          */
> -       stream_2_content = string_stream_get_string(stream_2);
> +       stream_2_content = string_stream_get_string(test, stream_2, GFP_KERNEL);
>         combined_length = strlen(original_content) + strlen(stream_2_content);
>         combined_length++; /* for terminating \0 */
>         combined_content = kunit_kmalloc(test, combined_length, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, combined_content);
>         snprintf(combined_content, combined_length, "%s%s", original_content, stream_2_content);
>
> -       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream_1), combined_content);
> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream_1, GFP_KERNEL),
> +                          combined_content);
>
>         /* Append content of non-empty stream to empty stream */
>         string_stream_destroy(stream_1);
> @@ -204,7 +206,8 @@ static void string_stream_append_test(struct kunit *test)
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
>
>         string_stream_append(stream_1, stream_2);
> -       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream_1), stream_2_content);
> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream_1, GFP_KERNEL),
> +                          stream_2_content);
>  }
>
>  /* Adding an empty string should not create a fragment. */
> @@ -224,7 +227,8 @@ static void string_stream_append_empty_string_test(struct kunit *test)
>         string_stream_add(stream, "Add this line");
>         string_stream_add(stream, "%s", "");
>         KUNIT_EXPECT_EQ(test, list_count_nodes(&stream->fragments), 1);
> -       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream), "Add this line");
> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream, GFP_KERNEL),
> +                          "Add this line");
>  }
>
>  /* Adding strings without automatic newline appending */
> @@ -244,7 +248,7 @@ static void string_stream_no_auto_newline_test(struct kunit *test)
>         string_stream_add(stream, "Two\n");
>         string_stream_add(stream, "%s\n", "Three");
>         string_stream_add(stream, "Four");
> -       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream),
> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream, GFP_KERNEL),
>                            "OneTwo\nThree\nFour");
>  }
>
> @@ -271,7 +275,7 @@ static void string_stream_auto_newline_test(struct kunit *test)
>         string_stream_add(stream, "Five\n%s", "Six");
>         string_stream_add(stream, "Seven\n\n");
>         string_stream_add(stream, "Eight");
> -       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream),
> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream, GFP_KERNEL),
>                            "One\nTwo\nThree\nFour\nFive\nSix\nSeven\n\nEight\n");
>  }
>
> diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
> index 1dcf6513b692..eb673d3ea3bd 100644
> --- a/lib/kunit/string-stream.c
> +++ b/lib/kunit/string-stream.c
> @@ -117,13 +117,14 @@ static void string_stream_clear(struct string_stream *stream)
>         spin_unlock(&stream->lock);
>  }
>
> -char *string_stream_get_string(struct string_stream *stream)
> +char *string_stream_get_string(struct kunit *test, struct string_stream *stream,
> +                              gfp_t gfp)
>  {
>         struct string_stream_fragment *frag_container;
>         size_t buf_len = stream->length + 1; /* +1 for null byte. */
>         char *buf;
>
> -       buf = kunit_kzalloc(stream->test, buf_len, stream->gfp);
> +       buf = kunit_kzalloc(test, buf_len, gfp);
>         if (!buf)
>                 return NULL;
>
> @@ -140,8 +141,7 @@ int string_stream_append(struct string_stream *stream,
>  {
>         const char *other_content;
>
> -       other_content = string_stream_get_string(other);
> -
> +       other_content = string_stream_get_string(other->test, other, other->gfp);
>         if (!other_content)
>                 return -ENOMEM;
>
> diff --git a/lib/kunit/string-stream.h b/lib/kunit/string-stream.h
> index 048930bf97f0..6b4a747881c5 100644
> --- a/lib/kunit/string-stream.h
> +++ b/lib/kunit/string-stream.h
> @@ -39,7 +39,8 @@ int __printf(2, 0) string_stream_vadd(struct string_stream *stream,
>                                       const char *fmt,
>                                       va_list args);
>
> -char *string_stream_get_string(struct string_stream *stream);
> +char *string_stream_get_string(struct kunit *test, struct string_stream *stream,
> +                              gfp_t gfp);
>
>  int string_stream_append(struct string_stream *stream,
>                          struct string_stream *other);
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 49698a168437..520e15f49d0d 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -286,7 +286,7 @@ static void kunit_print_string_stream(struct kunit *test,
>         if (string_stream_is_empty(stream))
>                 return;
>
> -       buf = string_stream_get_string(stream);
> +       buf = string_stream_get_string(test, stream, GFP_KERNEL);
>         if (!buf) {
>                 kunit_err(test,
>                           "Could not allocate buffer, dumping stream:\n");
> --
> 2.30.2
>

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

Powered by blists - more mailing lists