Re: [PATCH v4 07/10] kunit: string-stream: Decouple string_stream from kunit

[David Gow]

On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald
<rf@xxxxxxxxxxxxxxxxxxxxx> 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
> object can be resource-managed as a single object:
>
>     alloc_string_stream() API is unchanged and takes a pointer to a
>     struct kunit but it now registers the returned string_stream object to
>     be resource-managed.
>
>     raw_alloc_string_stream() is a new function that allocates a
>     bare string_stream without any association to a struct kunit.
>
>     free_string_stream() is a new function that frees a resource-managed
>     string_stream allocated by alloc_string_stream().
>
>     raw_free_string_stream() is a new function that frees a non-managed
>     string_stream allocated by raw_alloc_string_stream().
>
> The confusing function string_stream_destroy() has been removed. This only
> called string_stream_clear() but didn't free the struct string_stream.
> Instead string_stream_clear() has been exported, and the new functions use
> the more conventional naming of "free" as the opposite of "alloc".
>
> Signed-off-by: Richard Fitzgerald <rf@xxxxxxxxxxxxxxxxxxxxx>
> ---

I'm in favour of this. Should we go further and get rid of the struct
kunit member from string_stream totally?

Also, note that the kunit_action_t casting is causing warnings on some
clang configs (and technically isn't valid C). Personally, I still
like it, but expect more emails from the kernel test robot and others.

Reviewed-by: David Gow <davidgow@xxxxxxxxxx>

>  lib/kunit/string-stream-test.c |  2 +-
>  lib/kunit/string-stream.c      | 92 +++++++++++++++++++++++-----------
>  lib/kunit/string-stream.h      | 12 ++++-
>  lib/kunit/test.c               |  2 +-
>  4 files changed, 77 insertions(+), 31 deletions(-)
>
> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
> index 8a30bb7d5fb7..437aa4b3179d 100644
> --- a/lib/kunit/string-stream-test.c
> +++ b/lib/kunit/string-stream-test.c
> @@ -200,7 +200,7 @@ static void string_stream_append_test(struct kunit *test)
>                            combined_content);
>
>         /* Append content of non-empty stream to empty stream */
> -       string_stream_destroy(stream_1);
> +       string_stream_clear(stream_1);
>
>         stream_1 = alloc_string_stream(test, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
> diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
> index eb673d3ea3bd..06104a729b45 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);
>
> @@ -102,7 +98,7 @@ int string_stream_add(struct string_stream *stream, const char *fmt, ...)
>         return result;
>  }
>
> -static void string_stream_clear(struct string_stream *stream)
> +void string_stream_clear(struct string_stream *stream)
>  {
>         struct string_stream_fragment *frag_container, *frag_container_safe;
>
> @@ -111,16 +107,28 @@ 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);
>  }
>
> +static void _string_stream_concatenate_to_buf(struct string_stream *stream,
> +                                             char *buf, size_t buf_len)
> +{
> +       struct string_stream_fragment *frag_container;
> +
> +       buf[0] = '\0';
> +
> +       spin_lock(&stream->lock);
> +       list_for_each_entry(frag_container, &stream->fragments, node)
> +               strlcat(buf, frag_container->fragment, buf_len);
> +       spin_unlock(&stream->lock);
> +}
> +
>  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;
>
> @@ -128,10 +136,7 @@ char *string_stream_get_string(struct kunit *test, struct string_stream *stream,
>         if (!buf)
>                 return NULL;
>
> -       spin_lock(&stream->lock);
> -       list_for_each_entry(frag_container, &stream->fragments, node)
> -               strlcat(buf, frag_container->fragment, buf_len);
> -       spin_unlock(&stream->lock);
> +       _string_stream_concatenate_to_buf(stream, buf, buf_len);
>
>         return buf;
>  }
> @@ -139,13 +144,20 @@ char *string_stream_get_string(struct kunit *test, struct string_stream *stream,
>  int string_stream_append(struct string_stream *stream,
>                          struct string_stream *other)
>  {
> -       const char *other_content;
> +       size_t other_buf_len = other->length + 1; /* +1 for null byte. */
> +       char *other_buf;
> +       int ret;
>
> -       other_content = string_stream_get_string(other->test, other, other->gfp);
> -       if (!other_content)
> +       other_buf = kmalloc(other_buf_len, GFP_KERNEL);
> +       if (!other_buf)
>                 return -ENOMEM;
>
> -       return string_stream_add(stream, other_content);
> +       _string_stream_concatenate_to_buf(other, other_buf, other_buf_len);
> +
> +       ret = string_stream_add(stream, other_buf);
> +       kfree(other_buf);
> +
> +       return ret;
>  }
>
>  bool string_stream_is_empty(struct string_stream *stream)
> @@ -153,23 +165,47 @@ bool string_stream_is_empty(struct string_stream *stream)
>         return list_empty(&stream->fragments);
>  }
>
> -struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp)
> +void raw_free_string_stream(struct string_stream *stream)
> +{
> +       string_stream_clear(stream);
> +       kfree(stream);
> +}
> +
> +struct string_stream *raw_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);
>
>         return stream;
>  }
>
> -void string_stream_destroy(struct string_stream *stream)
> +struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp)
>  {
> -       string_stream_clear(stream);
> +       struct string_stream *stream;
> +
> +       stream = raw_alloc_string_stream(gfp);
> +       if (IS_ERR(stream))
> +               return stream;
> +
> +       stream->test = test;
> +
> +       if (kunit_add_action_or_reset(test, (kunit_action_t *)raw_free_string_stream, stream) != 0)
> +               return ERR_PTR(-ENOMEM);

As the kernel test robot notes, this sort of function pointer casting
is not technically valid C, and some compiler setups are starting to
warn on it.

Personally, I'm still okay with it, but expect a bunch of robot email
complaining about it if it lands. If more compilers / configs start
warning, though, I'll try to fix all callers of the KUnit action
functions which are affected.

> +
> +       return stream;
> +}
> +
> +void free_string_stream(struct kunit *test, struct string_stream *stream)
> +{
> +       if (!stream)
> +               return;
> +
> +       kunit_release_action(test, (kunit_action_t *)raw_free_string_stream, (void *)stream);

(As above.)

>  }
> diff --git a/lib/kunit/string-stream.h b/lib/kunit/string-stream.h
> index 6b4a747881c5..fbeda486382a 100644
> --- a/lib/kunit/string-stream.h
> +++ b/lib/kunit/string-stream.h
> @@ -23,14 +23,24 @@ struct string_stream {
>         struct list_head fragments;
>         /* length and fragments are protected by this lock */
>         spinlock_t lock;
> +
> +       /*
> +        * Pointer to kunit this stream is associated with, or NULL if
> +        * not associated with a kunit.
> +        */
>         struct kunit *test;
> +

Can we just get rid of this totally? I don't think anything's actually
using it now. (Or have I missed something?)


>         gfp_t gfp;
>         bool append_newlines;
>  };
>
>  struct kunit;
>
> +struct string_stream *raw_alloc_string_stream(gfp_t gfp);
> +void raw_free_string_stream(struct string_stream *stream);
> +
>  struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp);
> +void free_string_stream(struct kunit *test, struct string_stream *stream);
>
>  int __printf(2, 3) string_stream_add(struct string_stream *stream,
>                                      const char *fmt, ...);
> @@ -47,7 +57,7 @@ int string_stream_append(struct string_stream *stream,
>
>  bool string_stream_is_empty(struct string_stream *stream);
>
> -void string_stream_destroy(struct string_stream *stream);
> +void string_stream_clear(struct string_stream *stream);
>
>  static inline void string_stream_set_append_newlines(struct string_stream *stream,
>                                                      bool append_newlines)
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 520e15f49d0d..4b69d12dfc96 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -322,7 +322,7 @@ static void kunit_fail(struct kunit *test, const struct kunit_loc *loc,
>
>         kunit_print_string_stream(test, stream);
>
> -       string_stream_destroy(stream);
> +       free_string_stream(test, stream);
>  }
>
>  void __noreturn __kunit_abort(struct kunit *test)
> --
> 2.30.2
>

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature