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(-)
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);
}
}
--
2.30.2On 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
>
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
© 2016 - 2025 Red Hat, Inc.