| Message ID | 20230824143129.1957914-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-rc7 next-20230824] [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-Don-t-create-a-fragment-for-empty-strings/20230824-223722 base: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git kunit patch link: https://lore.kernel.org/r/20230824143129.1957914-8-rf%40opensource.cirrus.com patch subject: [PATCH v5 07/10] kunit: string-stream: Decouple string_stream from kunit config: hexagon-randconfig-001-20230825 (https://download.01.org/0day-ci/archive/20230825/202308251401.n2nyRNLW-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/20230825/202308251401.n2nyRNLW-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/202308251401.n2nyRNLW-lkp@intel.com/ All warnings (new ones prefixed by >>): >> lib/kunit/string-stream-test.c:19:25: warning: cast from 'void (*)(const void *)' to 'kunit_action_t *' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict] 19 | kunit_add_action(test, (kunit_action_t *)kfree, (void *)str); | ^~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated. vim +19 lib/kunit/string-stream-test.c 13 14 static char *get_concatenated_string(struct kunit *test, struct string_stream *stream) 15 { 16 char *str = string_stream_get_string(stream); 17 18 KUNIT_ASSERT_NOT_ERR_OR_NULL(test, str); > 19 kunit_add_action(test, (kunit_action_t *)kfree, (void *)str); 20 21 return str; 22 } 23
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 >
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); 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); } }
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(). --- 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(-)