[v5,07/10] kunit: string-stream: Decouple string_stream from kunit

Re-work string_stream so that it is not tied to a struct kunit. This is
to allow using it for the log of struct kunit_suite.

Instead of resource-managing individual allocations the whole string_stream
can be resource-managed, if required.

    alloc_string_stream() now allocates a string stream that is
    not resource-managed.

    string_stream_destroy() now works on an unmanaged string_stream
    allocated by alloc_string_stream() and frees the entire
    string_stream (previously it only freed the fragments).

For resource-managed allocations use kunit_alloc_string_stream()
and kunit_free_string_stream().

In addition to this, string_stream_get_string() now returns an
unmanaged buffer that the caller must kfree().

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
Changes since V4:
- Adding the kunit_[alloc|free]_string_stream() functions has been split
  out into the previous patch to reduce the amount of code churn in
  this patch.
- string_stream_destroy() has been kept and re-used instead of replacing
  it with a new function.
- string_stream_get_string() now returns an unmanaged buffer. This avoids
  a large code change to string_stream_append().
- Added wrapper function for resource free to prevent the type warning of
  passing string_stream_destroy() directly to kunit_add_action_or_reset().
---
 lib/kunit/string-stream-test.c |  2 +-
 lib/kunit/string-stream.c      | 59 ++++++++++++++++++++++------------
 lib/kunit/string-stream.h      |  4 ++-
 lib/kunit/test.c               |  2 +-
 4 files changed, 44 insertions(+), 23 deletions(-)

On Thu, 24 Aug 2023 at 22:32, Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> Re-work string_stream so that it is not tied to a struct kunit. This is
> to allow using it for the log of struct kunit_suite.
>
> Instead of resource-managing individual allocations the whole string_stream
> can be resource-managed, if required.
>
>     alloc_string_stream() now allocates a string stream that is
>     not resource-managed.
>
>     string_stream_destroy() now works on an unmanaged string_stream
>     allocated by alloc_string_stream() and frees the entire
>     string_stream (previously it only freed the fragments).
>
> For resource-managed allocations use kunit_alloc_string_stream()
> and kunit_free_string_stream().
>
> In addition to this, string_stream_get_string() now returns an
> unmanaged buffer that the caller must kfree().
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
> Changes since V4:
> - Adding the kunit_[alloc|free]_string_stream() functions has been split
>   out into the previous patch to reduce the amount of code churn in
>   this patch.
> - string_stream_destroy() has been kept and re-used instead of replacing
>   it with a new function.
> - string_stream_get_string() now returns an unmanaged buffer. This avoids
>   a large code change to string_stream_append().
> - Added wrapper function for resource free to prevent the type warning of
>   passing string_stream_destroy() directly to kunit_add_action_or_reset().
> ---

The changes all make sense to me, and work here. Thanks!

The only slight issue is there's one missing spot which still casts
the kunit_action_t function pointer directly, in the test. Up to you
if you want to change that, too (though it may need a helper of its
own.)

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

Cheers,
-- David



>  lib/kunit/string-stream-test.c |  2 +-
>  lib/kunit/string-stream.c      | 59 ++++++++++++++++++++++------------
>  lib/kunit/string-stream.h      |  4 ++-
>  lib/kunit/test.c               |  2 +-
>  4 files changed, 44 insertions(+), 23 deletions(-)
>
> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
> index 89549c237069..45a2d221f1b5 100644
> --- a/lib/kunit/string-stream-test.c
> +++ b/lib/kunit/string-stream-test.c
> @@ -16,6 +16,7 @@ static char *get_concatenated_string(struct kunit *test, struct string_stream *s
>         char *str = string_stream_get_string(stream);
>
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, str);
> +       kunit_add_action(test, (kunit_action_t *)kfree, (void *)str);

This still directly casts kfree to kunit_action_t, so triggers a
warning. I'm okay with it personally (and at some point we'll probably
implement a public kunit_free_at_end() function to do things like
this, which we already have in some other tests).


