| Message ID | 20230824143129.1957914-9-rf@opensource.cirrus.com |
|---|---|
| State | New |
| Headers | show |
| Series | kunit: Add dynamically-extending log | expand |
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.
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;
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(-)