string_stream_managed_free_test() allocates a resource-managed
string_stream and tests that kunit_free_string_stream() calls
string_stream_destroy().

string_stream_resource_free_test() allocates a resource-managed
string_stream and tests that string_stream_destroy() is called
when the test resources are cleaned up.

The old string_stream_init_test() has been split into two tests,
one for kunit_alloc_string_stream() and the other for
alloc_string_stream().

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
Changes since V4:
- Added test case for kunit_free_string_stream().
- Split the initialization test into separate tests for managed and
  unmanaged allocations.
---
 lib/kunit/string-stream-test.c | 135 ++++++++++++++++++++++++++++++++-
 lib/kunit/string-stream.c      |   3 +
 2 files changed, 134 insertions(+), 4 deletions(-)

diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
index 45a2d221f1b5..6897c57e0db7 100644
--- a/lib/kunit/string-stream-test.c
+++ b/lib/kunit/string-stream-test.c
@@ -6,11 +6,25 @@
  * Author: Brendan Higgins <brendanhiggins@google.com>
  */
 
+#include <kunit/static_stub.h>
 #include <kunit/test.h>
 #include <linux/slab.h>
 
 #include "string-stream.h"
 
+struct string_stream_test_priv {
+	/* For testing resource-managed free. */
+	struct string_stream *freed_stream;
+	bool stream_free_again;
+};
+
+static void cleanup_raw_stream(void *p)
+{
+	struct string_stream *stream = p;
+
+	string_stream_destroy(stream);
+}
+
 static char *get_concatenated_string(struct kunit *test, struct string_stream *stream)
 {
 	char *str = string_stream_get_string(stream);
@@ -21,11 +35,12 @@ static char *get_concatenated_string(struct kunit *test, struct string_stream *s
 	return str;
 }
 
-/* string_stream object is initialized correctly. */
-static void string_stream_init_test(struct kunit *test)
+/* Managed string_stream object is initialized correctly. */
+static void string_stream_managed_init_test(struct kunit *test)
 {
 	struct string_stream *stream;
 
+	/* Resource-managed initialization. */
 	stream = kunit_alloc_string_stream(test, GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
 
@@ -37,6 +52,101 @@ static void string_stream_init_test(struct kunit *test)
 	KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
 }
 
+/* Unmanaged string_stream object is initialized correctly. */
+static void string_stream_unmanaged_init_test(struct kunit *test)
+{
+	struct string_stream *stream;
+
+	stream = alloc_string_stream(GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
+	kunit_add_action(test, cleanup_raw_stream, stream);
+
+	KUNIT_EXPECT_EQ(test, stream->length, 0);
+	KUNIT_EXPECT_TRUE(test, list_empty(&stream->fragments));
+	KUNIT_EXPECT_EQ(test, stream->gfp, GFP_KERNEL);
+	KUNIT_EXPECT_FALSE(test, stream->append_newlines);
+
+	KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
+}
+
+static void string_stream_destroy_stub(struct string_stream *stream)
+{
+	struct kunit *fake_test = kunit_get_current_test();
+	struct string_stream_test_priv *priv = fake_test->priv;
+
+	if (priv->freed_stream)
+		priv->stream_free_again = true;
+
+	priv->freed_stream = stream;
+
+	/*
+	 * Avoid calling deactivate_static_stub() or changing
+	 * current->kunit_test during cleanup. Leave the stream to
+	 * be freed during the test exit.
+	 */
+}
+
+/* kunit_free_string_stream() calls string_stream_desrtoy() */
+static void string_stream_managed_free_test(struct kunit *test)
+{
+	struct string_stream_test_priv *priv = test->priv;
+	struct string_stream *stream;
+
+	priv->freed_stream = NULL;
+	priv->stream_free_again = false;
+	kunit_activate_static_stub(test,
+				   string_stream_destroy,
+				   string_stream_destroy_stub);
+
+	stream = kunit_alloc_string_stream(test, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
+
+	/* This should call the stub function. */
+	kunit_free_string_stream(test, stream);
+
+	KUNIT_EXPECT_PTR_EQ(test, priv->freed_stream, stream);
+	KUNIT_EXPECT_FALSE(test, priv->stream_free_again);
+}
+
+/* string_stream object is freed when test is cleaned up. */
+static void string_stream_resource_free_test(struct kunit *test)
+{
+	struct string_stream_test_priv *priv = test->priv;
+	struct kunit *fake_test;
+	struct string_stream *stream;
+
+	fake_test = kunit_kzalloc(test, sizeof(*fake_test), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fake_test);
+
+	kunit_init_test(fake_test, "string_stream_fake_test", NULL);
+	fake_test->priv = priv;
+
+	/*
+	 * Activate stub before creating string_stream so the
+	 * string_stream will be cleaned up first.
+	 */
+	priv->freed_stream = NULL;
+	priv->stream_free_again = false;
+	kunit_activate_static_stub(fake_test,
+				   string_stream_destroy,
+				   string_stream_destroy_stub);
+
+	stream = kunit_alloc_string_stream(fake_test, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
+
+	/* Set current->kunit_test to fake_test so the static stub will be called. */
+	current->kunit_test = fake_test;
+
+	/* Cleanup test - the stub function should be called */
+	kunit_cleanup(fake_test);
+
+	/* Set current->kunit_test back to current test. */
+	current->kunit_test = test;
+
+	KUNIT_EXPECT_PTR_EQ(test, priv->freed_stream, stream);
+	KUNIT_EXPECT_FALSE(test, priv->stream_free_again);
+}
+
 /*
  * Add a series of lines to a string_stream. Check that all lines
  * appear in the correct order and no characters are dropped.
@@ -327,8 +437,24 @@ static void string_stream_auto_newline_test(struct kunit *test)
 			   "One\nTwo\nThree\nFour\nFive\nSix\nSeven\n\nEight\n");
 }
 
+static int string_stream_test_init(struct kunit *test)
+{
+	struct string_stream_test_priv *priv;
+
+	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	test->priv = priv;
+
+	return 0;
+}
+
 static struct kunit_case string_stream_test_cases[] = {
-	KUNIT_CASE(string_stream_init_test),
+	KUNIT_CASE(string_stream_managed_init_test),
+	KUNIT_CASE(string_stream_unmanaged_init_test),
+	KUNIT_CASE(string_stream_managed_free_test),
+	KUNIT_CASE(string_stream_resource_free_test),
 	KUNIT_CASE(string_stream_line_add_test),
 	KUNIT_CASE(string_stream_variable_length_line_test),
 	KUNIT_CASE(string_stream_append_test),
@@ -341,6 +467,7 @@ static struct kunit_case string_stream_test_cases[] = {
 
 static struct kunit_suite string_stream_test_suite = {
 	.name = "string-stream-test",
-	.test_cases = string_stream_test_cases
+	.test_cases = string_stream_test_cases,
+	.init = string_stream_test_init,
 };
 kunit_test_suites(&string_stream_test_suite);
diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
index c39f1cba3bcd..d2ded5207e9e 100644
--- a/lib/kunit/string-stream.c
+++ b/lib/kunit/string-stream.c
@@ -6,6 +6,7 @@
  * Author: Brendan Higgins <brendanhiggins@google.com>
  */
 
+#include <kunit/static_stub.h>
 #include <kunit/test.h>
 #include <linux/list.h>
 #include <linux/slab.h>
@@ -170,6 +171,8 @@ struct string_stream *alloc_string_stream(gfp_t gfp)
 
 void string_stream_destroy(struct string_stream *stream)
 {
+	KUNIT_STATIC_STUB_REDIRECT(string_stream_destroy, stream);
+
 	if (!stream)
 		return;
 
-- 
2.30.2

On Thu, 24 Aug 2023 at 22:32, 'Richard Fitzgerald' via KUnit
Development <kunit-dev@googlegroups.com> wrote:
>
> string_stream_managed_free_test() allocates a resource-managed
> string_stream and tests that kunit_free_string_stream() calls
> string_stream_destroy().
>
> string_stream_resource_free_test() allocates a resource-managed
> string_stream and tests that string_stream_destroy() is called
> when the test resources are cleaned up.
>
> The old string_stream_init_test() has been split into two tests,
> one for kunit_alloc_string_stream() and the other for
> alloc_string_stream().
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
> Changes since V4:
> - Added test case for kunit_free_string_stream().
> - Split the initialization test into separate tests for managed and
>   unmanaged allocations.
> ---

Looking over this again, I'm not convinced the streams are actually
getting freed. Once the stub has finished, the stream is removed from
the list of deferred actions / resources.

I'll have another look tomorrow in case I missed anything, but if so,
this might need to be explained further / made more obvious.

-- David

>  lib/kunit/string-stream-test.c | 135 ++++++++++++++++++++++++++++++++-
>  lib/kunit/string-stream.c      |   3 +
>  2 files changed, 134 insertions(+), 4 deletions(-)
>
> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
> index 45a2d221f1b5..6897c57e0db7 100644
> --- a/lib/kunit/string-stream-test.c
> +++ b/lib/kunit/string-stream-test.c
> @@ -6,11 +6,25 @@
>   * Author: Brendan Higgins <brendanhiggins@google.com>
>   */
>
> +#include <kunit/static_stub.h>
>  #include <kunit/test.h>
>  #include <linux/slab.h>
>
>  #include "string-stream.h"
>
> +struct string_stream_test_priv {
> +       /* For testing resource-managed free. */
> +       struct string_stream *freed_stream;
> +       bool stream_free_again;
> +};
> +
> +static void cleanup_raw_stream(void *p)
> +{
> +       struct string_stream *stream = p;
> +
> +       string_stream_destroy(stream);
> +}

Is this worth having here? It's only used once, and it could easily be
replaced with a manual call to string_stream_destroy() at the end of
the function.

If it were likely that list_empty() or string_stream_is_empty() would
crash, it would be more worthwhile to make sure the cleanup happens
anyway, but as-is, I think it's safe enough either way.

> +
>  static char *get_concatenated_string(struct kunit *test, struct string_stream *stream)
>  {
>         char *str = string_stream_get_string(stream);
> @@ -21,11 +35,12 @@ static char *get_concatenated_string(struct kunit *test, struct string_stream *s
>         return str;
>  }
>
> -/* string_stream object is initialized correctly. */
> -static void string_stream_init_test(struct kunit *test)
> +/* Managed string_stream object is initialized correctly. */
> +static void string_stream_managed_init_test(struct kunit *test)
>  {
>         struct string_stream *stream;
>
> +       /* Resource-managed initialization. */
>         stream = kunit_alloc_string_stream(test, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
>
> @@ -37,6 +52,101 @@ static void string_stream_init_test(struct kunit *test)
>         KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
>  }
>
> +/* Unmanaged string_stream object is initialized correctly. */
> +static void string_stream_unmanaged_init_test(struct kunit *test)
> +{
> +       struct string_stream *stream;
> +
> +       stream = alloc_string_stream(GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
> +       kunit_add_action(test, cleanup_raw_stream, stream);
> +
> +       KUNIT_EXPECT_EQ(test, stream->length, 0);
> +       KUNIT_EXPECT_TRUE(test, list_empty(&stream->fragments));
> +       KUNIT_EXPECT_EQ(test, stream->gfp, GFP_KERNEL);
> +       KUNIT_EXPECT_FALSE(test, stream->append_newlines);
> +
> +       KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
> +}
> +
> +static void string_stream_destroy_stub(struct string_stream *stream)
> +{
> +       struct kunit *fake_test = kunit_get_current_test();
> +       struct string_stream_test_priv *priv = fake_test->priv;
> +
> +       if (priv->freed_stream)
> +               priv->stream_free_again = true;
> +
> +       priv->freed_stream = stream;
> +
> +       /*
> +        * Avoid calling deactivate_static_stub() or changing
> +        * current->kunit_test during cleanup. Leave the stream to
> +        * be freed during the test exit.
> +        */

Are we ever actually freeing this during test exit? I don't think so.

I think you'd need to either free 'stream' here, or free it (either
manually or with a deferred action) at the end of every test which
uses this stub.

That being said, I'd agree it's best to avoid manually calling
deactivate_static_stub() and/or changin current->kunit_test during
cleanup. While it should actually be safe, as far as I can tell, it'd
be very confusing.

> +}
> +
> +/* kunit_free_string_stream() calls string_stream_desrtoy() */
> +static void string_stream_managed_free_test(struct kunit *test)
> +{
> +       struct string_stream_test_priv *priv = test->priv;
> +       struct string_stream *stream;
> +
> +       priv->freed_stream = NULL;
> +       priv->stream_free_again = false;
> +       kunit_activate_static_stub(test,
> +                                  string_stream_destroy,
> +                                  string_stream_destroy_stub);
> +
> +       stream = kunit_alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
> +
> +       /* This should call the stub function. */
> +       kunit_free_string_stream(test, stream);
> +
> +       KUNIT_EXPECT_PTR_EQ(test, priv->freed_stream, stream);
> +       KUNIT_EXPECT_FALSE(test, priv->stream_free_again);

Stream is never freed here?

> +}
> +
> +/* string_stream object is freed when test is cleaned up. */
> +static void string_stream_resource_free_test(struct kunit *test)
> +{
> +       struct string_stream_test_priv *priv = test->priv;
> +       struct kunit *fake_test;
> +       struct string_stream *stream;
> +
> +       fake_test = kunit_kzalloc(test, sizeof(*fake_test), GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fake_test);
> +
> +       kunit_init_test(fake_test, "string_stream_fake_test", NULL);
> +       fake_test->priv = priv;
> +
> +       /*
> +        * Activate stub before creating string_stream so the
> +        * string_stream will be cleaned up first.
> +        */
> +       priv->freed_stream = NULL;
> +       priv->stream_free_again = false;
> +       kunit_activate_static_stub(fake_test,
> +                                  string_stream_destroy,
> +                                  string_stream_destroy_stub);
> +
> +       stream = kunit_alloc_string_stream(fake_test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
> +
> +       /* Set current->kunit_test to fake_test so the static stub will be called. */
> +       current->kunit_test = fake_test;
> +
> +       /* Cleanup test - the stub function should be called */
> +       kunit_cleanup(fake_test);
> +
> +       /* Set current->kunit_test back to current test. */
> +       current->kunit_test = test;
> +
> +       KUNIT_EXPECT_PTR_EQ(test, priv->freed_stream, stream);
> +       KUNIT_EXPECT_FALSE(test, priv->stream_free_again);

We need to free stream here.


> +}
> +
>  /*
>   * Add a series of lines to a string_stream. Check that all lines
>   * appear in the correct order and no characters are dropped.
> @@ -327,8 +437,24 @@ static void string_stream_auto_newline_test(struct kunit *test)
>                            "One\nTwo\nThree\nFour\nFive\nSix\nSeven\n\nEight\n");
>  }
>
> +static int string_stream_test_init(struct kunit *test)
> +{
> +       struct string_stream_test_priv *priv;
> +
> +       priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       test->priv = priv;
> +
> +       return 0;
> +}
> +
>  static struct kunit_case string_stream_test_cases[] = {
> -       KUNIT_CASE(string_stream_init_test),
> +       KUNIT_CASE(string_stream_managed_init_test),
> +       KUNIT_CASE(string_stream_unmanaged_init_test),
> +       KUNIT_CASE(string_stream_managed_free_test),
> +       KUNIT_CASE(string_stream_resource_free_test),
>         KUNIT_CASE(string_stream_line_add_test),
>         KUNIT_CASE(string_stream_variable_length_line_test),
>         KUNIT_CASE(string_stream_append_test),
> @@ -341,6 +467,7 @@ static struct kunit_case string_stream_test_cases[] = {
>
>  static struct kunit_suite string_stream_test_suite = {
>         .name = "string-stream-test",
> -       .test_cases = string_stream_test_cases
> +       .test_cases = string_stream_test_cases,
> +       .init = string_stream_test_init,
>  };
>  kunit_test_suites(&string_stream_test_suite);
> diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
> index c39f1cba3bcd..d2ded5207e9e 100644
> --- a/lib/kunit/string-stream.c
> +++ b/lib/kunit/string-stream.c
> @@ -6,6 +6,7 @@
>   * Author: Brendan Higgins <brendanhiggins@google.com>
>   */
>
> +#include <kunit/static_stub.h>
>  #include <kunit/test.h>
>  #include <linux/list.h>
>  #include <linux/slab.h>
> @@ -170,6 +171,8 @@ struct string_stream *alloc_string_stream(gfp_t gfp)
>
>  void string_stream_destroy(struct string_stream *stream)
>  {
> +       KUNIT_STATIC_STUB_REDIRECT(string_stream_destroy, stream);
> +
>         if (!stream)
>                 return;
>
> --
> 2.30.2
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230824143129.1957914-9-rf%40opensource.cirrus.com.

On 25/08/2023 07:49, David Gow wrote:
> On Thu, 24 Aug 2023 at 22:32, 'Richard Fitzgerald' via KUnit
> Development <kunit-dev@googlegroups.com> wrote:
>>
>> string_stream_managed_free_test() allocates a resource-managed
>> string_stream and tests that kunit_free_string_stream() calls
>> string_stream_destroy().
>>
>> string_stream_resource_free_test() allocates a resource-managed
>> string_stream and tests that string_stream_destroy() is called
>> when the test resources are cleaned up.
>>
>> The old string_stream_init_test() has been split into two tests,
>> one for kunit_alloc_string_stream() and the other for
>> alloc_string_stream().
>>
>> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
>> ---
>> Changes since V4:
>> - Added test case for kunit_free_string_stream().
>> - Split the initialization test into separate tests for managed and
>>    unmanaged allocations.
>> ---
> 
> Looking over this again, I'm not convinced the streams are actually
> getting freed. Once the stub has finished, the stream is removed from
> the list of deferred actions / resources.
> 

Argh, I think you're right. My original version stashed the stream into
the private data and freed it in a test exit() function so that it was
guaranteed to be freed even if the resource cleanup wasn't called and
the test function aborted before it could do a manual cleanup.
I decided to simplify that but actually that original implementation was
better.