>
>         return str;
>  }
> @@ -30,7 +31,6 @@ static void string_stream_init_test(struct kunit *test)
>
>         KUNIT_EXPECT_EQ(test, stream->length, 0);
>         KUNIT_EXPECT_TRUE(test, list_empty(&stream->fragments));
> -       KUNIT_EXPECT_PTR_EQ(test, stream->test, test);
>         KUNIT_EXPECT_EQ(test, stream->gfp, GFP_KERNEL);
>         KUNIT_EXPECT_FALSE(test, stream->append_newlines);
>
> diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
> index 12ecf15e1f6b..c39f1cba3bcd 100644
> --- a/lib/kunit/string-stream.c
> +++ b/lib/kunit/string-stream.c
> @@ -13,30 +13,28 @@
>  #include "string-stream.h"
>
>
> -static struct string_stream_fragment *alloc_string_stream_fragment(
> -               struct kunit *test, int len, gfp_t gfp)
> +static struct string_stream_fragment *alloc_string_stream_fragment(int len, gfp_t gfp)
>  {
>         struct string_stream_fragment *frag;
>
> -       frag = kunit_kzalloc(test, sizeof(*frag), gfp);
> +       frag = kzalloc(sizeof(*frag), gfp);
>         if (!frag)
>                 return ERR_PTR(-ENOMEM);
>
> -       frag->fragment = kunit_kmalloc(test, len, gfp);
> +       frag->fragment = kmalloc(len, gfp);
>         if (!frag->fragment) {
> -               kunit_kfree(test, frag);
> +               kfree(frag);
>                 return ERR_PTR(-ENOMEM);
>         }
>
>         return frag;
>  }
>
> -static void string_stream_fragment_destroy(struct kunit *test,
> -                                          struct string_stream_fragment *frag)
> +static void string_stream_fragment_destroy(struct string_stream_fragment *frag)
>  {
>         list_del(&frag->node);
> -       kunit_kfree(test, frag->fragment);
> -       kunit_kfree(test, frag);
> +       kfree(frag->fragment);
> +       kfree(frag);
>  }
>
>  int string_stream_vadd(struct string_stream *stream,
> @@ -65,9 +63,7 @@ int string_stream_vadd(struct string_stream *stream,
>         /* Need space for null byte. */
>         buf_len++;
>
> -       frag_container = alloc_string_stream_fragment(stream->test,
> -                                                     buf_len,
> -                                                     stream->gfp);
> +       frag_container = alloc_string_stream_fragment(buf_len, stream->gfp);
>         if (IS_ERR(frag_container))
>                 return PTR_ERR(frag_container);
>
> @@ -111,7 +107,7 @@ static void string_stream_clear(struct string_stream *stream)
>                                  frag_container_safe,
>                                  &stream->fragments,
>                                  node) {
> -               string_stream_fragment_destroy(stream->test, frag_container);
> +               string_stream_fragment_destroy(frag_container);
>         }
>         stream->length = 0;
>         spin_unlock(&stream->lock);
> @@ -123,7 +119,7 @@ char *string_stream_get_string(struct string_stream *stream)
>         size_t buf_len = stream->length + 1; /* +1 for null byte. */
>         char *buf;
>
> -       buf = kunit_kzalloc(stream->test, buf_len, stream->gfp);
> +       buf = kzalloc(buf_len, stream->gfp);
>         if (!buf)
>                 return NULL;
>
> @@ -139,13 +135,17 @@ int string_stream_append(struct string_stream *stream,
>                          struct string_stream *other)
>  {
>         const char *other_content;
> +       int ret;
>
>         other_content = string_stream_get_string(other);
>
>         if (!other_content)
>                 return -ENOMEM;
>
> -       return string_stream_add(stream, other_content);
> +       ret = string_stream_add(stream, other_content);
> +       kfree(other_content);
> +
> +       return ret;
>  }
>
>  bool string_stream_is_empty(struct string_stream *stream)
> @@ -153,16 +153,15 @@ bool string_stream_is_empty(struct string_stream *stream)
>         return list_empty(&stream->fragments);
>  }
>
> -static struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp)
> +struct string_stream *alloc_string_stream(gfp_t gfp)
>  {
>         struct string_stream *stream;
>
> -       stream = kunit_kzalloc(test, sizeof(*stream), gfp);
> +       stream = kzalloc(sizeof(*stream), gfp);
>         if (!stream)
>                 return ERR_PTR(-ENOMEM);
>
>         stream->gfp = gfp;
> -       stream->test = test;
>         INIT_LIST_HEAD(&stream->fragments);
>         spin_lock_init(&stream->lock);
>
> @@ -171,15 +170,35 @@ static struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp)
>
>  void string_stream_destroy(struct string_stream *stream)
>  {
> +       if (!stream)
> +               return;
> +
>         string_stream_clear(stream);
> +       kfree(stream);
> +}
> +
> +static void resource_free_string_stream(void *p)
> +{
> +       struct string_stream *stream = p;
> +
> +       string_stream_destroy(stream);
>  }
>
>  struct string_stream *kunit_alloc_string_stream(struct kunit *test, gfp_t gfp)
>  {
> -       return alloc_string_stream(test, gfp);
> +       struct string_stream *stream;
> +
> +       stream = alloc_string_stream(gfp);
> +       if (IS_ERR(stream))
> +               return stream;
> +
> +       if (kunit_add_action_or_reset(test, resource_free_string_stream, stream) != 0)
> +               return ERR_PTR(-ENOMEM);
> +
> +       return stream;
>  }
>
>  void kunit_free_string_stream(struct kunit *test, struct string_stream *stream)
>  {
> -       string_stream_destroy(stream);
> +       kunit_release_action(test, resource_free_string_stream, (void *)stream);
>  }
> diff --git a/lib/kunit/string-stream.h b/lib/kunit/string-stream.h
> index 3e70ee9d66e9..c55925a9b67f 100644
> --- a/lib/kunit/string-stream.h
> +++ b/lib/kunit/string-stream.h
> @@ -23,7 +23,6 @@ struct string_stream {
>         struct list_head fragments;
>         /* length and fragments are protected by this lock */
>         spinlock_t lock;
> -       struct kunit *test;
>         gfp_t gfp;
>         bool append_newlines;
>  };
> @@ -33,6 +32,9 @@ struct kunit;
>  struct string_stream *kunit_alloc_string_stream(struct kunit *test, gfp_t gfp);
>  void kunit_free_string_stream(struct kunit *test, struct string_stream *stream);
>
> +struct string_stream *alloc_string_stream(gfp_t gfp);
> +void free_string_stream(struct string_stream *stream);
> +
>  int __printf(2, 3) string_stream_add(struct string_stream *stream,
>                                      const char *fmt, ...);
>
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 93d9225d61e3..2ad45a4ac06a 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -296,7 +296,7 @@ static void kunit_print_string_stream(struct kunit *test,
>                 kunit_err(test, "\n");
>         } else {
>                 kunit_err(test, "%s", buf);
> -               kunit_kfree(test, buf);
> +               kfree(buf);
>         }
>  }
>
> --
> 2.30.2
>