| Message ID | 20230814132309.32641-8-rf@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | kunit: Add dynamically-extending log | expand |
Hi Richard, kernel test robot noticed the following build warnings: [auto build test WARNING on shuah-kselftest/kunit] [also build test WARNING on shuah-kselftest/kunit-fixes linus/master v6.5-rc6 next-20230809] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Richard-Fitzgerald/kunit-string-stream-Improve-testing-of-string_stream/20230814-212947 base: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git kunit patch link: https://lore.kernel.org/r/20230814132309.32641-8-rf%40opensource.cirrus.com patch subject: [PATCH v4 07/10] kunit: string-stream: Decouple string_stream from kunit config: hexagon-randconfig-r014-20230815 (https://download.01.org/0day-ci/archive/20230815/202308150347.LvFXkSdz-lkp@intel.com/config) compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a) reproduce: (https://download.01.org/0day-ci/archive/20230815/202308150347.LvFXkSdz-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202308150347.LvFXkSdz-lkp@intel.com/ All warnings (new ones prefixed by >>): >> lib/kunit/string-stream.c:199:38: warning: cast from 'void (*)(struct string_stream *)' to 'kunit_action_t *' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict] 199 | if (kunit_add_action_or_reset(test, (kunit_action_t *)raw_free_string_stream, stream) != 0) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/compiler.h:55:47: note: expanded from macro 'if' 55 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) ) | ^~~~ include/linux/compiler.h:57:52: note: expanded from macro '__trace_if_var' 57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) | ^~~~ >> lib/kunit/string-stream.c:199:38: warning: cast from 'void (*)(struct string_stream *)' to 'kunit_action_t *' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict] 199 | if (kunit_add_action_or_reset(test, (kunit_action_t *)raw_free_string_stream, stream) != 0) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/compiler.h:55:47: note: expanded from macro 'if' 55 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) ) | ^~~~ include/linux/compiler.h:57:61: note: expanded from macro '__trace_if_var' 57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) | ^~~~ >> lib/kunit/string-stream.c:199:38: warning: cast from 'void (*)(struct string_stream *)' to 'kunit_action_t *' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict] 199 | if (kunit_add_action_or_reset(test, (kunit_action_t *)raw_free_string_stream, stream) != 0) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/compiler.h:55:47: note: expanded from macro 'if' 55 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) ) | ^~~~ include/linux/compiler.h:57:86: note: expanded from macro '__trace_if_var' 57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) | ^~~~ include/linux/compiler.h:68:3: note: expanded from macro '__trace_if_value' 68 | (cond) ? \ | ^~~~ lib/kunit/string-stream.c:210:29: warning: cast from 'void (*)(struct string_stream *)' to 'kunit_action_t *' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict] 210 | kunit_release_action(test, (kunit_action_t *)raw_free_string_stream, (void *)stream); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 4 warnings generated. vim +199 lib/kunit/string-stream.c 188 189 struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp) 190 { 191 struct string_stream *stream; 192 193 stream = raw_alloc_string_stream(gfp); 194 if (IS_ERR(stream)) 195 return stream; 196 197 stream->test = test; 198 > 199 if (kunit_add_action_or_reset(test, (kunit_action_t *)raw_free_string_stream, stream) != 0) 200 return ERR_PTR(-ENOMEM); 201 202 return stream; 203 } 204
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.
On Tue, 15 Aug 2023 at 18:51, Richard Fitzgerald <rf@opensource.cirrus.com> wrote: > > 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. > Yeah, let's get rid of that. Having a stream->kunit exist but be NULL half the time is asking for issues down the line. > > 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. > That's probably best. If you want to keep it as-is, I'll fight for it, but it's probably better to err on the side of not introducing the warnings. Thanks, -- David
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(-)