| Message ID | 20230814132309.32641-8-rf@opensource.cirrus.com |
|---|---|
| State | New |
| Headers | show |
| Series | kunit: Add dynamically-extending log | expand |
On Mon, 14 Aug 2023 at 21:23, 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 > 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@opensource.cirrus.com> > --- 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@google.com> > 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 >
On 15/8/23 10:16, David Gow wrote: > On Mon, 14 Aug 2023 at 21:23, 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 >> 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@opensource.cirrus.com> >> --- > > I'm in favour of this. Should we go further and get rid of the struct > kunit member from string_stream totally? > I can do that. I was worried about some hairy-looking code in assert.c that used stream->test. But I've just looked at it again and it's really quite simple, and doesn't even need ->test. is_literal() allocates a temporary managed buffer, but it frees it before returning so it doesn't need to be managed. > 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. > I can send a new version to fix that.
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); + + 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); } 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; + 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)
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@opensource.cirrus.com> --- 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(-)