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