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%40op....