[PATCH v5 00/10] kunit: Add dynamically-extending log

[PATCH v5 00/10] kunit: Add dynamically-extending log

[Richard Fitzgerald]

This patch chain changes the logging implementation to use string_stream
so that the log will grow dynamically.

The first 8 patches add test code for string_stream, and make some
changes to string_stream needed to be able to use it for the log.

The final patch adds a performance report of string_stream.

CHANGES SINCE V4:
- Re-ordered the first 3 patches from V4 to squash the first two sets
  of string_stream tests into a single patch.
- Changed is_literal() so it doesn't need a struct kunit.
- Split out the new resource-managed alloc and free functions into
  a pre-patch to reduce the amount of code churn when the string_stream
  is decoupled from kunit.
- Wrapped the call to string_stream_geT_string() in string-stream-test
  in a local function to reduce the amount of code churn when the
  string_stream is decoupled from kunit.
- Some minor changes to test implementations.
- string_stream is now completely separated from kunit and the 'test'
  member of struct string_stream has been eliminated.

Richard Fitzgerald (10):
  kunit: string-stream: Don't create a fragment for empty strings
  kunit: string-stream: Improve testing of string_stream
  kunit: string-stream: Add option to make all lines end with newline
  kunit: string-stream: Add cases for string_stream newline appending
  kunit: Don't use a managed alloc in is_literal()
  kunit: string-stream: Add kunit_alloc_string_stream()
  kunit: string-stream: Decouple string_stream from kunit
  kunit: string-stream: Add tests for freeing resource-managed
    string_stream
  kunit: Use string_stream for test log
  kunit: string-stream: Test performance of string_stream

 include/kunit/test.h           |  14 +-
 lib/kunit/assert.c             |  14 +-
 lib/kunit/debugfs.c            |  36 ++-
 lib/kunit/kunit-test.c         |  46 ++-
 lib/kunit/string-stream-test.c | 508 +++++++++++++++++++++++++++++++--
 lib/kunit/string-stream.c      | 100 +++++--
 lib/kunit/string-stream.h      |  16 +-
 lib/kunit/test.c               |  50 +---
 8 files changed, 662 insertions(+), 122 deletions(-)

-- 
2.30.2

[PATCH v5 01/10] kunit: string-stream: Don't create a fragment for empty strings

[Richard Fitzgerald]

If the result of the formatted string is an empty string just return
instead of creating an empty fragment.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 lib/kunit/string-stream.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
index cc32743c1171..ed24d86af9f5 100644
--- a/lib/kunit/string-stream.c
+++ b/lib/kunit/string-stream.c
@@ -50,11 +50,17 @@ int string_stream_vadd(struct string_stream *stream,
 	/* Make a copy because `vsnprintf` could change it */
 	va_copy(args_for_counting, args);
 
-	/* Need space for null byte. */
-	len = vsnprintf(NULL, 0, fmt, args_for_counting) + 1;
+	/* Evaluate length of formatted string */
+	len = vsnprintf(NULL, 0, fmt, args_for_counting);
 
 	va_end(args_for_counting);
 
+	if (len == 0)
+		return 0;
+
+	/* Need space for null byte. */
+	len++;
+
 	frag_container = alloc_string_stream_fragment(stream->test,
 						      len,
 						      stream->gfp);
-- 
2.30.2

Re: [PATCH v5 01/10] kunit: string-stream: Don't create a fragment for empty strings

[Rae Moar]

On Thu, Aug 24, 2023 at 10:32 AM 'Richard Fitzgerald' via KUnit
Development <kunit-dev@googlegroups.com> wrote:
>
> If the result of the formatted string is an empty string just return
> instead of creating an empty fragment.
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>

This looks good to me!

Reviewed-by: Rae Moar <rmoar@google.com>

Thanks!

-Rae

> ---
>  lib/kunit/string-stream.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
> index cc32743c1171..ed24d86af9f5 100644
> --- a/lib/kunit/string-stream.c
> +++ b/lib/kunit/string-stream.c
> @@ -50,11 +50,17 @@ int string_stream_vadd(struct string_stream *stream,
>         /* Make a copy because `vsnprintf` could change it */
>         va_copy(args_for_counting, args);
>
> -       /* Need space for null byte. */
> -       len = vsnprintf(NULL, 0, fmt, args_for_counting) + 1;
> +       /* Evaluate length of formatted string */
> +       len = vsnprintf(NULL, 0, fmt, args_for_counting);
>
>         va_end(args_for_counting);
>
> +       if (len == 0)
> +               return 0;
> +
> +       /* Need space for null byte. */
> +       len++;
> +
>         frag_container = alloc_string_stream_fragment(stream->test,
>                                                       len,
>                                                       stream->gfp);
> --
> 2.30.2
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230824143129.1957914-2-rf%40opensource.cirrus.com.

Re: [PATCH v5 01/10] kunit: string-stream: Don't create a fragment for empty strings

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 1790 bytes --]

On Thu, 24 Aug 2023 at 22:32, 'Richard Fitzgerald' via KUnit
Development <kunit-dev@googlegroups.com> wrote:
>
> If the result of the formatted string is an empty string just return
> instead of creating an empty fragment.
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---

Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David


>  lib/kunit/string-stream.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
> index cc32743c1171..ed24d86af9f5 100644
> --- a/lib/kunit/string-stream.c
> +++ b/lib/kunit/string-stream.c
> @@ -50,11 +50,17 @@ int string_stream_vadd(struct string_stream *stream,
>         /* Make a copy because `vsnprintf` could change it */
>         va_copy(args_for_counting, args);
>
> -       /* Need space for null byte. */
> -       len = vsnprintf(NULL, 0, fmt, args_for_counting) + 1;
> +       /* Evaluate length of formatted string */
> +       len = vsnprintf(NULL, 0, fmt, args_for_counting);
>
>         va_end(args_for_counting);
>
> +       if (len == 0)
> +               return 0;
> +
> +       /* Need space for null byte. */
> +       len++;
> +
>         frag_container = alloc_string_stream_fragment(stream->test,
>                                                       len,
>                                                       stream->gfp);
> --
> 2.30.2
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230824143129.1957914-2-rf%40opensource.cirrus.com.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

[PATCH v5 02/10] kunit: string-stream: Improve testing of string_stream

[Richard Fitzgerald]

Replace the minimal tests with more-thorough testing.

string_stream_init_test() tests that struct string_stream is
initialized correctly.

string_stream_line_add_test() adds a series of numbered lines and
checks that the resulting string contains all the lines.

string_stream_variable_length_line_test() adds a large number of
lines of varying length to create many fragments, then tests that all
lines are present.

string_stream_append_test() tests various cases of using
string_stream_append() to append the content of one stream to another.

Adds string_stream_append_empty_string_test() to test that adding an
empty string to a string_stream doesn't create a new empty fragment.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
Changes since V4:
- Test cases for appending empty strings have been squashed into this
  patch.
- Call to string_stream_get_string() is wrapped in a local function
  get_concatenated_string(). This is to avoid a lot of code churn later
  when string_stream_get_string() is changed to return an unmanaged buffer.
---
 lib/kunit/string-stream-test.c | 232 ++++++++++++++++++++++++++++++---
 1 file changed, 216 insertions(+), 16 deletions(-)

diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
index 110f3a993250..2b761ba01835 100644
--- a/lib/kunit/string-stream-test.c
+++ b/lib/kunit/string-stream-test.c
@@ -11,38 +11,238 @@
 
 #include "string-stream.h"
 
-static void string_stream_test_empty_on_creation(struct kunit *test)
+static char *get_concatenated_string(struct kunit *test, struct string_stream *stream)
 {
-	struct string_stream *stream = alloc_string_stream(test, GFP_KERNEL);
+	char *str = string_stream_get_string(stream);
+
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, str);
+
+	return str;
+}
+
+/* string_stream object is initialized correctly. */
+static void string_stream_init_test(struct kunit *test)
+{
+	struct string_stream *stream;
+
+	stream = alloc_string_stream(test, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
+
+	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_TRUE(test, string_stream_is_empty(stream));
 }
 
-static void string_stream_test_not_empty_after_add(struct kunit *test)
+/*
+ * Add a series of lines to a string_stream. Check that all lines
+ * appear in the correct order and no characters are dropped.
+ */
+static void string_stream_line_add_test(struct kunit *test)
 {
-	struct string_stream *stream = alloc_string_stream(test, GFP_KERNEL);
+	struct string_stream *stream;
+	char line[60];
+	char *concat_string, *pos, *string_end;
+	size_t len, total_len;
+	int num_lines, i;
 
-	string_stream_add(stream, "Foo");
+	stream = alloc_string_stream(test, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
 
-	KUNIT_EXPECT_FALSE(test, string_stream_is_empty(stream));
+	/* Add series of sequence numbered lines */
+	total_len = 0;
+	for (i = 0; i < 100; ++i) {
+		len = snprintf(line, sizeof(line),
+			"The quick brown fox jumps over the lazy penguin %d\n", i);
+
+		/* Sanity-check that our test string isn't truncated */
+		KUNIT_ASSERT_LT(test, len, sizeof(line));
+
+		string_stream_add(stream, line);
+		total_len += len;
+	}
+	num_lines = i;
+
+	concat_string = get_concatenated_string(test, stream);
+	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, concat_string);
+	KUNIT_EXPECT_EQ(test, strlen(concat_string), total_len);
+
+	/*
+	 * Split the concatenated string at the newlines and check that
+	 * all the original added strings are present.
+	 */
+	pos = concat_string;
+	for (i = 0; i < num_lines; ++i) {
+		string_end = strchr(pos, '\n');
+		KUNIT_EXPECT_NOT_NULL(test, string_end);
+
+		/* Convert to NULL-terminated string */
+		*string_end = '\0';
+
+		snprintf(line, sizeof(line),
+			 "The quick brown fox jumps over the lazy penguin %d", i);
+		KUNIT_EXPECT_STREQ(test, pos, line);
+
+		pos = string_end + 1;
+	}
+
+	/* There shouldn't be any more data after this */
+	KUNIT_EXPECT_EQ(test, strlen(pos), 0);
 }
 
-static void string_stream_test_get_string(struct kunit *test)
+/* Add a series of lines of variable length to a string_stream. */
+static void string_stream_variable_length_line_test(struct kunit *test)
 {
-	struct string_stream *stream = alloc_string_stream(test, GFP_KERNEL);
-	char *output;
+	static const char line[] =
+		"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"
+		" 0123456789!$%^&*()_-+={}[]:;@'~#<>,.?/|";
+	struct string_stream *stream;
+	struct rnd_state rnd;
+	char *concat_string, *pos, *string_end;
+	size_t offset, total_len;
+	int num_lines, i;
 
-	string_stream_add(stream, "Foo");
-	string_stream_add(stream, " %s", "bar");
+	stream = alloc_string_stream(test, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
 
-	output = string_stream_get_string(stream);
-	KUNIT_ASSERT_STREQ(test, output, "Foo bar");
+	/*
+	 * Log many lines of varying lengths until we have created
+	 * many fragments.
+	 * The "randomness" must be repeatable.
+	 */
+	prandom_seed_state(&rnd, 3141592653589793238ULL);
+	total_len = 0;
+	for (i = 0; i < 100; ++i) {
+		offset = prandom_u32_state(&rnd) % (sizeof(line) - 1);
+		string_stream_add(stream, "%s\n", &line[offset]);
+		total_len += sizeof(line) - offset;
+	}
+	num_lines = i;
+
+	concat_string = get_concatenated_string(test, stream);
+	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, concat_string);
+	KUNIT_EXPECT_EQ(test, strlen(concat_string), total_len);
+
+	/*
+	 * Split the concatenated string at the newlines and check that
+	 * all the original added strings are present.
+	 */
+	prandom_seed_state(&rnd, 3141592653589793238ULL);
+	pos = concat_string;
+	for (i = 0; i < num_lines; ++i) {
+		string_end = strchr(pos, '\n');
+		KUNIT_EXPECT_NOT_NULL(test, string_end);
+
+		/* Convert to NULL-terminated string */
+		*string_end = '\0';
+
+		offset = prandom_u32_state(&rnd) % (sizeof(line) - 1);
+		KUNIT_EXPECT_STREQ(test, pos, &line[offset]);
+
+		pos = string_end + 1;
+	}
+
+	/* There shouldn't be any more data after this */
+	KUNIT_EXPECT_EQ(test, strlen(pos), 0);
+}
+
+/* Appending the content of one string stream to another. */
+static void string_stream_append_test(struct kunit *test)
+{
+	static const char * const strings_1[] = {
+		"one", "two", "three", "four", "five", "six",
+		"seven", "eight", "nine", "ten",
+	};
+	static const char * const strings_2[] = {
+		"Apple", "Pear", "Orange", "Banana", "Grape", "Apricot",
+	};
+	struct string_stream *stream_1, *stream_2;
+	const char *original_content, *stream_2_content;
+	char *combined_content;
+	size_t combined_length;
+	int i;
+
+	stream_1 = alloc_string_stream(test, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
+
+	stream_2 = alloc_string_stream(test, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_2);
+
+	/* Append content of empty stream to empty stream */
+	string_stream_append(stream_1, stream_2);
+	KUNIT_EXPECT_EQ(test, strlen(get_concatenated_string(test, stream_1)), 0);
+
+	/* Add some data to stream_1 */
+	for (i = 0; i < ARRAY_SIZE(strings_1); ++i)
+		string_stream_add(stream_1, "%s\n", strings_1[i]);
+
+	original_content = get_concatenated_string(test, stream_1);
+
+	/* Append content of empty stream to non-empty stream */
+	string_stream_append(stream_1, stream_2);
+	KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1), original_content);
+
+	/* Add some data to stream_2 */
+	for (i = 0; i < ARRAY_SIZE(strings_2); ++i)
+		string_stream_add(stream_2, "%s\n", strings_2[i]);
+
+	/* Append content of non-empty stream to non-empty stream */
+	string_stream_append(stream_1, stream_2);
+
+	/*
+	 * End result should be the original content of stream_1 plus
+	 * the content of stream_2.
+	 */
+	stream_2_content = get_concatenated_string(test, stream_2);
+	combined_length = strlen(original_content) + strlen(stream_2_content);
+	combined_length++; /* for terminating \0 */
+	combined_content = kunit_kmalloc(test, combined_length, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, combined_content);
+	snprintf(combined_content, combined_length, "%s%s", original_content, stream_2_content);
+
+	KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1), combined_content);
+
+	/* Append content of non-empty stream to empty stream */
+	string_stream_destroy(stream_1);
+
+	stream_1 = alloc_string_stream(test, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
+
+	string_stream_append(stream_1, stream_2);
+	KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1), stream_2_content);
+}
+
+/* Adding an empty string should not create a fragment. */
+static void string_stream_append_empty_string_test(struct kunit *test)
+{
+	struct string_stream *stream;
+	int original_frag_count;
+
+	stream = alloc_string_stream(test, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
+
+	/* Formatted empty string */
+	string_stream_add(stream, "%s", "");
+	KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
+	KUNIT_EXPECT_TRUE(test, list_empty(&stream->fragments));
+
+	/* Adding an empty string to a non-empty stream */
+	string_stream_add(stream, "Add this line");
+	original_frag_count = list_count_nodes(&stream->fragments);
+
+	string_stream_add(stream, "%s", "");
+	KUNIT_EXPECT_EQ(test, list_count_nodes(&stream->fragments), original_frag_count);
+	KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream), "Add this line");
 }
 
 static struct kunit_case string_stream_test_cases[] = {
-	KUNIT_CASE(string_stream_test_empty_on_creation),
-	KUNIT_CASE(string_stream_test_not_empty_after_add),
-	KUNIT_CASE(string_stream_test_get_string),
+	KUNIT_CASE(string_stream_init_test),
+	KUNIT_CASE(string_stream_line_add_test),
+	KUNIT_CASE(string_stream_variable_length_line_test),
+	KUNIT_CASE(string_stream_append_test),
+	KUNIT_CASE(string_stream_append_empty_string_test),
 	{}
 };
 
-- 
2.30.2

Re: [PATCH v5 02/10] kunit: string-stream: Improve testing of string_stream

[Rae Moar]

On Thu, Aug 24, 2023 at 10:31 AM Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> Replace the minimal tests with more-thorough testing.
>
> string_stream_init_test() tests that struct string_stream is
> initialized correctly.
>
> string_stream_line_add_test() adds a series of numbered lines and
> checks that the resulting string contains all the lines.
>
> string_stream_variable_length_line_test() adds a large number of
> lines of varying length to create many fragments, then tests that all
> lines are present.
>
> string_stream_append_test() tests various cases of using
> string_stream_append() to append the content of one stream to another.
>
> Adds string_stream_append_empty_string_test() to test that adding an
> empty string to a string_stream doesn't create a new empty fragment.
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>

Hello!

These tests all look good to me. I like all of the details and
comments. Great to see these additions to the string-stream-test!

Reviewed-by: Rae Moar <rmoar@google.com>

Thanks!
-Rae

> ---
> Changes since V4:
> - Test cases for appending empty strings have been squashed into this
>   patch.
> - Call to string_stream_get_string() is wrapped in a local function
>   get_concatenated_string(). This is to avoid a lot of code churn later
>   when string_stream_get_string() is changed to return an unmanaged buffer.
> ---
>  lib/kunit/string-stream-test.c | 232 ++++++++++++++++++++++++++++++---
>  1 file changed, 216 insertions(+), 16 deletions(-)
>
> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
> index 110f3a993250..2b761ba01835 100644
> --- a/lib/kunit/string-stream-test.c
> +++ b/lib/kunit/string-stream-test.c
> @@ -11,38 +11,238 @@
>
>  #include "string-stream.h"
>
> -static void string_stream_test_empty_on_creation(struct kunit *test)
> +static char *get_concatenated_string(struct kunit *test, struct string_stream *stream)
>  {
> -       struct string_stream *stream = alloc_string_stream(test, GFP_KERNEL);
> +       char *str = string_stream_get_string(stream);
> +
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, str);
> +
> +       return str;
> +}
> +
> +/* string_stream object is initialized correctly. */
> +static void string_stream_init_test(struct kunit *test)
> +{
> +       struct string_stream *stream;
> +
> +       stream = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
> +
> +       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);

As mentioned in the last version, if this causes a warning we will
look into it on the KUnit side.

>
>         KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
>  }
>
> -static void string_stream_test_not_empty_after_add(struct kunit *test)
> +/*
> + * Add a series of lines to a string_stream. Check that all lines
> + * appear in the correct order and no characters are dropped.
> + */
> +static void string_stream_line_add_test(struct kunit *test)
>  {
> -       struct string_stream *stream = alloc_string_stream(test, GFP_KERNEL);
> +       struct string_stream *stream;
> +       char line[60];
> +       char *concat_string, *pos, *string_end;
> +       size_t len, total_len;
> +       int num_lines, i;
>
> -       string_stream_add(stream, "Foo");
> +       stream = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
>
> -       KUNIT_EXPECT_FALSE(test, string_stream_is_empty(stream));
> +       /* Add series of sequence numbered lines */
> +       total_len = 0;
> +       for (i = 0; i < 100; ++i) {
> +               len = snprintf(line, sizeof(line),
> +                       "The quick brown fox jumps over the lazy penguin %d\n", i);
> +
> +               /* Sanity-check that our test string isn't truncated */
> +               KUNIT_ASSERT_LT(test, len, sizeof(line));
> +
> +               string_stream_add(stream, line);
> +               total_len += len;
> +       }
> +       num_lines = i;
> +
> +       concat_string = get_concatenated_string(test, stream);
> +       KUNIT_EXPECT_NOT_ERR_OR_NULL(test, concat_string);
> +       KUNIT_EXPECT_EQ(test, strlen(concat_string), total_len);
> +
> +       /*
> +        * Split the concatenated string at the newlines and check that
> +        * all the original added strings are present.
> +        */
> +       pos = concat_string;
> +       for (i = 0; i < num_lines; ++i) {
> +               string_end = strchr(pos, '\n');
> +               KUNIT_EXPECT_NOT_NULL(test, string_end);
> +
> +               /* Convert to NULL-terminated string */
> +               *string_end = '\0';
> +
> +               snprintf(line, sizeof(line),
> +                        "The quick brown fox jumps over the lazy penguin %d", i);
> +               KUNIT_EXPECT_STREQ(test, pos, line);
> +
> +               pos = string_end + 1;
> +       }
> +
> +       /* There shouldn't be any more data after this */
> +       KUNIT_EXPECT_EQ(test, strlen(pos), 0);
>  }
>
> -static void string_stream_test_get_string(struct kunit *test)
> +/* Add a series of lines of variable length to a string_stream. */
> +static void string_stream_variable_length_line_test(struct kunit *test)
>  {
> -       struct string_stream *stream = alloc_string_stream(test, GFP_KERNEL);
> -       char *output;
> +       static const char line[] =
> +               "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"
> +               " 0123456789!$%^&*()_-+={}[]:;@'~#<>,.?/|";
> +       struct string_stream *stream;
> +       struct rnd_state rnd;
> +       char *concat_string, *pos, *string_end;
> +       size_t offset, total_len;
> +       int num_lines, i;
>
> -       string_stream_add(stream, "Foo");
> -       string_stream_add(stream, " %s", "bar");
> +       stream = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
>
> -       output = string_stream_get_string(stream);
> -       KUNIT_ASSERT_STREQ(test, output, "Foo bar");
> +       /*
> +        * Log many lines of varying lengths until we have created
> +        * many fragments.
> +        * The "randomness" must be repeatable.
> +        */
> +       prandom_seed_state(&rnd, 3141592653589793238ULL);
> +       total_len = 0;
> +       for (i = 0; i < 100; ++i) {
> +               offset = prandom_u32_state(&rnd) % (sizeof(line) - 1);
> +               string_stream_add(stream, "%s\n", &line[offset]);
> +               total_len += sizeof(line) - offset;
> +       }
> +       num_lines = i;
> +
> +       concat_string = get_concatenated_string(test, stream);
> +       KUNIT_EXPECT_NOT_ERR_OR_NULL(test, concat_string);
> +       KUNIT_EXPECT_EQ(test, strlen(concat_string), total_len);
> +
> +       /*
> +        * Split the concatenated string at the newlines and check that
> +        * all the original added strings are present.
> +        */
> +       prandom_seed_state(&rnd, 3141592653589793238ULL);
> +       pos = concat_string;
> +       for (i = 0; i < num_lines; ++i) {
> +               string_end = strchr(pos, '\n');
> +               KUNIT_EXPECT_NOT_NULL(test, string_end);
> +
> +               /* Convert to NULL-terminated string */
> +               *string_end = '\0';
> +
> +               offset = prandom_u32_state(&rnd) % (sizeof(line) - 1);
> +               KUNIT_EXPECT_STREQ(test, pos, &line[offset]);
> +
> +               pos = string_end + 1;
> +       }
> +
> +       /* There shouldn't be any more data after this */
> +       KUNIT_EXPECT_EQ(test, strlen(pos), 0);
> +}
> +
> +/* Appending the content of one string stream to another. */
> +static void string_stream_append_test(struct kunit *test)
> +{
> +       static const char * const strings_1[] = {
> +               "one", "two", "three", "four", "five", "six",
> +               "seven", "eight", "nine", "ten",
> +       };
> +       static const char * const strings_2[] = {
> +               "Apple", "Pear", "Orange", "Banana", "Grape", "Apricot",
> +       };
> +       struct string_stream *stream_1, *stream_2;
> +       const char *original_content, *stream_2_content;

I would maybe consider changing the name original_content to
stream_1_content but definitely not worth it as this version looks
very good.

> +       char *combined_content;
> +       size_t combined_length;
> +       int i;
> +
> +       stream_1 = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
> +
> +       stream_2 = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_2);
> +
> +       /* Append content of empty stream to empty stream */
> +       string_stream_append(stream_1, stream_2);
> +       KUNIT_EXPECT_EQ(test, strlen(get_concatenated_string(test, stream_1)), 0);
> +
> +       /* Add some data to stream_1 */
> +       for (i = 0; i < ARRAY_SIZE(strings_1); ++i)
> +               string_stream_add(stream_1, "%s\n", strings_1[i]);
> +
> +       original_content = get_concatenated_string(test, stream_1);
> +
> +       /* Append content of empty stream to non-empty stream */
> +       string_stream_append(stream_1, stream_2);
> +       KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1), original_content);
> +
> +       /* Add some data to stream_2 */
> +       for (i = 0; i < ARRAY_SIZE(strings_2); ++i)
> +               string_stream_add(stream_2, "%s\n", strings_2[i]);
> +
> +       /* Append content of non-empty stream to non-empty stream */
> +       string_stream_append(stream_1, stream_2);
> +
> +       /*
> +        * End result should be the original content of stream_1 plus
> +        * the content of stream_2.
> +        */
> +       stream_2_content = get_concatenated_string(test, stream_2);
> +       combined_length = strlen(original_content) + strlen(stream_2_content);
> +       combined_length++; /* for terminating \0 */
> +       combined_content = kunit_kmalloc(test, combined_length, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, combined_content);
> +       snprintf(combined_content, combined_length, "%s%s", original_content, stream_2_content);
> +
> +       KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1), combined_content);
> +
> +       /* Append content of non-empty stream to empty stream */
> +       string_stream_destroy(stream_1);
> +
> +       stream_1 = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
> +
> +       string_stream_append(stream_1, stream_2);
> +       KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1), stream_2_content);
> +}
> +
> +/* Adding an empty string should not create a fragment. */
> +static void string_stream_append_empty_string_test(struct kunit *test)
> +{
> +       struct string_stream *stream;
> +       int original_frag_count;
> +
> +       stream = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
> +
> +       /* Formatted empty string */
> +       string_stream_add(stream, "%s", "");
> +       KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
> +       KUNIT_EXPECT_TRUE(test, list_empty(&stream->fragments));
> +
> +       /* Adding an empty string to a non-empty stream */
> +       string_stream_add(stream, "Add this line");
> +       original_frag_count = list_count_nodes(&stream->fragments);
> +
> +       string_stream_add(stream, "%s", "");
> +       KUNIT_EXPECT_EQ(test, list_count_nodes(&stream->fragments), original_frag_count);
> +       KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream), "Add this line");
>  }
>
>  static struct kunit_case string_stream_test_cases[] = {
> -       KUNIT_CASE(string_stream_test_empty_on_creation),
> -       KUNIT_CASE(string_stream_test_not_empty_after_add),
> -       KUNIT_CASE(string_stream_test_get_string),
> +       KUNIT_CASE(string_stream_init_test),
> +       KUNIT_CASE(string_stream_line_add_test),
> +       KUNIT_CASE(string_stream_variable_length_line_test),
> +       KUNIT_CASE(string_stream_append_test),
> +       KUNIT_CASE(string_stream_append_empty_string_test),
>         {}
>  };
>
> --
> 2.30.2
>

Re: [PATCH v5 02/10] kunit: string-stream: Improve testing of string_stream

[Richard Fitzgerald]

On 24/08/2023 23:42, Rae Moar wrote:
> On Thu, Aug 24, 2023 at 10:31 AM Richard Fitzgerald
> <rf@opensource.cirrus.com> wrote:
>>
>> Replace the minimal tests with more-thorough testing.
>>

<SNIP>

>> +       KUNIT_EXPECT_EQ(test, stream->gfp, GFP_KERNEL);
> 
> As mentioned in the last version, if this causes a warning we will
> look into it on the KUnit side.
>

It does. I left it because you said you'd do a fix.
But maybe it's better to change it to

KUNIT_EXPECT_TRUE(test, stream_gfp == GFP_KERNEL);

to avoid the warning for now.

Re: [PATCH v5 02/10] kunit: string-stream: Improve testing of string_stream

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 12165 bytes --]

On Thu, 24 Aug 2023 at 22:33, Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> Replace the minimal tests with more-thorough testing.
>
> string_stream_init_test() tests that struct string_stream is
> initialized correctly.
>
> string_stream_line_add_test() adds a series of numbered lines and
> checks that the resulting string contains all the lines.
>
> string_stream_variable_length_line_test() adds a large number of
> lines of varying length to create many fragments, then tests that all
> lines are present.
>
> string_stream_append_test() tests various cases of using
> string_stream_append() to append the content of one stream to another.
>
> Adds string_stream_append_empty_string_test() to test that adding an
> empty string to a string_stream doesn't create a new empty fragment.
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
> Changes since V4:
> - Test cases for appending empty strings have been squashed into this
>   patch.
> - Call to string_stream_get_string() is wrapped in a local function
>   get_concatenated_string(). This is to avoid a lot of code churn later
>   when string_stream_get_string() is changed to return an unmanaged buffer.
> ---

This looks good to me. I'm not 100% sold on the
'get_concatenated_string()' function long-term (despite its obvious
benefits during the refactoring), but that's just personal taste. This
version is fine regardless.

Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David


>  lib/kunit/string-stream-test.c | 232 ++++++++++++++++++++++++++++++---
>  1 file changed, 216 insertions(+), 16 deletions(-)
>
> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
> index 110f3a993250..2b761ba01835 100644
> --- a/lib/kunit/string-stream-test.c
> +++ b/lib/kunit/string-stream-test.c
> @@ -11,38 +11,238 @@
>
>  #include "string-stream.h"
>
> -static void string_stream_test_empty_on_creation(struct kunit *test)
> +static char *get_concatenated_string(struct kunit *test, struct string_stream *stream)
>  {
> -       struct string_stream *stream = alloc_string_stream(test, GFP_KERNEL);
> +       char *str = string_stream_get_string(stream);
> +
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, str);
> +
> +       return str;
> +}
> +
> +/* string_stream object is initialized correctly. */
> +static void string_stream_init_test(struct kunit *test)
> +{
> +       struct string_stream *stream;
> +
> +       stream = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
> +
> +       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_TRUE(test, string_stream_is_empty(stream));
>  }
>
> -static void string_stream_test_not_empty_after_add(struct kunit *test)
> +/*
> + * Add a series of lines to a string_stream. Check that all lines
> + * appear in the correct order and no characters are dropped.
> + */
> +static void string_stream_line_add_test(struct kunit *test)
>  {
> -       struct string_stream *stream = alloc_string_stream(test, GFP_KERNEL);
> +       struct string_stream *stream;
> +       char line[60];
> +       char *concat_string, *pos, *string_end;
> +       size_t len, total_len;
> +       int num_lines, i;
>
> -       string_stream_add(stream, "Foo");
> +       stream = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
>
> -       KUNIT_EXPECT_FALSE(test, string_stream_is_empty(stream));
> +       /* Add series of sequence numbered lines */
> +       total_len = 0;
> +       for (i = 0; i < 100; ++i) {
> +               len = snprintf(line, sizeof(line),
> +                       "The quick brown fox jumps over the lazy penguin %d\n", i);
> +
> +               /* Sanity-check that our test string isn't truncated */
> +               KUNIT_ASSERT_LT(test, len, sizeof(line));
> +
> +               string_stream_add(stream, line);
> +               total_len += len;
> +       }
> +       num_lines = i;
> +
> +       concat_string = get_concatenated_string(test, stream);
> +       KUNIT_EXPECT_NOT_ERR_OR_NULL(test, concat_string);
> +       KUNIT_EXPECT_EQ(test, strlen(concat_string), total_len);
> +
> +       /*
> +        * Split the concatenated string at the newlines and check that
> +        * all the original added strings are present.
> +        */
> +       pos = concat_string;
> +       for (i = 0; i < num_lines; ++i) {
> +               string_end = strchr(pos, '\n');
> +               KUNIT_EXPECT_NOT_NULL(test, string_end);
> +
> +               /* Convert to NULL-terminated string */
> +               *string_end = '\0';
> +
> +               snprintf(line, sizeof(line),
> +                        "The quick brown fox jumps over the lazy penguin %d", i);
> +               KUNIT_EXPECT_STREQ(test, pos, line);
> +
> +               pos = string_end + 1;
> +       }
> +
> +       /* There shouldn't be any more data after this */
> +       KUNIT_EXPECT_EQ(test, strlen(pos), 0);
>  }
>
> -static void string_stream_test_get_string(struct kunit *test)
> +/* Add a series of lines of variable length to a string_stream. */
> +static void string_stream_variable_length_line_test(struct kunit *test)
>  {
> -       struct string_stream *stream = alloc_string_stream(test, GFP_KERNEL);
> -       char *output;
> +       static const char line[] =
> +               "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"
> +               " 0123456789!$%^&*()_-+={}[]:;@'~#<>,.?/|";
> +       struct string_stream *stream;
> +       struct rnd_state rnd;
> +       char *concat_string, *pos, *string_end;
> +       size_t offset, total_len;
> +       int num_lines, i;
>
> -       string_stream_add(stream, "Foo");
> -       string_stream_add(stream, " %s", "bar");
> +       stream = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
>
> -       output = string_stream_get_string(stream);
> -       KUNIT_ASSERT_STREQ(test, output, "Foo bar");
> +       /*
> +        * Log many lines of varying lengths until we have created
> +        * many fragments.
> +        * The "randomness" must be repeatable.
> +        */
> +       prandom_seed_state(&rnd, 3141592653589793238ULL);
> +       total_len = 0;
> +       for (i = 0; i < 100; ++i) {
> +               offset = prandom_u32_state(&rnd) % (sizeof(line) - 1);
> +               string_stream_add(stream, "%s\n", &line[offset]);
> +               total_len += sizeof(line) - offset;
> +       }
> +       num_lines = i;
> +
> +       concat_string = get_concatenated_string(test, stream);
> +       KUNIT_EXPECT_NOT_ERR_OR_NULL(test, concat_string);
> +       KUNIT_EXPECT_EQ(test, strlen(concat_string), total_len);
> +
> +       /*
> +        * Split the concatenated string at the newlines and check that
> +        * all the original added strings are present.
> +        */
> +       prandom_seed_state(&rnd, 3141592653589793238ULL);
> +       pos = concat_string;
> +       for (i = 0; i < num_lines; ++i) {
> +               string_end = strchr(pos, '\n');
> +               KUNIT_EXPECT_NOT_NULL(test, string_end);
> +
> +               /* Convert to NULL-terminated string */
> +               *string_end = '\0';
> +
> +               offset = prandom_u32_state(&rnd) % (sizeof(line) - 1);
> +               KUNIT_EXPECT_STREQ(test, pos, &line[offset]);
> +
> +               pos = string_end + 1;
> +       }
> +
> +       /* There shouldn't be any more data after this */
> +       KUNIT_EXPECT_EQ(test, strlen(pos), 0);
> +}
> +
> +/* Appending the content of one string stream to another. */
> +static void string_stream_append_test(struct kunit *test)
> +{
> +       static const char * const strings_1[] = {
> +               "one", "two", "three", "four", "five", "six",
> +               "seven", "eight", "nine", "ten",
> +       };
> +       static const char * const strings_2[] = {
> +               "Apple", "Pear", "Orange", "Banana", "Grape", "Apricot",
> +       };
> +       struct string_stream *stream_1, *stream_2;
> +       const char *original_content, *stream_2_content;
> +       char *combined_content;
> +       size_t combined_length;
> +       int i;
> +
> +       stream_1 = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
> +
> +       stream_2 = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_2);
> +
> +       /* Append content of empty stream to empty stream */
> +       string_stream_append(stream_1, stream_2);
> +       KUNIT_EXPECT_EQ(test, strlen(get_concatenated_string(test, stream_1)), 0);
> +
> +       /* Add some data to stream_1 */
> +       for (i = 0; i < ARRAY_SIZE(strings_1); ++i)
> +               string_stream_add(stream_1, "%s\n", strings_1[i]);
> +
> +       original_content = get_concatenated_string(test, stream_1);
> +
> +       /* Append content of empty stream to non-empty stream */
> +       string_stream_append(stream_1, stream_2);
> +       KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1), original_content);
> +
> +       /* Add some data to stream_2 */
> +       for (i = 0; i < ARRAY_SIZE(strings_2); ++i)
> +               string_stream_add(stream_2, "%s\n", strings_2[i]);
> +
> +       /* Append content of non-empty stream to non-empty stream */
> +       string_stream_append(stream_1, stream_2);
> +
> +       /*
> +        * End result should be the original content of stream_1 plus
> +        * the content of stream_2.
> +        */
> +       stream_2_content = get_concatenated_string(test, stream_2);
> +       combined_length = strlen(original_content) + strlen(stream_2_content);
> +       combined_length++; /* for terminating \0 */
> +       combined_content = kunit_kmalloc(test, combined_length, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, combined_content);
> +       snprintf(combined_content, combined_length, "%s%s", original_content, stream_2_content);
> +
> +       KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1), combined_content);
> +
> +       /* Append content of non-empty stream to empty stream */
> +       string_stream_destroy(stream_1);
> +
> +       stream_1 = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
> +
> +       string_stream_append(stream_1, stream_2);
> +       KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1), stream_2_content);
> +}
> +
> +/* Adding an empty string should not create a fragment. */
> +static void string_stream_append_empty_string_test(struct kunit *test)
> +{
> +       struct string_stream *stream;
> +       int original_frag_count;
> +
> +       stream = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
> +
> +       /* Formatted empty string */
> +       string_stream_add(stream, "%s", "");
> +       KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
> +       KUNIT_EXPECT_TRUE(test, list_empty(&stream->fragments));
> +
> +       /* Adding an empty string to a non-empty stream */
> +       string_stream_add(stream, "Add this line");
> +       original_frag_count = list_count_nodes(&stream->fragments);
> +
> +       string_stream_add(stream, "%s", "");
> +       KUNIT_EXPECT_EQ(test, list_count_nodes(&stream->fragments), original_frag_count);
> +       KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream), "Add this line");
>  }
>
>  static struct kunit_case string_stream_test_cases[] = {
> -       KUNIT_CASE(string_stream_test_empty_on_creation),
> -       KUNIT_CASE(string_stream_test_not_empty_after_add),
> -       KUNIT_CASE(string_stream_test_get_string),
> +       KUNIT_CASE(string_stream_init_test),
> +       KUNIT_CASE(string_stream_line_add_test),
> +       KUNIT_CASE(string_stream_variable_length_line_test),
> +       KUNIT_CASE(string_stream_append_test),
> +       KUNIT_CASE(string_stream_append_empty_string_test),
>         {}
>  };
>
> --
> 2.30.2
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH v5 02/10] kunit: string-stream: Improve testing of string_stream

[Richard Fitzgerald]

On 25/08/2023 07:49, David Gow wrote:
> On Thu, 24 Aug 2023 at 22:33, Richard Fitzgerald
> <rf@opensource.cirrus.com> wrote:
>>
>> Replace the minimal tests with more-thorough testing.
>>
>> string_stream_init_test() tests that struct string_stream is
>> initialized correctly.
>>
>> string_stream_line_add_test() adds a series of numbered lines and
>> checks that the resulting string contains all the lines.
>>
>> string_stream_variable_length_line_test() adds a large number of
>> lines of varying length to create many fragments, then tests that all
>> lines are present.
>>
>> string_stream_append_test() tests various cases of using
>> string_stream_append() to append the content of one stream to another.
>>
>> Adds string_stream_append_empty_string_test() to test that adding an
>> empty string to a string_stream doesn't create a new empty fragment.
>>
>> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
>> ---
>> Changes since V4:
>> - Test cases for appending empty strings have been squashed into this
>>    patch.
>> - Call to string_stream_get_string() is wrapped in a local function
>>    get_concatenated_string(). This is to avoid a lot of code churn later
>>    when string_stream_get_string() is changed to return an unmanaged buffer.
>> ---
> 
> This looks good to me. I'm not 100% sold on the
> 'get_concatenated_string()' function long-term (despite its obvious
> benefits during the refactoring), but that's just personal taste. This
> version is fine regardless.
> 

Yes, we can remove it later. I just wanted to avoid having one enormous
patch that changes everything all over the place.

[PATCH v5 03/10] kunit: string-stream: Add option to make all lines end with newline

[Richard Fitzgerald]

Add an optional feature to string_stream that will append a newline to
any added string that does not already end with a newline. The purpose
of this is so that string_stream can be used to collect log lines.

This is enabled/disabled by calling string_stream_set_append_newlines().

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 lib/kunit/string-stream.c | 28 +++++++++++++++++++++-------
 lib/kunit/string-stream.h |  7 +++++++
 2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
index ed24d86af9f5..1dcf6513b692 100644
--- a/lib/kunit/string-stream.c
+++ b/lib/kunit/string-stream.c
@@ -44,32 +44,46 @@ int string_stream_vadd(struct string_stream *stream,
 		       va_list args)
 {
 	struct string_stream_fragment *frag_container;
-	int len;
+	int buf_len, result_len;
 	va_list args_for_counting;
 
 	/* Make a copy because `vsnprintf` could change it */
 	va_copy(args_for_counting, args);
 
 	/* Evaluate length of formatted string */
-	len = vsnprintf(NULL, 0, fmt, args_for_counting);
+	buf_len = vsnprintf(NULL, 0, fmt, args_for_counting);
 
 	va_end(args_for_counting);
 
-	if (len == 0)
+	if (buf_len == 0)
 		return 0;
 
+	/* Reserve one extra for possible appended newline. */
+	if (stream->append_newlines)
+		buf_len++;
+
 	/* Need space for null byte. */
-	len++;
+	buf_len++;
 
 	frag_container = alloc_string_stream_fragment(stream->test,
-						      len,
+						      buf_len,
 						      stream->gfp);
 	if (IS_ERR(frag_container))
 		return PTR_ERR(frag_container);
 
-	len = vsnprintf(frag_container->fragment, len, fmt, args);
+	if (stream->append_newlines) {
+		/* Don't include reserved newline byte in writeable length. */
+		result_len = vsnprintf(frag_container->fragment, buf_len - 1, fmt, args);
+
+		/* Append newline if necessary. */
+		if (frag_container->fragment[result_len - 1] != '\n')
+			result_len = strlcat(frag_container->fragment, "\n", buf_len);
+	} else {
+		result_len = vsnprintf(frag_container->fragment, buf_len, fmt, args);
+	}
+
 	spin_lock(&stream->lock);
-	stream->length += len;
+	stream->length += result_len;
 	list_add_tail(&frag_container->node, &stream->fragments);
 	spin_unlock(&stream->lock);
 
diff --git a/lib/kunit/string-stream.h b/lib/kunit/string-stream.h
index b669f9a75a94..048930bf97f0 100644
--- a/lib/kunit/string-stream.h
+++ b/lib/kunit/string-stream.h
@@ -25,6 +25,7 @@ struct string_stream {
 	spinlock_t lock;
 	struct kunit *test;
 	gfp_t gfp;
+	bool append_newlines;
 };
 
 struct kunit;
@@ -47,4 +48,10 @@ bool string_stream_is_empty(struct string_stream *stream);
 
 void string_stream_destroy(struct string_stream *stream);
 
+static inline void string_stream_set_append_newlines(struct string_stream *stream,
+						     bool append_newlines)
+{
+	stream->append_newlines = append_newlines;
+}
+
 #endif /* _KUNIT_STRING_STREAM_H */
-- 
2.30.2

Re: [PATCH v5 03/10] kunit: string-stream: Add option to make all lines end with newline

[Rae Moar]

On Thu, Aug 24, 2023 at 10:32 AM Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> Add an optional feature to string_stream that will append a newline to
> any added string that does not already end with a newline. The purpose
> of this is so that string_stream can be used to collect log lines.
>
> This is enabled/disabled by calling string_stream_set_append_newlines().
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>

Hello,

This again looks good to me!

Reviewed-by: Rae Moar <rmoar@google.com>

Thanks!
-Rae

> ---
>  lib/kunit/string-stream.c | 28 +++++++++++++++++++++-------
>  lib/kunit/string-stream.h |  7 +++++++
>  2 files changed, 28 insertions(+), 7 deletions(-)
>
> diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
> index ed24d86af9f5..1dcf6513b692 100644
> --- a/lib/kunit/string-stream.c
> +++ b/lib/kunit/string-stream.c
> @@ -44,32 +44,46 @@ int string_stream_vadd(struct string_stream *stream,
>                        va_list args)
>  {
>         struct string_stream_fragment *frag_container;
> -       int len;
> +       int buf_len, result_len;
>         va_list args_for_counting;
>
>         /* Make a copy because `vsnprintf` could change it */
>         va_copy(args_for_counting, args);
>
>         /* Evaluate length of formatted string */
> -       len = vsnprintf(NULL, 0, fmt, args_for_counting);
> +       buf_len = vsnprintf(NULL, 0, fmt, args_for_counting);
>
>         va_end(args_for_counting);
>
> -       if (len == 0)
> +       if (buf_len == 0)
>                 return 0;
>
> +       /* Reserve one extra for possible appended newline. */
> +       if (stream->append_newlines)
> +               buf_len++;
> +
>         /* Need space for null byte. */
> -       len++;
> +       buf_len++;
>
>         frag_container = alloc_string_stream_fragment(stream->test,
> -                                                     len,
> +                                                     buf_len,
>                                                       stream->gfp);
>         if (IS_ERR(frag_container))
>                 return PTR_ERR(frag_container);
>
> -       len = vsnprintf(frag_container->fragment, len, fmt, args);
> +       if (stream->append_newlines) {
> +               /* Don't include reserved newline byte in writeable length. */
> +               result_len = vsnprintf(frag_container->fragment, buf_len - 1, fmt, args);
> +
> +               /* Append newline if necessary. */
> +               if (frag_container->fragment[result_len - 1] != '\n')
> +                       result_len = strlcat(frag_container->fragment, "\n", buf_len);
> +       } else {
> +               result_len = vsnprintf(frag_container->fragment, buf_len, fmt, args);
> +       }
> +
>         spin_lock(&stream->lock);
> -       stream->length += len;
> +       stream->length += result_len;
>         list_add_tail(&frag_container->node, &stream->fragments);
>         spin_unlock(&stream->lock);
>
> diff --git a/lib/kunit/string-stream.h b/lib/kunit/string-stream.h
> index b669f9a75a94..048930bf97f0 100644
> --- a/lib/kunit/string-stream.h
> +++ b/lib/kunit/string-stream.h
> @@ -25,6 +25,7 @@ struct string_stream {
>         spinlock_t lock;
>         struct kunit *test;
>         gfp_t gfp;
> +       bool append_newlines;
>  };
>
>  struct kunit;
> @@ -47,4 +48,10 @@ bool string_stream_is_empty(struct string_stream *stream);
>
>  void string_stream_destroy(struct string_stream *stream);
>
> +static inline void string_stream_set_append_newlines(struct string_stream *stream,
> +                                                    bool append_newlines)
> +{
> +       stream->append_newlines = append_newlines;
> +}
> +
>  #endif /* _KUNIT_STRING_STREAM_H */
> --
> 2.30.2
>

Re: [PATCH v5 03/10] kunit: string-stream: Add option to make all lines end with newline

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 3920 bytes --]

On Thu, 24 Aug 2023 at 22:32, Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> Add an optional feature to string_stream that will append a newline to
> any added string that does not already end with a newline. The purpose
> of this is so that string_stream can be used to collect log lines.
>
> This is enabled/disabled by calling string_stream_set_append_newlines().
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---

This is the same as v4, patch 4, and still looks good.

(In the future, feel free to leave the Reviewed-by: tag from the
previous version, so long as there are no significant changes.)

Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David


>  lib/kunit/string-stream.c | 28 +++++++++++++++++++++-------
>  lib/kunit/string-stream.h |  7 +++++++
>  2 files changed, 28 insertions(+), 7 deletions(-)
>
> diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
> index ed24d86af9f5..1dcf6513b692 100644
> --- a/lib/kunit/string-stream.c
> +++ b/lib/kunit/string-stream.c
> @@ -44,32 +44,46 @@ int string_stream_vadd(struct string_stream *stream,
>                        va_list args)
>  {
>         struct string_stream_fragment *frag_container;
> -       int len;
> +       int buf_len, result_len;
>         va_list args_for_counting;
>
>         /* Make a copy because `vsnprintf` could change it */
>         va_copy(args_for_counting, args);
>
>         /* Evaluate length of formatted string */
> -       len = vsnprintf(NULL, 0, fmt, args_for_counting);
> +       buf_len = vsnprintf(NULL, 0, fmt, args_for_counting);
>
>         va_end(args_for_counting);
>
> -       if (len == 0)
> +       if (buf_len == 0)
>                 return 0;
>
> +       /* Reserve one extra for possible appended newline. */
> +       if (stream->append_newlines)
> +               buf_len++;
> +
>         /* Need space for null byte. */
> -       len++;
> +       buf_len++;
>
>         frag_container = alloc_string_stream_fragment(stream->test,
> -                                                     len,
> +                                                     buf_len,
>                                                       stream->gfp);
>         if (IS_ERR(frag_container))
>                 return PTR_ERR(frag_container);
>
> -       len = vsnprintf(frag_container->fragment, len, fmt, args);
> +       if (stream->append_newlines) {
> +               /* Don't include reserved newline byte in writeable length. */
> +               result_len = vsnprintf(frag_container->fragment, buf_len - 1, fmt, args);
> +
> +               /* Append newline if necessary. */
> +               if (frag_container->fragment[result_len - 1] != '\n')
> +                       result_len = strlcat(frag_container->fragment, "\n", buf_len);
> +       } else {
> +               result_len = vsnprintf(frag_container->fragment, buf_len, fmt, args);
> +       }
> +
>         spin_lock(&stream->lock);
> -       stream->length += len;
> +       stream->length += result_len;
>         list_add_tail(&frag_container->node, &stream->fragments);
>         spin_unlock(&stream->lock);
>
> diff --git a/lib/kunit/string-stream.h b/lib/kunit/string-stream.h
> index b669f9a75a94..048930bf97f0 100644
> --- a/lib/kunit/string-stream.h
> +++ b/lib/kunit/string-stream.h
> @@ -25,6 +25,7 @@ struct string_stream {
>         spinlock_t lock;
>         struct kunit *test;
>         gfp_t gfp;
> +       bool append_newlines;
>  };
>
>  struct kunit;
> @@ -47,4 +48,10 @@ bool string_stream_is_empty(struct string_stream *stream);
>
>  void string_stream_destroy(struct string_stream *stream);
>
> +static inline void string_stream_set_append_newlines(struct string_stream *stream,
> +                                                    bool append_newlines)
> +{
> +       stream->append_newlines = append_newlines;
> +}
> +
>  #endif /* _KUNIT_STRING_STREAM_H */
> --
> 2.30.2
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

[PATCH v5 04/10] kunit: string-stream: Add cases for string_stream newline appending

[Richard Fitzgerald]

Add test cases for testing the string_stream feature that appends a
newline to strings that do not already end with a newline.

string_stream_no_auto_newline_test() tests with this feature disabled.
Newlines should not be added or dropped.

string_stream_auto_newline_test() tests with this feature enabled.
Newlines should be added to lines that do not end with a newline.

string_stream_append_auto_newline_test() tests appending the
content of one stream to another stream when the target stream
has newline appending enabled.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
Changes since V4:
- string_stream_append_auto_newline_test() doesn't clear the destination
  stream_1 between the newline and no-newline case. This is just a
  simplification of the code.
  
- string_stream_no_auto_newline_test() uses the same set of test strings
  as string_stream_auto_newline_test().
---
 lib/kunit/string-stream-test.c | 93 ++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
index 2b761ba01835..2a9936db1b9f 100644
--- a/lib/kunit/string-stream-test.c
+++ b/lib/kunit/string-stream-test.c
@@ -32,6 +32,7 @@ static void string_stream_init_test(struct kunit *test)
 	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);
 
 	KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
 }
@@ -214,6 +215,45 @@ static void string_stream_append_test(struct kunit *test)
 	KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1), stream_2_content);
 }
 
+/* Appending the content of one string stream to one with auto-newlining. */
+static void string_stream_append_auto_newline_test(struct kunit *test)
+{
+	struct string_stream *stream_1, *stream_2;
+
+	/* Stream 1 has newline appending enabled */
+	stream_1 = alloc_string_stream(test, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
+	string_stream_set_append_newlines(stream_1, true);
+	KUNIT_EXPECT_TRUE(test, stream_1->append_newlines);
+
+	/* Stream 2 does not append newlines */
+	stream_2 = alloc_string_stream(test, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_2);
+
+	/* Appending a stream with a newline should not add another newline */
+	string_stream_add(stream_1, "Original string\n");
+	string_stream_add(stream_2, "Appended content\n");
+	string_stream_add(stream_2, "More stuff\n");
+	string_stream_append(stream_1, stream_2);
+	KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1),
+			   "Original string\nAppended content\nMore stuff\n");
+
+	string_stream_destroy(stream_2);
+	stream_2 = alloc_string_stream(test, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_2);
+
+	/*
+	 * Appending a stream without newline should add a final newline.
+	 * The appended string_stream is treated as a single string so newlines
+	 * should not be inserted between fragments.
+	 */
+	string_stream_add(stream_2, "Another");
+	string_stream_add(stream_2, "And again");
+	string_stream_append(stream_1, stream_2);
+	KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1),
+			   "Original string\nAppended content\nMore stuff\nAnotherAnd again\n");
+}
+
 /* Adding an empty string should not create a fragment. */
 static void string_stream_append_empty_string_test(struct kunit *test)
 {
@@ -237,12 +277,65 @@ static void string_stream_append_empty_string_test(struct kunit *test)
 	KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream), "Add this line");
 }
 
+/* Adding strings without automatic newline appending */
+static void string_stream_no_auto_newline_test(struct kunit *test)
+{
+	struct string_stream *stream;
+
+	stream = alloc_string_stream(test, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
+
+	/*
+	 * Add some strings with and without newlines. All formatted newlines
+	 * should be preserved. It should not add any extra newlines.
+	 */
+	string_stream_add(stream, "One");
+	string_stream_add(stream, "Two\n");
+	string_stream_add(stream, "%s\n", "Three");
+	string_stream_add(stream, "%s", "Four\n");
+	string_stream_add(stream, "Five\n%s", "Six");
+	string_stream_add(stream, "Seven\n\n");
+	string_stream_add(stream, "Eight");
+	KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream),
+			   "OneTwo\nThree\nFour\nFive\nSixSeven\n\nEight");
+}
+
+/* Adding strings with automatic newline appending */
+static void string_stream_auto_newline_test(struct kunit *test)
+{
+	struct string_stream *stream;
+
+	stream = alloc_string_stream(test, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
+
+	string_stream_set_append_newlines(stream, true);
+	KUNIT_EXPECT_TRUE(test, stream->append_newlines);
+
+	/*
+	 * Add some strings with and without newlines. Newlines should
+	 * be appended to lines that do not end with \n, but newlines
+	 * resulting from the formatting should not be changed.
+	 */
+	string_stream_add(stream, "One");
+	string_stream_add(stream, "Two\n");
+	string_stream_add(stream, "%s\n", "Three");
+	string_stream_add(stream, "%s", "Four\n");
+	string_stream_add(stream, "Five\n%s", "Six");
+	string_stream_add(stream, "Seven\n\n");
+	string_stream_add(stream, "Eight");
+	KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream),
+			   "One\nTwo\nThree\nFour\nFive\nSix\nSeven\n\nEight\n");
+}
+
 static struct kunit_case string_stream_test_cases[] = {
 	KUNIT_CASE(string_stream_init_test),
 	KUNIT_CASE(string_stream_line_add_test),
 	KUNIT_CASE(string_stream_variable_length_line_test),
 	KUNIT_CASE(string_stream_append_test),
+	KUNIT_CASE(string_stream_append_auto_newline_test),
 	KUNIT_CASE(string_stream_append_empty_string_test),
+	KUNIT_CASE(string_stream_no_auto_newline_test),
+	KUNIT_CASE(string_stream_auto_newline_test),
 	{}
 };
 
-- 
2.30.2

Re: [PATCH v5 04/10] kunit: string-stream: Add cases for string_stream newline appending

[Rae Moar]

On Thu, Aug 24, 2023 at 10:32 AM Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> Add test cases for testing the string_stream feature that appends a
> newline to strings that do not already end with a newline.
>
> string_stream_no_auto_newline_test() tests with this feature disabled.
> Newlines should not be added or dropped.
>
> string_stream_auto_newline_test() tests with this feature enabled.
> Newlines should be added to lines that do not end with a newline.
>
> string_stream_append_auto_newline_test() tests appending the
> content of one stream to another stream when the target stream
> has newline appending enabled.
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>

Hi!

This looks good to me! I like the changes.

Reviewed-by: Rae Moar <rmoar@google.com>

Thanks!
-Rae

> ---
> Changes since V4:
> - string_stream_append_auto_newline_test() doesn't clear the destination
>   stream_1 between the newline and no-newline case. This is just a
>   simplification of the code.
>
> - string_stream_no_auto_newline_test() uses the same set of test strings
>   as string_stream_auto_newline_test().
> ---
>  lib/kunit/string-stream-test.c | 93 ++++++++++++++++++++++++++++++++++
>  1 file changed, 93 insertions(+)
>
> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
> index 2b761ba01835..2a9936db1b9f 100644
> --- a/lib/kunit/string-stream-test.c
> +++ b/lib/kunit/string-stream-test.c
> @@ -32,6 +32,7 @@ static void string_stream_init_test(struct kunit *test)
>         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);
>
>         KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
>  }
> @@ -214,6 +215,45 @@ static void string_stream_append_test(struct kunit *test)
>         KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1), stream_2_content);
>  }
>
> +/* Appending the content of one string stream to one with auto-newlining. */
> +static void string_stream_append_auto_newline_test(struct kunit *test)
> +{
> +       struct string_stream *stream_1, *stream_2;
> +
> +       /* Stream 1 has newline appending enabled */
> +       stream_1 = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
> +       string_stream_set_append_newlines(stream_1, true);
> +       KUNIT_EXPECT_TRUE(test, stream_1->append_newlines);
> +
> +       /* Stream 2 does not append newlines */
> +       stream_2 = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_2);
> +
> +       /* Appending a stream with a newline should not add another newline */
> +       string_stream_add(stream_1, "Original string\n");
> +       string_stream_add(stream_2, "Appended content\n");
> +       string_stream_add(stream_2, "More stuff\n");
> +       string_stream_append(stream_1, stream_2);
> +       KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1),
> +                          "Original string\nAppended content\nMore stuff\n");
> +
> +       string_stream_destroy(stream_2);
> +       stream_2 = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_2);
> +
> +       /*
> +        * Appending a stream without newline should add a final newline.
> +        * The appended string_stream is treated as a single string so newlines
> +        * should not be inserted between fragments.
> +        */
> +       string_stream_add(stream_2, "Another");
> +       string_stream_add(stream_2, "And again");
> +       string_stream_append(stream_1, stream_2);
> +       KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1),
> +                          "Original string\nAppended content\nMore stuff\nAnotherAnd again\n");
> +}
> +
>  /* Adding an empty string should not create a fragment. */
>  static void string_stream_append_empty_string_test(struct kunit *test)
>  {
> @@ -237,12 +277,65 @@ static void string_stream_append_empty_string_test(struct kunit *test)
>         KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream), "Add this line");
>  }
>
> +/* Adding strings without automatic newline appending */
> +static void string_stream_no_auto_newline_test(struct kunit *test)
> +{
> +       struct string_stream *stream;
> +
> +       stream = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
> +
> +       /*
> +        * Add some strings with and without newlines. All formatted newlines
> +        * should be preserved. It should not add any extra newlines.
> +        */
> +       string_stream_add(stream, "One");
> +       string_stream_add(stream, "Two\n");
> +       string_stream_add(stream, "%s\n", "Three");
> +       string_stream_add(stream, "%s", "Four\n");
> +       string_stream_add(stream, "Five\n%s", "Six");
> +       string_stream_add(stream, "Seven\n\n");
> +       string_stream_add(stream, "Eight");
> +       KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream),
> +                          "OneTwo\nThree\nFour\nFive\nSixSeven\n\nEight");
> +}
> +
> +/* Adding strings with automatic newline appending */
> +static void string_stream_auto_newline_test(struct kunit *test)
> +{
> +       struct string_stream *stream;
> +
> +       stream = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
> +
> +       string_stream_set_append_newlines(stream, true);
> +       KUNIT_EXPECT_TRUE(test, stream->append_newlines);
> +
> +       /*
> +        * Add some strings with and without newlines. Newlines should
> +        * be appended to lines that do not end with \n, but newlines
> +        * resulting from the formatting should not be changed.
> +        */
> +       string_stream_add(stream, "One");
> +       string_stream_add(stream, "Two\n");
> +       string_stream_add(stream, "%s\n", "Three");
> +       string_stream_add(stream, "%s", "Four\n");
> +       string_stream_add(stream, "Five\n%s", "Six");
> +       string_stream_add(stream, "Seven\n\n");
> +       string_stream_add(stream, "Eight");
> +       KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream),
> +                          "One\nTwo\nThree\nFour\nFive\nSix\nSeven\n\nEight\n");
> +}
> +
>  static struct kunit_case string_stream_test_cases[] = {
>         KUNIT_CASE(string_stream_init_test),
>         KUNIT_CASE(string_stream_line_add_test),
>         KUNIT_CASE(string_stream_variable_length_line_test),
>         KUNIT_CASE(string_stream_append_test),
> +       KUNIT_CASE(string_stream_append_auto_newline_test),
>         KUNIT_CASE(string_stream_append_empty_string_test),
> +       KUNIT_CASE(string_stream_no_auto_newline_test),
> +       KUNIT_CASE(string_stream_auto_newline_test),
>         {}
>  };
>
> --
> 2.30.2
>

Re: [PATCH v5 04/10] kunit: string-stream: Add cases for string_stream newline appending

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 6926 bytes --]

On Thu, 24 Aug 2023 at 22:32, Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> Add test cases for testing the string_stream feature that appends a
> newline to strings that do not already end with a newline.
>
> string_stream_no_auto_newline_test() tests with this feature disabled.
> Newlines should not be added or dropped.
>
> string_stream_auto_newline_test() tests with this feature enabled.
> Newlines should be added to lines that do not end with a newline.
>
> string_stream_append_auto_newline_test() tests appending the
> content of one stream to another stream when the target stream
> has newline appending enabled.
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
> Changes since V4:
> - string_stream_append_auto_newline_test() doesn't clear the destination
>   stream_1 between the newline and no-newline case. This is just a
>   simplification of the code.
>
> - string_stream_no_auto_newline_test() uses the same set of test strings
>   as string_stream_auto_newline_test().
> ---

Much better, thanks!

Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David


>  lib/kunit/string-stream-test.c | 93 ++++++++++++++++++++++++++++++++++
>  1 file changed, 93 insertions(+)
>
> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
> index 2b761ba01835..2a9936db1b9f 100644
> --- a/lib/kunit/string-stream-test.c
> +++ b/lib/kunit/string-stream-test.c
> @@ -32,6 +32,7 @@ static void string_stream_init_test(struct kunit *test)
>         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);
>
>         KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
>  }
> @@ -214,6 +215,45 @@ static void string_stream_append_test(struct kunit *test)
>         KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1), stream_2_content);
>  }
>
> +/* Appending the content of one string stream to one with auto-newlining. */
> +static void string_stream_append_auto_newline_test(struct kunit *test)
> +{
> +       struct string_stream *stream_1, *stream_2;
> +
> +       /* Stream 1 has newline appending enabled */
> +       stream_1 = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
> +       string_stream_set_append_newlines(stream_1, true);
> +       KUNIT_EXPECT_TRUE(test, stream_1->append_newlines);
> +
> +       /* Stream 2 does not append newlines */
> +       stream_2 = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_2);
> +
> +       /* Appending a stream with a newline should not add another newline */
> +       string_stream_add(stream_1, "Original string\n");
> +       string_stream_add(stream_2, "Appended content\n");
> +       string_stream_add(stream_2, "More stuff\n");
> +       string_stream_append(stream_1, stream_2);
> +       KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1),
> +                          "Original string\nAppended content\nMore stuff\n");
> +
> +       string_stream_destroy(stream_2);
> +       stream_2 = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_2);
> +
> +       /*
> +        * Appending a stream without newline should add a final newline.
> +        * The appended string_stream is treated as a single string so newlines
> +        * should not be inserted between fragments.
> +        */
> +       string_stream_add(stream_2, "Another");
> +       string_stream_add(stream_2, "And again");
> +       string_stream_append(stream_1, stream_2);
> +       KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1),
> +                          "Original string\nAppended content\nMore stuff\nAnotherAnd again\n");
> +}
> +
>  /* Adding an empty string should not create a fragment. */
>  static void string_stream_append_empty_string_test(struct kunit *test)
>  {
> @@ -237,12 +277,65 @@ static void string_stream_append_empty_string_test(struct kunit *test)
>         KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream), "Add this line");
>  }
>
> +/* Adding strings without automatic newline appending */
> +static void string_stream_no_auto_newline_test(struct kunit *test)
> +{
> +       struct string_stream *stream;
> +
> +       stream = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
> +
> +       /*
> +        * Add some strings with and without newlines. All formatted newlines
> +        * should be preserved. It should not add any extra newlines.
> +        */
> +       string_stream_add(stream, "One");
> +       string_stream_add(stream, "Two\n");
> +       string_stream_add(stream, "%s\n", "Three");
> +       string_stream_add(stream, "%s", "Four\n");
> +       string_stream_add(stream, "Five\n%s", "Six");
> +       string_stream_add(stream, "Seven\n\n");
> +       string_stream_add(stream, "Eight");
> +       KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream),
> +                          "OneTwo\nThree\nFour\nFive\nSixSeven\n\nEight");
> +}
> +
> +/* Adding strings with automatic newline appending */
> +static void string_stream_auto_newline_test(struct kunit *test)
> +{
> +       struct string_stream *stream;
> +
> +       stream = alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
> +
> +       string_stream_set_append_newlines(stream, true);
> +       KUNIT_EXPECT_TRUE(test, stream->append_newlines);
> +
> +       /*
> +        * Add some strings with and without newlines. Newlines should
> +        * be appended to lines that do not end with \n, but newlines
> +        * resulting from the formatting should not be changed.
> +        */
> +       string_stream_add(stream, "One");
> +       string_stream_add(stream, "Two\n");
> +       string_stream_add(stream, "%s\n", "Three");
> +       string_stream_add(stream, "%s", "Four\n");
> +       string_stream_add(stream, "Five\n%s", "Six");
> +       string_stream_add(stream, "Seven\n\n");
> +       string_stream_add(stream, "Eight");
> +       KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream),
> +                          "One\nTwo\nThree\nFour\nFive\nSix\nSeven\n\nEight\n");
> +}
> +
>  static struct kunit_case string_stream_test_cases[] = {
>         KUNIT_CASE(string_stream_init_test),
>         KUNIT_CASE(string_stream_line_add_test),
>         KUNIT_CASE(string_stream_variable_length_line_test),
>         KUNIT_CASE(string_stream_append_test),
> +       KUNIT_CASE(string_stream_append_auto_newline_test),
>         KUNIT_CASE(string_stream_append_empty_string_test),
> +       KUNIT_CASE(string_stream_no_auto_newline_test),
> +       KUNIT_CASE(string_stream_auto_newline_test),
>         {}
>  };
>
> --
> 2.30.2
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

[PATCH v5 05/10] kunit: Don't use a managed alloc in is_literal()

[Richard Fitzgerald]

There is no need to use a test-managed alloc in is_literal().
The function frees the temporary buffer before returning.

This removes the only use of the test and gfp members of
struct string_stream outside of the string_stream implementation.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 lib/kunit/assert.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
index 05a09652f5a1..dd1d633d0fe2 100644
--- a/lib/kunit/assert.c
+++ b/lib/kunit/assert.c
@@ -89,8 +89,7 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
 EXPORT_SYMBOL_GPL(kunit_ptr_not_err_assert_format);
 
 /* Checks if `text` is a literal representing `value`, e.g. "5" and 5 */
-static bool is_literal(struct kunit *test, const char *text, long long value,
-		       gfp_t gfp)
+static bool is_literal(const char *text, long long value)
 {
 	char *buffer;
 	int len;
@@ -100,14 +99,15 @@ static bool is_literal(struct kunit *test, const char *text, long long value,
 	if (strlen(text) != len)
 		return false;
 
-	buffer = kunit_kmalloc(test, len+1, gfp);
+	buffer = kmalloc(len+1, GFP_KERNEL);
 	if (!buffer)
 		return false;
 
 	snprintf(buffer, len+1, "%lld", value);
 	ret = strncmp(buffer, text, len) == 0;
 
-	kunit_kfree(test, buffer);
+	kfree(buffer);
+
 	return ret;
 }
 
@@ -125,14 +125,12 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
 			  binary_assert->text->left_text,
 			  binary_assert->text->operation,
 			  binary_assert->text->right_text);
-	if (!is_literal(stream->test, binary_assert->text->left_text,
-			binary_assert->left_value, stream->gfp))
+	if (!is_literal(binary_assert->text->left_text, binary_assert->left_value))
 		string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld (0x%llx)\n",
 				  binary_assert->text->left_text,
 				  binary_assert->left_value,
 				  binary_assert->left_value);
-	if (!is_literal(stream->test, binary_assert->text->right_text,
-			binary_assert->right_value, stream->gfp))
+	if (!is_literal(binary_assert->text->right_text, binary_assert->right_value))
 		string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld (0x%llx)",
 				  binary_assert->text->right_text,
 				  binary_assert->right_value,
-- 
2.30.2

Re: [PATCH v5 05/10] kunit: Don't use a managed alloc in is_literal()

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 3474 bytes --]

On Thu, 24 Aug 2023 at 22:31, Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> There is no need to use a test-managed alloc in is_literal().
> The function frees the temporary buffer before returning.
>
> This removes the only use of the test and gfp members of
> struct string_stream outside of the string_stream implementation.
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---

This makes sense to me, particularly given how independent
string-stream otherwise is from the KUnit resource management bits.

The only possible downside is that the memory won't be cleaned up if
strncmp() crashes due to 'text' being somehow invalid. But given this
is really only even used with static data (generated by the assert
macros), and to fail on the strncmp and not the strlen() would require
some horrible race-condition-y madness, I don't think it's ever
reasonably possible to hit that case.

So, looks good.

Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David


>  lib/kunit/assert.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
> index 05a09652f5a1..dd1d633d0fe2 100644
> --- a/lib/kunit/assert.c
> +++ b/lib/kunit/assert.c
> @@ -89,8 +89,7 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
>  EXPORT_SYMBOL_GPL(kunit_ptr_not_err_assert_format);
>
>  /* Checks if `text` is a literal representing `value`, e.g. "5" and 5 */
> -static bool is_literal(struct kunit *test, const char *text, long long value,
> -                      gfp_t gfp)
> +static bool is_literal(const char *text, long long value)
>  {
>         char *buffer;
>         int len;
> @@ -100,14 +99,15 @@ static bool is_literal(struct kunit *test, const char *text, long long value,
>         if (strlen(text) != len)
>                 return false;
>
> -       buffer = kunit_kmalloc(test, len+1, gfp);
> +       buffer = kmalloc(len+1, GFP_KERNEL);
>         if (!buffer)
>                 return false;
>
>         snprintf(buffer, len+1, "%lld", value);
>         ret = strncmp(buffer, text, len) == 0;
>
> -       kunit_kfree(test, buffer);
> +       kfree(buffer);
> +
>         return ret;
>  }
>
> @@ -125,14 +125,12 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
>                           binary_assert->text->left_text,
>                           binary_assert->text->operation,
>                           binary_assert->text->right_text);
> -       if (!is_literal(stream->test, binary_assert->text->left_text,
> -                       binary_assert->left_value, stream->gfp))
> +       if (!is_literal(binary_assert->text->left_text, binary_assert->left_value))
>                 string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld (0x%llx)\n",
>                                   binary_assert->text->left_text,
>                                   binary_assert->left_value,
>                                   binary_assert->left_value);
> -       if (!is_literal(stream->test, binary_assert->text->right_text,
> -                       binary_assert->right_value, stream->gfp))
> +       if (!is_literal(binary_assert->text->right_text, binary_assert->right_value))
>                 string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld (0x%llx)",
>                                   binary_assert->text->right_text,
>                                   binary_assert->right_value,
> --
> 2.30.2
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

[PATCH v5 06/10] kunit: string-stream: Add kunit_alloc_string_stream()

[Richard Fitzgerald]

Add function kunit_alloc_string_stream() to do a resource-managed
allocation of a string stream, and corresponding
kunit_free_string_stream() to free the resource-managed stream.

This is preparing for decoupling the string_stream
implementation from struct kunit, to reduce the amount of code
churn when that happens. Currently:
 - kunit_alloc_string_stream() only calls alloc_string_stream().
 - kunit_free_string_stream() takes a struct kunit* which
   isn't used yet.

Callers of the old alloc_string_stream() and
string_stream_destroy() are all requesting a managed allocation
so have been changed to use the new functions.

alloc_string_stream() has been temporarily made static because
its current behavior has been replaced with
kunit_alloc_string_stream().

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 lib/kunit/string-stream-test.c | 28 ++++++++++++++--------------
 lib/kunit/string-stream.c      | 12 +++++++++++-
 lib/kunit/string-stream.h      |  3 ++-
 lib/kunit/test.c               |  4 ++--
 4 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
index 2a9936db1b9f..89549c237069 100644
--- a/lib/kunit/string-stream-test.c
+++ b/lib/kunit/string-stream-test.c
@@ -25,7 +25,7 @@ static void string_stream_init_test(struct kunit *test)
 {
 	struct string_stream *stream;
 
-	stream = alloc_string_stream(test, GFP_KERNEL);
+	stream = kunit_alloc_string_stream(test, GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
 
 	KUNIT_EXPECT_EQ(test, stream->length, 0);
@@ -49,7 +49,7 @@ static void string_stream_line_add_test(struct kunit *test)
 	size_t len, total_len;
 	int num_lines, i;
 
-	stream = alloc_string_stream(test, GFP_KERNEL);
+	stream = kunit_alloc_string_stream(test, GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
 
 	/* Add series of sequence numbered lines */
@@ -105,7 +105,7 @@ static void string_stream_variable_length_line_test(struct kunit *test)
 	size_t offset, total_len;
 	int num_lines, i;
 
-	stream = alloc_string_stream(test, GFP_KERNEL);
+	stream = kunit_alloc_string_stream(test, GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
 
 	/*
@@ -165,10 +165,10 @@ static void string_stream_append_test(struct kunit *test)
 	size_t combined_length;
 	int i;
 
-	stream_1 = alloc_string_stream(test, GFP_KERNEL);
+	stream_1 = kunit_alloc_string_stream(test, GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
 
-	stream_2 = alloc_string_stream(test, GFP_KERNEL);
+	stream_2 = kunit_alloc_string_stream(test, GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_2);
 
 	/* Append content of empty stream to empty stream */
@@ -206,9 +206,9 @@ static void string_stream_append_test(struct kunit *test)
 	KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1), combined_content);
 
 	/* Append content of non-empty stream to empty stream */
-	string_stream_destroy(stream_1);
+	kunit_free_string_stream(test, stream_1);
 
-	stream_1 = alloc_string_stream(test, GFP_KERNEL);
+	stream_1 = kunit_alloc_string_stream(test, GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
 
 	string_stream_append(stream_1, stream_2);
@@ -221,13 +221,13 @@ static void string_stream_append_auto_newline_test(struct kunit *test)
 	struct string_stream *stream_1, *stream_2;
 
 	/* Stream 1 has newline appending enabled */
-	stream_1 = alloc_string_stream(test, GFP_KERNEL);
+	stream_1 = kunit_alloc_string_stream(test, GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
 	string_stream_set_append_newlines(stream_1, true);
 	KUNIT_EXPECT_TRUE(test, stream_1->append_newlines);
 
 	/* Stream 2 does not append newlines */
-	stream_2 = alloc_string_stream(test, GFP_KERNEL);
+	stream_2 = kunit_alloc_string_stream(test, GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_2);
 
 	/* Appending a stream with a newline should not add another newline */
@@ -238,8 +238,8 @@ static void string_stream_append_auto_newline_test(struct kunit *test)
 	KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1),
 			   "Original string\nAppended content\nMore stuff\n");
 
-	string_stream_destroy(stream_2);
-	stream_2 = alloc_string_stream(test, GFP_KERNEL);
+	kunit_free_string_stream(test, stream_2);
+	stream_2 = kunit_alloc_string_stream(test, GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_2);
 
 	/*
@@ -260,7 +260,7 @@ static void string_stream_append_empty_string_test(struct kunit *test)
 	struct string_stream *stream;
 	int original_frag_count;
 
-	stream = alloc_string_stream(test, GFP_KERNEL);
+	stream = kunit_alloc_string_stream(test, GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
 
 	/* Formatted empty string */
@@ -282,7 +282,7 @@ static void string_stream_no_auto_newline_test(struct kunit *test)
 {
 	struct string_stream *stream;
 
-	stream = alloc_string_stream(test, GFP_KERNEL);
+	stream = kunit_alloc_string_stream(test, GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
 
 	/*
@@ -305,7 +305,7 @@ static void string_stream_auto_newline_test(struct kunit *test)
 {
 	struct string_stream *stream;
 
-	stream = alloc_string_stream(test, GFP_KERNEL);
+	stream = kunit_alloc_string_stream(test, GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
 
 	string_stream_set_append_newlines(stream, true);
diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
index 1dcf6513b692..12ecf15e1f6b 100644
--- a/lib/kunit/string-stream.c
+++ b/lib/kunit/string-stream.c
@@ -153,7 +153,7 @@ 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)
+static struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp)
 {
 	struct string_stream *stream;
 
@@ -173,3 +173,13 @@ void string_stream_destroy(struct string_stream *stream)
 {
 	string_stream_clear(stream);
 }
+
+struct string_stream *kunit_alloc_string_stream(struct kunit *test, gfp_t gfp)
+{
+	return alloc_string_stream(test, gfp);
+}
+
+void kunit_free_string_stream(struct kunit *test, struct string_stream *stream)
+{
+	string_stream_destroy(stream);
+}
diff --git a/lib/kunit/string-stream.h b/lib/kunit/string-stream.h
index 048930bf97f0..3e70ee9d66e9 100644
--- a/lib/kunit/string-stream.h
+++ b/lib/kunit/string-stream.h
@@ -30,7 +30,8 @@ struct string_stream {
 
 struct kunit;
 
-struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp);
+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);
 
 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 49698a168437..93d9225d61e3 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -308,7 +308,7 @@ static void kunit_fail(struct kunit *test, const struct kunit_loc *loc,
 
 	kunit_set_failure(test);
 
-	stream = alloc_string_stream(test, GFP_KERNEL);
+	stream = kunit_alloc_string_stream(test, GFP_KERNEL);
 	if (IS_ERR(stream)) {
 		WARN(true,
 		     "Could not allocate stream to print failed assertion in %s:%d\n",
@@ -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);
+	kunit_free_string_stream(test, stream);
 }
 
 void __noreturn __kunit_abort(struct kunit *test)
-- 
2.30.2

Re: [PATCH v5 06/10] kunit: string-stream: Add kunit_alloc_string_stream()

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 8591 bytes --]

On Thu, 24 Aug 2023 at 22:32, Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> Add function kunit_alloc_string_stream() to do a resource-managed
> allocation of a string stream, and corresponding
> kunit_free_string_stream() to free the resource-managed stream.
>
> This is preparing for decoupling the string_stream
> implementation from struct kunit, to reduce the amount of code
> churn when that happens. Currently:
>  - kunit_alloc_string_stream() only calls alloc_string_stream().
>  - kunit_free_string_stream() takes a struct kunit* which
>    isn't used yet.
>
> Callers of the old alloc_string_stream() and
> string_stream_destroy() are all requesting a managed allocation
> so have been changed to use the new functions.
>
> alloc_string_stream() has been temporarily made static because
> its current behavior has been replaced with
> kunit_alloc_string_stream().
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---

Looks good.

Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David


>  lib/kunit/string-stream-test.c | 28 ++++++++++++++--------------
>  lib/kunit/string-stream.c      | 12 +++++++++++-
>  lib/kunit/string-stream.h      |  3 ++-
>  lib/kunit/test.c               |  4 ++--
>  4 files changed, 29 insertions(+), 18 deletions(-)
>
> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
> index 2a9936db1b9f..89549c237069 100644
> --- a/lib/kunit/string-stream-test.c
> +++ b/lib/kunit/string-stream-test.c
> @@ -25,7 +25,7 @@ static void string_stream_init_test(struct kunit *test)
>  {
>         struct string_stream *stream;
>
> -       stream = alloc_string_stream(test, GFP_KERNEL);
> +       stream = kunit_alloc_string_stream(test, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
>
>         KUNIT_EXPECT_EQ(test, stream->length, 0);
> @@ -49,7 +49,7 @@ static void string_stream_line_add_test(struct kunit *test)
>         size_t len, total_len;
>         int num_lines, i;
>
> -       stream = alloc_string_stream(test, GFP_KERNEL);
> +       stream = kunit_alloc_string_stream(test, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
>
>         /* Add series of sequence numbered lines */
> @@ -105,7 +105,7 @@ static void string_stream_variable_length_line_test(struct kunit *test)
>         size_t offset, total_len;
>         int num_lines, i;
>
> -       stream = alloc_string_stream(test, GFP_KERNEL);
> +       stream = kunit_alloc_string_stream(test, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
>
>         /*
> @@ -165,10 +165,10 @@ static void string_stream_append_test(struct kunit *test)
>         size_t combined_length;
>         int i;
>
> -       stream_1 = alloc_string_stream(test, GFP_KERNEL);
> +       stream_1 = kunit_alloc_string_stream(test, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
>
> -       stream_2 = alloc_string_stream(test, GFP_KERNEL);
> +       stream_2 = kunit_alloc_string_stream(test, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_2);
>
>         /* Append content of empty stream to empty stream */
> @@ -206,9 +206,9 @@ static void string_stream_append_test(struct kunit *test)
>         KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1), combined_content);
>
>         /* Append content of non-empty stream to empty stream */
> -       string_stream_destroy(stream_1);
> +       kunit_free_string_stream(test, stream_1);
>
> -       stream_1 = alloc_string_stream(test, GFP_KERNEL);
> +       stream_1 = kunit_alloc_string_stream(test, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
>
>         string_stream_append(stream_1, stream_2);
> @@ -221,13 +221,13 @@ static void string_stream_append_auto_newline_test(struct kunit *test)
>         struct string_stream *stream_1, *stream_2;
>
>         /* Stream 1 has newline appending enabled */
> -       stream_1 = alloc_string_stream(test, GFP_KERNEL);
> +       stream_1 = kunit_alloc_string_stream(test, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
>         string_stream_set_append_newlines(stream_1, true);
>         KUNIT_EXPECT_TRUE(test, stream_1->append_newlines);
>
>         /* Stream 2 does not append newlines */
> -       stream_2 = alloc_string_stream(test, GFP_KERNEL);
> +       stream_2 = kunit_alloc_string_stream(test, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_2);
>
>         /* Appending a stream with a newline should not add another newline */
> @@ -238,8 +238,8 @@ static void string_stream_append_auto_newline_test(struct kunit *test)
>         KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1),
>                            "Original string\nAppended content\nMore stuff\n");
>
> -       string_stream_destroy(stream_2);
> -       stream_2 = alloc_string_stream(test, GFP_KERNEL);
> +       kunit_free_string_stream(test, stream_2);
> +       stream_2 = kunit_alloc_string_stream(test, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_2);
>
>         /*
> @@ -260,7 +260,7 @@ static void string_stream_append_empty_string_test(struct kunit *test)
>         struct string_stream *stream;
>         int original_frag_count;
>
> -       stream = alloc_string_stream(test, GFP_KERNEL);
> +       stream = kunit_alloc_string_stream(test, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
>
>         /* Formatted empty string */
> @@ -282,7 +282,7 @@ static void string_stream_no_auto_newline_test(struct kunit *test)
>  {
>         struct string_stream *stream;
>
> -       stream = alloc_string_stream(test, GFP_KERNEL);
> +       stream = kunit_alloc_string_stream(test, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
>
>         /*
> @@ -305,7 +305,7 @@ static void string_stream_auto_newline_test(struct kunit *test)
>  {
>         struct string_stream *stream;
>
> -       stream = alloc_string_stream(test, GFP_KERNEL);
> +       stream = kunit_alloc_string_stream(test, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
>
>         string_stream_set_append_newlines(stream, true);
> diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
> index 1dcf6513b692..12ecf15e1f6b 100644
> --- a/lib/kunit/string-stream.c
> +++ b/lib/kunit/string-stream.c
> @@ -153,7 +153,7 @@ 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)
> +static struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp)
>  {
>         struct string_stream *stream;
>
> @@ -173,3 +173,13 @@ void string_stream_destroy(struct string_stream *stream)
>  {
>         string_stream_clear(stream);
>  }
> +
> +struct string_stream *kunit_alloc_string_stream(struct kunit *test, gfp_t gfp)
> +{
> +       return alloc_string_stream(test, gfp);
> +}
> +
> +void kunit_free_string_stream(struct kunit *test, struct string_stream *stream)
> +{
> +       string_stream_destroy(stream);
> +}
> diff --git a/lib/kunit/string-stream.h b/lib/kunit/string-stream.h
> index 048930bf97f0..3e70ee9d66e9 100644
> --- a/lib/kunit/string-stream.h
> +++ b/lib/kunit/string-stream.h
> @@ -30,7 +30,8 @@ struct string_stream {
>
>  struct kunit;
>
> -struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp);
> +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);
>
>  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 49698a168437..93d9225d61e3 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -308,7 +308,7 @@ static void kunit_fail(struct kunit *test, const struct kunit_loc *loc,
>
>         kunit_set_failure(test);
>
> -       stream = alloc_string_stream(test, GFP_KERNEL);
> +       stream = kunit_alloc_string_stream(test, GFP_KERNEL);
>         if (IS_ERR(stream)) {
>                 WARN(true,
>                      "Could not allocate stream to print failed assertion in %s:%d\n",
> @@ -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);
> +       kunit_free_string_stream(test, stream);
>  }
>
>  void __noreturn __kunit_abort(struct kunit *test)
> --
> 2.30.2
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

[PATCH v5 07/10] kunit: string-stream: Decouple string_stream from kunit

[Richard Fitzgerald]

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.2

Re: [PATCH v5 07/10] kunit: string-stream: Decouple string_stream from kunit

[kernel test robot]

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

Re: [PATCH v5 07/10] kunit: string-stream: Decouple string_stream from kunit

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 9664 bytes --]

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
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

[PATCH v5 08/10] kunit: string-stream: Add tests for freeing resource-managed string_stream

[Richard Fitzgerald]

string_stream_managed_free_test() allocates a resource-managed
string_stream and tests that kunit_free_string_stream() calls
string_stream_destroy().

string_stream_resource_free_test() allocates a resource-managed
string_stream and tests that string_stream_destroy() is called
when the test resources are cleaned up.

The old string_stream_init_test() has been split into two tests,
one for kunit_alloc_string_stream() and the other for
alloc_string_stream().

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
Changes since V4:
- Added test case for kunit_free_string_stream().
- Split the initialization test into separate tests for managed and
  unmanaged allocations.
---
 lib/kunit/string-stream-test.c | 135 ++++++++++++++++++++++++++++++++-
 lib/kunit/string-stream.c      |   3 +
 2 files changed, 134 insertions(+), 4 deletions(-)

diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
index 45a2d221f1b5..6897c57e0db7 100644
--- a/lib/kunit/string-stream-test.c
+++ b/lib/kunit/string-stream-test.c
@@ -6,11 +6,25 @@
  * Author: Brendan Higgins <brendanhiggins@google.com>
  */
 
+#include <kunit/static_stub.h>
 #include <kunit/test.h>
 #include <linux/slab.h>
 
 #include "string-stream.h"
 
+struct string_stream_test_priv {
+	/* For testing resource-managed free. */
+	struct string_stream *freed_stream;
+	bool stream_free_again;
+};
+
+static void cleanup_raw_stream(void *p)
+{
+	struct string_stream *stream = p;
+
+	string_stream_destroy(stream);
+}
+
 static char *get_concatenated_string(struct kunit *test, struct string_stream *stream)
 {
 	char *str = string_stream_get_string(stream);
@@ -21,11 +35,12 @@ static char *get_concatenated_string(struct kunit *test, struct string_stream *s
 	return str;
 }
 
-/* string_stream object is initialized correctly. */
-static void string_stream_init_test(struct kunit *test)
+/* Managed string_stream object is initialized correctly. */
+static void string_stream_managed_init_test(struct kunit *test)
 {
 	struct string_stream *stream;
 
+	/* Resource-managed initialization. */
 	stream = kunit_alloc_string_stream(test, GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
 
@@ -37,6 +52,101 @@ static void string_stream_init_test(struct kunit *test)
 	KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
 }
 
+/* Unmanaged string_stream object is initialized correctly. */
+static void string_stream_unmanaged_init_test(struct kunit *test)
+{
+	struct string_stream *stream;
+
+	stream = alloc_string_stream(GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
+	kunit_add_action(test, cleanup_raw_stream, stream);
+
+	KUNIT_EXPECT_EQ(test, stream->length, 0);
+	KUNIT_EXPECT_TRUE(test, list_empty(&stream->fragments));
+	KUNIT_EXPECT_EQ(test, stream->gfp, GFP_KERNEL);
+	KUNIT_EXPECT_FALSE(test, stream->append_newlines);
+
+	KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
+}
+
+static void string_stream_destroy_stub(struct string_stream *stream)
+{
+	struct kunit *fake_test = kunit_get_current_test();
+	struct string_stream_test_priv *priv = fake_test->priv;
+
+	if (priv->freed_stream)
+		priv->stream_free_again = true;
+
+	priv->freed_stream = stream;
+
+	/*
+	 * Avoid calling deactivate_static_stub() or changing
+	 * current->kunit_test during cleanup. Leave the stream to
+	 * be freed during the test exit.
+	 */
+}
+
+/* kunit_free_string_stream() calls string_stream_desrtoy() */
+static void string_stream_managed_free_test(struct kunit *test)
+{
+	struct string_stream_test_priv *priv = test->priv;
+	struct string_stream *stream;
+
+	priv->freed_stream = NULL;
+	priv->stream_free_again = false;
+	kunit_activate_static_stub(test,
+				   string_stream_destroy,
+				   string_stream_destroy_stub);
+
+	stream = kunit_alloc_string_stream(test, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
+
+	/* This should call the stub function. */
+	kunit_free_string_stream(test, stream);
+
+	KUNIT_EXPECT_PTR_EQ(test, priv->freed_stream, stream);
+	KUNIT_EXPECT_FALSE(test, priv->stream_free_again);
+}
+
+/* string_stream object is freed when test is cleaned up. */
+static void string_stream_resource_free_test(struct kunit *test)
+{
+	struct string_stream_test_priv *priv = test->priv;
+	struct kunit *fake_test;
+	struct string_stream *stream;
+
+	fake_test = kunit_kzalloc(test, sizeof(*fake_test), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fake_test);
+
+	kunit_init_test(fake_test, "string_stream_fake_test", NULL);
+	fake_test->priv = priv;
+
+	/*
+	 * Activate stub before creating string_stream so the
+	 * string_stream will be cleaned up first.
+	 */
+	priv->freed_stream = NULL;
+	priv->stream_free_again = false;
+	kunit_activate_static_stub(fake_test,
+				   string_stream_destroy,
+				   string_stream_destroy_stub);
+
+	stream = kunit_alloc_string_stream(fake_test, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
+
+	/* Set current->kunit_test to fake_test so the static stub will be called. */
+	current->kunit_test = fake_test;
+
+	/* Cleanup test - the stub function should be called */
+	kunit_cleanup(fake_test);
+
+	/* Set current->kunit_test back to current test. */
+	current->kunit_test = test;
+
+	KUNIT_EXPECT_PTR_EQ(test, priv->freed_stream, stream);
+	KUNIT_EXPECT_FALSE(test, priv->stream_free_again);
+}
+
 /*
  * Add a series of lines to a string_stream. Check that all lines
  * appear in the correct order and no characters are dropped.
@@ -327,8 +437,24 @@ static void string_stream_auto_newline_test(struct kunit *test)
 			   "One\nTwo\nThree\nFour\nFive\nSix\nSeven\n\nEight\n");
 }
 
+static int string_stream_test_init(struct kunit *test)
+{
+	struct string_stream_test_priv *priv;
+
+	priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	test->priv = priv;
+
+	return 0;
+}
+
 static struct kunit_case string_stream_test_cases[] = {
-	KUNIT_CASE(string_stream_init_test),
+	KUNIT_CASE(string_stream_managed_init_test),
+	KUNIT_CASE(string_stream_unmanaged_init_test),
+	KUNIT_CASE(string_stream_managed_free_test),
+	KUNIT_CASE(string_stream_resource_free_test),
 	KUNIT_CASE(string_stream_line_add_test),
 	KUNIT_CASE(string_stream_variable_length_line_test),
 	KUNIT_CASE(string_stream_append_test),
@@ -341,6 +467,7 @@ static struct kunit_case string_stream_test_cases[] = {
 
 static struct kunit_suite string_stream_test_suite = {
 	.name = "string-stream-test",
-	.test_cases = string_stream_test_cases
+	.test_cases = string_stream_test_cases,
+	.init = string_stream_test_init,
 };
 kunit_test_suites(&string_stream_test_suite);
diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
index c39f1cba3bcd..d2ded5207e9e 100644
--- a/lib/kunit/string-stream.c
+++ b/lib/kunit/string-stream.c
@@ -6,6 +6,7 @@
  * Author: Brendan Higgins <brendanhiggins@google.com>
  */
 
+#include <kunit/static_stub.h>
 #include <kunit/test.h>
 #include <linux/list.h>
 #include <linux/slab.h>
@@ -170,6 +171,8 @@ struct string_stream *alloc_string_stream(gfp_t gfp)
 
 void string_stream_destroy(struct string_stream *stream)
 {
+	KUNIT_STATIC_STUB_REDIRECT(string_stream_destroy, stream);
+
 	if (!stream)
 		return;
 
-- 
2.30.2

Re: [PATCH v5 08/10] kunit: string-stream: Add tests for freeing resource-managed string_stream

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 9956 bytes --]

On Thu, 24 Aug 2023 at 22:32, 'Richard Fitzgerald' via KUnit
Development <kunit-dev@googlegroups.com> wrote:
>
> string_stream_managed_free_test() allocates a resource-managed
> string_stream and tests that kunit_free_string_stream() calls
> string_stream_destroy().
>
> string_stream_resource_free_test() allocates a resource-managed
> string_stream and tests that string_stream_destroy() is called
> when the test resources are cleaned up.
>
> The old string_stream_init_test() has been split into two tests,
> one for kunit_alloc_string_stream() and the other for
> alloc_string_stream().
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
> Changes since V4:
> - Added test case for kunit_free_string_stream().
> - Split the initialization test into separate tests for managed and
>   unmanaged allocations.
> ---

Looking over this again, I'm not convinced the streams are actually
getting freed. Once the stub has finished, the stream is removed from
the list of deferred actions / resources.

I'll have another look tomorrow in case I missed anything, but if so,
this might need to be explained further / made more obvious.

-- David

>  lib/kunit/string-stream-test.c | 135 ++++++++++++++++++++++++++++++++-
>  lib/kunit/string-stream.c      |   3 +
>  2 files changed, 134 insertions(+), 4 deletions(-)
>
> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
> index 45a2d221f1b5..6897c57e0db7 100644
> --- a/lib/kunit/string-stream-test.c
> +++ b/lib/kunit/string-stream-test.c
> @@ -6,11 +6,25 @@
>   * Author: Brendan Higgins <brendanhiggins@google.com>
>   */
>
> +#include <kunit/static_stub.h>
>  #include <kunit/test.h>
>  #include <linux/slab.h>
>
>  #include "string-stream.h"
>
> +struct string_stream_test_priv {
> +       /* For testing resource-managed free. */
> +       struct string_stream *freed_stream;
> +       bool stream_free_again;
> +};
> +
> +static void cleanup_raw_stream(void *p)
> +{
> +       struct string_stream *stream = p;
> +
> +       string_stream_destroy(stream);
> +}

Is this worth having here? It's only used once, and it could easily be
replaced with a manual call to string_stream_destroy() at the end of
the function.

If it were likely that list_empty() or string_stream_is_empty() would
crash, it would be more worthwhile to make sure the cleanup happens
anyway, but as-is, I think it's safe enough either way.

> +
>  static char *get_concatenated_string(struct kunit *test, struct string_stream *stream)
>  {
>         char *str = string_stream_get_string(stream);
> @@ -21,11 +35,12 @@ static char *get_concatenated_string(struct kunit *test, struct string_stream *s
>         return str;
>  }
>
> -/* string_stream object is initialized correctly. */
> -static void string_stream_init_test(struct kunit *test)
> +/* Managed string_stream object is initialized correctly. */
> +static void string_stream_managed_init_test(struct kunit *test)
>  {
>         struct string_stream *stream;
>
> +       /* Resource-managed initialization. */
>         stream = kunit_alloc_string_stream(test, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
>
> @@ -37,6 +52,101 @@ static void string_stream_init_test(struct kunit *test)
>         KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
>  }
>
> +/* Unmanaged string_stream object is initialized correctly. */
> +static void string_stream_unmanaged_init_test(struct kunit *test)
> +{
> +       struct string_stream *stream;
> +
> +       stream = alloc_string_stream(GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
> +       kunit_add_action(test, cleanup_raw_stream, stream);
> +
> +       KUNIT_EXPECT_EQ(test, stream->length, 0);
> +       KUNIT_EXPECT_TRUE(test, list_empty(&stream->fragments));
> +       KUNIT_EXPECT_EQ(test, stream->gfp, GFP_KERNEL);
> +       KUNIT_EXPECT_FALSE(test, stream->append_newlines);
> +
> +       KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
> +}
> +
> +static void string_stream_destroy_stub(struct string_stream *stream)
> +{
> +       struct kunit *fake_test = kunit_get_current_test();
> +       struct string_stream_test_priv *priv = fake_test->priv;
> +
> +       if (priv->freed_stream)
> +               priv->stream_free_again = true;
> +
> +       priv->freed_stream = stream;
> +
> +       /*
> +        * Avoid calling deactivate_static_stub() or changing
> +        * current->kunit_test during cleanup. Leave the stream to
> +        * be freed during the test exit.
> +        */

Are we ever actually freeing this during test exit? I don't think so.

I think you'd need to either free 'stream' here, or free it (either
manually or with a deferred action) at the end of every test which
uses this stub.

That being said, I'd agree it's best to avoid manually calling
deactivate_static_stub() and/or changin current->kunit_test during
cleanup. While it should actually be safe, as far as I can tell, it'd
be very confusing.

> +}
> +
> +/* kunit_free_string_stream() calls string_stream_desrtoy() */
> +static void string_stream_managed_free_test(struct kunit *test)
> +{
> +       struct string_stream_test_priv *priv = test->priv;
> +       struct string_stream *stream;
> +
> +       priv->freed_stream = NULL;
> +       priv->stream_free_again = false;
> +       kunit_activate_static_stub(test,
> +                                  string_stream_destroy,
> +                                  string_stream_destroy_stub);
> +
> +       stream = kunit_alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
> +
> +       /* This should call the stub function. */
> +       kunit_free_string_stream(test, stream);
> +
> +       KUNIT_EXPECT_PTR_EQ(test, priv->freed_stream, stream);
> +       KUNIT_EXPECT_FALSE(test, priv->stream_free_again);

Stream is never freed here?

> +}
> +
> +/* string_stream object is freed when test is cleaned up. */
> +static void string_stream_resource_free_test(struct kunit *test)
> +{
> +       struct string_stream_test_priv *priv = test->priv;
> +       struct kunit *fake_test;
> +       struct string_stream *stream;
> +
> +       fake_test = kunit_kzalloc(test, sizeof(*fake_test), GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fake_test);
> +
> +       kunit_init_test(fake_test, "string_stream_fake_test", NULL);
> +       fake_test->priv = priv;
> +
> +       /*
> +        * Activate stub before creating string_stream so the
> +        * string_stream will be cleaned up first.
> +        */
> +       priv->freed_stream = NULL;
> +       priv->stream_free_again = false;
> +       kunit_activate_static_stub(fake_test,
> +                                  string_stream_destroy,
> +                                  string_stream_destroy_stub);
> +
> +       stream = kunit_alloc_string_stream(fake_test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
> +
> +       /* Set current->kunit_test to fake_test so the static stub will be called. */
> +       current->kunit_test = fake_test;
> +
> +       /* Cleanup test - the stub function should be called */
> +       kunit_cleanup(fake_test);
> +
> +       /* Set current->kunit_test back to current test. */
> +       current->kunit_test = test;
> +
> +       KUNIT_EXPECT_PTR_EQ(test, priv->freed_stream, stream);
> +       KUNIT_EXPECT_FALSE(test, priv->stream_free_again);

We need to free stream here.


> +}
> +
>  /*
>   * Add a series of lines to a string_stream. Check that all lines
>   * appear in the correct order and no characters are dropped.
> @@ -327,8 +437,24 @@ static void string_stream_auto_newline_test(struct kunit *test)
>                            "One\nTwo\nThree\nFour\nFive\nSix\nSeven\n\nEight\n");
>  }
>
> +static int string_stream_test_init(struct kunit *test)
> +{
> +       struct string_stream_test_priv *priv;
> +
> +       priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       test->priv = priv;
> +
> +       return 0;
> +}
> +
>  static struct kunit_case string_stream_test_cases[] = {
> -       KUNIT_CASE(string_stream_init_test),
> +       KUNIT_CASE(string_stream_managed_init_test),
> +       KUNIT_CASE(string_stream_unmanaged_init_test),
> +       KUNIT_CASE(string_stream_managed_free_test),
> +       KUNIT_CASE(string_stream_resource_free_test),
>         KUNIT_CASE(string_stream_line_add_test),
>         KUNIT_CASE(string_stream_variable_length_line_test),
>         KUNIT_CASE(string_stream_append_test),
> @@ -341,6 +467,7 @@ static struct kunit_case string_stream_test_cases[] = {
>
>  static struct kunit_suite string_stream_test_suite = {
>         .name = "string-stream-test",
> -       .test_cases = string_stream_test_cases
> +       .test_cases = string_stream_test_cases,
> +       .init = string_stream_test_init,
>  };
>  kunit_test_suites(&string_stream_test_suite);
> diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
> index c39f1cba3bcd..d2ded5207e9e 100644
> --- a/lib/kunit/string-stream.c
> +++ b/lib/kunit/string-stream.c
> @@ -6,6 +6,7 @@
>   * Author: Brendan Higgins <brendanhiggins@google.com>
>   */
>
> +#include <kunit/static_stub.h>
>  #include <kunit/test.h>
>  #include <linux/list.h>
>  #include <linux/slab.h>
> @@ -170,6 +171,8 @@ struct string_stream *alloc_string_stream(gfp_t gfp)
>
>  void string_stream_destroy(struct string_stream *stream)
>  {
> +       KUNIT_STATIC_STUB_REDIRECT(string_stream_destroy, stream);
> +
>         if (!stream)
>                 return;
>
> --
> 2.30.2
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230824143129.1957914-9-rf%40opensource.cirrus.com.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH v5 08/10] kunit: string-stream: Add tests for freeing resource-managed string_stream

[Richard Fitzgerald]

On 25/08/2023 07:49, David Gow wrote:
> On Thu, 24 Aug 2023 at 22:32, 'Richard Fitzgerald' via KUnit
> Development <kunit-dev@googlegroups.com> wrote:
>>
>> string_stream_managed_free_test() allocates a resource-managed
>> string_stream and tests that kunit_free_string_stream() calls
>> string_stream_destroy().
>>
>> string_stream_resource_free_test() allocates a resource-managed
>> string_stream and tests that string_stream_destroy() is called
>> when the test resources are cleaned up.
>>
>> The old string_stream_init_test() has been split into two tests,
>> one for kunit_alloc_string_stream() and the other for
>> alloc_string_stream().
>>
>> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
>> ---
>> Changes since V4:
>> - Added test case for kunit_free_string_stream().
>> - Split the initialization test into separate tests for managed and
>>    unmanaged allocations.
>> ---
> 
> Looking over this again, I'm not convinced the streams are actually
> getting freed. Once the stub has finished, the stream is removed from
> the list of deferred actions / resources.
> 

Argh, I think you're right. My original version stashed the stream into
the private data and freed it in a test exit() function so that it was
guaranteed to be freed even if the resource cleanup wasn't called and
the test function aborted before it could do a manual cleanup.
I decided to simplify that but actually that original implementation was
better.

[PATCH v5 09/10] kunit: Use string_stream for test log

[Richard Fitzgerald]

Replace the fixed-size log buffer with a string_stream so that the
log can grow as lines are added.

string_stream_clear() has been made public for the log truncation
done in kunit_init_test().

The existing kunit log tests have been updated for using a
string_stream as the log. No new test have been added because there
are already tests for the underlying string_stream.

As the log tests now depend on string_stream functions they cannot
build when kunit-test is a module. They have been surrounded by
a #if to replace them with skipping version when the test is
build as a module. Though this isn't pretty, it avoids moving
code to another file while that code is also being changed.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
Changes since V4:
- Don't move the log tests to another file. Deal with only including them
  when the test is built-in by wrapping them in a #if. This is to simplify
  code review, because it avoids having a block of code which moves from
  one file to another but at the same time the code has changed.
- Use kunit_add_action() to automatically free the string returned by
  string_stream_get_string().
---
 include/kunit/test.h      | 14 +++++-------
 lib/kunit/debugfs.c       | 36 +++++++++++++++++++-----------
 lib/kunit/kunit-test.c    | 46 ++++++++++++++++++++++++++++++++-------
 lib/kunit/string-stream.c |  2 +-
 lib/kunit/string-stream.h |  2 ++
 lib/kunit/test.c          | 44 +++++--------------------------------
 6 files changed, 75 insertions(+), 69 deletions(-)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index d33114097d0d..b915a0fe93c0 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -32,9 +32,7 @@
 DECLARE_STATIC_KEY_FALSE(kunit_running);
 
 struct kunit;
-
-/* Size of log associated with test. */
-#define KUNIT_LOG_SIZE 2048
+struct string_stream;
 
 /* Maximum size of parameter description string. */
 #define KUNIT_PARAM_DESC_SIZE 128
@@ -132,7 +130,7 @@ struct kunit_case {
 	/* private: internal use only. */
 	enum kunit_status status;
 	char *module_name;
-	char *log;
+	struct string_stream *log;
 };
 
 static inline char *kunit_status_to_ok_not_ok(enum kunit_status status)
@@ -252,7 +250,7 @@ struct kunit_suite {
 	/* private: internal use only */
 	char status_comment[KUNIT_STATUS_COMMENT_SIZE];
 	struct dentry *debugfs;
-	char *log;
+	struct string_stream *log;
 	int suite_init_err;
 };
 
@@ -278,7 +276,7 @@ struct kunit {
 
 	/* private: internal use only. */
 	const char *name; /* Read only after initialization! */
-	char *log; /* Points at case log after initialization */
+	struct string_stream *log; /* Points at case log after initialization */
 	struct kunit_try_catch try_catch;
 	/* param_value is the current parameter value for a test case. */
 	const void *param_value;
@@ -314,7 +312,7 @@ const char *kunit_filter_glob(void);
 char *kunit_filter(void);
 char *kunit_filter_action(void);
 
-void kunit_init_test(struct kunit *test, const char *name, char *log);
+void kunit_init_test(struct kunit *test, const char *name, struct string_stream *log);
 
 int kunit_run_tests(struct kunit_suite *suite);
 
@@ -472,7 +470,7 @@ static inline void *kunit_kcalloc(struct kunit *test, size_t n, size_t size, gfp
 
 void kunit_cleanup(struct kunit *test);
 
-void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...);
+void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt, ...);
 
 /**
  * kunit_mark_skipped() - Marks @test_or_suite as skipped
diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c
index 22c5c496a68f..270d185737e6 100644
--- a/lib/kunit/debugfs.c
+++ b/lib/kunit/debugfs.c
@@ -37,14 +37,21 @@ void kunit_debugfs_init(void)
 		debugfs_rootdir = debugfs_create_dir(KUNIT_DEBUGFS_ROOT, NULL);
 }
 
-static void debugfs_print_result(struct seq_file *seq,
-				 struct kunit_suite *suite,
-				 struct kunit_case *test_case)
+static void debugfs_print_result(struct seq_file *seq, struct string_stream *log)
 {
-	if (!test_case || !test_case->log)
+	struct string_stream_fragment *frag_container;
+
+	if (!log)
 		return;
 
-	seq_printf(seq, "%s", test_case->log);
+	/*
+	 * Walk the fragments so we don't need to allocate a temporary
+	 * buffer to hold the entire string.
+	 */
+	spin_lock(&log->lock);
+	list_for_each_entry(frag_container, &log->fragments, node)
+		seq_printf(seq, "%s", frag_container->fragment);
+	spin_unlock(&log->lock);
 }
 
 /*
@@ -69,10 +76,9 @@ static int debugfs_print_results(struct seq_file *seq, void *v)
 	seq_printf(seq, KUNIT_SUBTEST_INDENT "1..%zd\n", kunit_suite_num_test_cases(suite));
 
 	kunit_suite_for_each_test_case(suite, test_case)
-		debugfs_print_result(seq, suite, test_case);
+		debugfs_print_result(seq, test_case->log);
 
-	if (suite->log)
-		seq_printf(seq, "%s", suite->log);
+	debugfs_print_result(seq, suite->log);
 
 	seq_printf(seq, "%s %d %s\n",
 		   kunit_status_to_ok_not_ok(success), 1, suite->name);
@@ -105,9 +111,13 @@ void kunit_debugfs_create_suite(struct kunit_suite *suite)
 	struct kunit_case *test_case;
 
 	/* Allocate logs before creating debugfs representation. */
-	suite->log = kzalloc(KUNIT_LOG_SIZE, GFP_KERNEL);
-	kunit_suite_for_each_test_case(suite, test_case)
-		test_case->log = kzalloc(KUNIT_LOG_SIZE, GFP_KERNEL);
+	suite->log = alloc_string_stream(GFP_KERNEL);
+	string_stream_set_append_newlines(suite->log, true);
+
+	kunit_suite_for_each_test_case(suite, test_case) {
+		test_case->log = alloc_string_stream(GFP_KERNEL);
+		string_stream_set_append_newlines(test_case->log, true);
+	}
 
 	suite->debugfs = debugfs_create_dir(suite->name, debugfs_rootdir);
 
@@ -121,7 +131,7 @@ void kunit_debugfs_destroy_suite(struct kunit_suite *suite)
 	struct kunit_case *test_case;
 
 	debugfs_remove_recursive(suite->debugfs);
-	kfree(suite->log);
+	string_stream_destroy(suite->log);
 	kunit_suite_for_each_test_case(suite, test_case)
-		kfree(test_case->log);
+		string_stream_destroy(test_case->log);
 }
diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
index 83d8e90ca7a2..616e287aa4bf 100644
--- a/lib/kunit/kunit-test.c
+++ b/lib/kunit/kunit-test.c
@@ -8,6 +8,7 @@
 #include <kunit/test.h>
 #include <kunit/test-bug.h>
 
+#include "string-stream.h"
 #include "try-catch-impl.h"
 
 struct kunit_try_catch_test_context {
@@ -530,12 +531,19 @@ static struct kunit_suite kunit_resource_test_suite = {
 	.test_cases = kunit_resource_test_cases,
 };
 
+/*
+ * Log tests call string_stream functions, which aren't exported. So only
+ * build this code if this test is built-in.
+ */
+#if IS_BUILTIN(CONFIG_KUNIT_TEST)
 static void kunit_log_test(struct kunit *test)
 {
 	struct kunit_suite suite;
+	char *full_log;
 
-	suite.log = kunit_kzalloc(test, KUNIT_LOG_SIZE, GFP_KERNEL);
+	suite.log = kunit_alloc_string_stream(test, GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, suite.log);
+	string_stream_set_append_newlines(suite.log, true);
 
 	kunit_log(KERN_INFO, test, "put this in log.");
 	kunit_log(KERN_INFO, test, "this too.");
@@ -543,14 +551,21 @@ static void kunit_log_test(struct kunit *test)
 	kunit_log(KERN_INFO, &suite, "along with this.");
 
 #ifdef CONFIG_KUNIT_DEBUGFS
+	KUNIT_EXPECT_TRUE(test, test->log->append_newlines);
+
+	full_log = string_stream_get_string(test->log);
+	kunit_add_action(test, (kunit_action_t *)kfree, full_log);
 	KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
-				     strstr(test->log, "put this in log."));
+				     strstr(full_log, "put this in log."));
 	KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
-				     strstr(test->log, "this too."));
+				     strstr(full_log, "this too."));
+
+	full_log = string_stream_get_string(suite.log);
+	kunit_add_action(test, (kunit_action_t *)kfree, full_log);
 	KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
-				     strstr(suite.log, "add to suite log."));
+				     strstr(full_log, "add to suite log."));
 	KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
-				     strstr(suite.log, "along with this."));
+				     strstr(full_log, "along with this."));
 #else
 	KUNIT_EXPECT_NULL(test, test->log);
 #endif
@@ -558,15 +573,30 @@ static void kunit_log_test(struct kunit *test)
 
 static void kunit_log_newline_test(struct kunit *test)
 {
+	char *full_log;
+
 	kunit_info(test, "Add newline\n");
 	if (test->log) {
-		KUNIT_ASSERT_NOT_NULL_MSG(test, strstr(test->log, "Add newline\n"),
-			"Missing log line, full log:\n%s", test->log);
-		KUNIT_EXPECT_NULL(test, strstr(test->log, "Add newline\n\n"));
+		full_log = string_stream_get_string(test->log);
+		kunit_add_action(test, (kunit_action_t *)kfree, full_log);
+		KUNIT_ASSERT_NOT_NULL_MSG(test, strstr(full_log, "Add newline\n"),
+			"Missing log line, full log:\n%s", full_log);
+		KUNIT_EXPECT_NULL(test, strstr(full_log, "Add newline\n\n"));
 	} else {
 		kunit_skip(test, "only useful when debugfs is enabled");
 	}
 }
+#else
+static void kunit_log_test(struct kunit *test)
+{
+	kunit_skip(test, "Log tests only run when built-in");
+}
+
+static void kunit_log_newline_test(struct kunit *test)
+{
+	kunit_skip(test, "Log tests only run when built-in");
+}
+#endif /* IS_BUILTIN(CONFIG_KUNIT_TEST) */
 
 static struct kunit_case kunit_log_test_cases[] = {
 	KUNIT_CASE(kunit_log_test),
diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
index d2ded5207e9e..a6f3616c2048 100644
--- a/lib/kunit/string-stream.c
+++ b/lib/kunit/string-stream.c
@@ -99,7 +99,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;
 
diff --git a/lib/kunit/string-stream.h b/lib/kunit/string-stream.h
index c55925a9b67f..7be2450c7079 100644
--- a/lib/kunit/string-stream.h
+++ b/lib/kunit/string-stream.h
@@ -42,6 +42,8 @@ int __printf(2, 0) string_stream_vadd(struct string_stream *stream,
 				      const char *fmt,
 				      va_list args);
 
+void string_stream_clear(struct string_stream *stream);
+
 char *string_stream_get_string(struct string_stream *stream);
 
 int string_stream_append(struct string_stream *stream,
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 2ad45a4ac06a..b153808ff1ec 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -109,51 +109,17 @@ static void kunit_print_test_stats(struct kunit *test,
 		  stats.total);
 }
 
-/**
- * kunit_log_newline() - Add newline to the end of log if one is not
- * already present.
- * @log: The log to add the newline to.
- */
-static void kunit_log_newline(char *log)
-{
-	int log_len, len_left;
-
-	log_len = strlen(log);
-	len_left = KUNIT_LOG_SIZE - log_len - 1;
-
-	if (log_len > 0 && log[log_len - 1] != '\n')
-		strncat(log, "\n", len_left);
-}
-
-/*
- * Append formatted message to log, size of which is limited to
- * KUNIT_LOG_SIZE bytes (including null terminating byte).
- */
-void kunit_log_append(char *log, const char *fmt, ...)
+/* Append formatted message to log. */
+void kunit_log_append(struct string_stream *log, const char *fmt, ...)
 {
 	va_list args;
-	int len, log_len, len_left;
 
 	if (!log)
 		return;
 
-	log_len = strlen(log);
-	len_left = KUNIT_LOG_SIZE - log_len - 1;
-	if (len_left <= 0)
-		return;
-
-	/* Evaluate length of line to add to log */
 	va_start(args, fmt);
-	len = vsnprintf(NULL, 0, fmt, args) + 1;
+	string_stream_vadd(log, fmt, args);
 	va_end(args);
-
-	/* Print formatted line to the log */
-	va_start(args, fmt);
-	vsnprintf(log + log_len, min(len, len_left), fmt, args);
-	va_end(args);
-
-	/* Add newline to end of log if not already present. */
-	kunit_log_newline(log);
 }
 EXPORT_SYMBOL_GPL(kunit_log_append);
 
@@ -359,14 +325,14 @@ void __kunit_do_failed_assertion(struct kunit *test,
 }
 EXPORT_SYMBOL_GPL(__kunit_do_failed_assertion);
 
-void kunit_init_test(struct kunit *test, const char *name, char *log)
+void kunit_init_test(struct kunit *test, const char *name, struct string_stream *log)
 {
 	spin_lock_init(&test->lock);
 	INIT_LIST_HEAD(&test->resources);
 	test->name = name;
 	test->log = log;
 	if (test->log)
-		test->log[0] = '\0';
+		string_stream_clear(log);
 	test->status = KUNIT_SUCCESS;
 	test->status_comment[0] = '\0';
 }
-- 
2.30.2

Re: [PATCH v5 09/10] kunit: Use string_stream for test log

[kernel test robot]

Hi Richard,

kernel test robot noticed the following build warnings:

[auto build test WARNING on shuah-kselftest/kunit]
[also build test WARNING on next-20230824]
[cannot apply to shuah-kselftest/kunit-fixes linus/master v6.5-rc7]
[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-10-rf%40opensource.cirrus.com
patch subject: [PATCH v5 09/10] kunit: Use string_stream for test log
config: hexagon-randconfig-002-20230825 (https://download.01.org/0day-ci/archive/20230825/202308251443.dMAw1CW3-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/202308251443.dMAw1CW3-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/202308251443.dMAw1CW3-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> lib/kunit/kunit-test.c:542:8: warning: unused variable 'full_log' [-Wunused-variable]
     542 |         char *full_log;
         |               ^
   lib/kunit/kunit-test.c:581:26: warning: cast from 'void (*)(const void *)' to 'kunit_action_t *' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict]
     581 |                 kunit_add_action(test, (kunit_action_t *)kfree, full_log);
         |                                        ^~~~~~~~~~~~~~~~~~~~~~~
   2 warnings generated.


vim +/full_log +542 lib/kunit/kunit-test.c

   533	
   534	/*
   535	 * Log tests call string_stream functions, which aren't exported. So only
   536	 * build this code if this test is built-in.
   537	 */
   538	#if IS_BUILTIN(CONFIG_KUNIT_TEST)
   539	static void kunit_log_test(struct kunit *test)
   540	{
   541		struct kunit_suite suite;
 > 542		char *full_log;
   543	
   544		suite.log = kunit_alloc_string_stream(test, GFP_KERNEL);
   545		KUNIT_ASSERT_NOT_ERR_OR_NULL(test, suite.log);
   546		string_stream_set_append_newlines(suite.log, true);
   547	
   548		kunit_log(KERN_INFO, test, "put this in log.");
   549		kunit_log(KERN_INFO, test, "this too.");
   550		kunit_log(KERN_INFO, &suite, "add to suite log.");
   551		kunit_log(KERN_INFO, &suite, "along with this.");
   552	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v5 09/10] kunit: Use string_stream for test log

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 14657 bytes --]

On Thu, 24 Aug 2023 at 22:33, Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> Replace the fixed-size log buffer with a string_stream so that the
> log can grow as lines are added.
>
> string_stream_clear() has been made public for the log truncation
> done in kunit_init_test().
>
> The existing kunit log tests have been updated for using a
> string_stream as the log. No new test have been added because there
> are already tests for the underlying string_stream.
>
> As the log tests now depend on string_stream functions they cannot
> build when kunit-test is a module. They have been surrounded by
> a #if to replace them with skipping version when the test is
> build as a module. Though this isn't pretty, it avoids moving
> code to another file while that code is also being changed.
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
> Changes since V4:
> - Don't move the log tests to another file. Deal with only including them
>   when the test is built-in by wrapping them in a #if. This is to simplify
>   code review, because it avoids having a block of code which moves from
>   one file to another but at the same time the code has changed.
> - Use kunit_add_action() to automatically free the string returned by
>   string_stream_get_string().
> ---

Looks pretty good to me, and works fine here.

The kunit_add_action() cast does trigger the clang warning (but again,
it's not something which bothers me much personally). But since you've
cleaned it up elsewhere, it may be worth adding a wrapper here, at
least until we have a kunit_free_at_end() function or similar.

Otherwise,
Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David

>  include/kunit/test.h      | 14 +++++-------
>  lib/kunit/debugfs.c       | 36 +++++++++++++++++++-----------
>  lib/kunit/kunit-test.c    | 46 ++++++++++++++++++++++++++++++++-------
>  lib/kunit/string-stream.c |  2 +-
>  lib/kunit/string-stream.h |  2 ++
>  lib/kunit/test.c          | 44 +++++--------------------------------
>  6 files changed, 75 insertions(+), 69 deletions(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index d33114097d0d..b915a0fe93c0 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -32,9 +32,7 @@
>  DECLARE_STATIC_KEY_FALSE(kunit_running);
>
>  struct kunit;
> -
> -/* Size of log associated with test. */
> -#define KUNIT_LOG_SIZE 2048
> +struct string_stream;
>
>  /* Maximum size of parameter description string. */
>  #define KUNIT_PARAM_DESC_SIZE 128
> @@ -132,7 +130,7 @@ struct kunit_case {
>         /* private: internal use only. */
>         enum kunit_status status;
>         char *module_name;
> -       char *log;
> +       struct string_stream *log;
>  };
>
>  static inline char *kunit_status_to_ok_not_ok(enum kunit_status status)
> @@ -252,7 +250,7 @@ struct kunit_suite {
>         /* private: internal use only */
>         char status_comment[KUNIT_STATUS_COMMENT_SIZE];
>         struct dentry *debugfs;
> -       char *log;
> +       struct string_stream *log;
>         int suite_init_err;
>  };
>
> @@ -278,7 +276,7 @@ struct kunit {
>
>         /* private: internal use only. */
>         const char *name; /* Read only after initialization! */
> -       char *log; /* Points at case log after initialization */
> +       struct string_stream *log; /* Points at case log after initialization */
>         struct kunit_try_catch try_catch;
>         /* param_value is the current parameter value for a test case. */
>         const void *param_value;
> @@ -314,7 +312,7 @@ const char *kunit_filter_glob(void);
>  char *kunit_filter(void);
>  char *kunit_filter_action(void);
>
> -void kunit_init_test(struct kunit *test, const char *name, char *log);
> +void kunit_init_test(struct kunit *test, const char *name, struct string_stream *log);
>
>  int kunit_run_tests(struct kunit_suite *suite);
>
> @@ -472,7 +470,7 @@ static inline void *kunit_kcalloc(struct kunit *test, size_t n, size_t size, gfp
>
>  void kunit_cleanup(struct kunit *test);
>
> -void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...);
> +void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt, ...);
>
>  /**
>   * kunit_mark_skipped() - Marks @test_or_suite as skipped
> diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c
> index 22c5c496a68f..270d185737e6 100644
> --- a/lib/kunit/debugfs.c
> +++ b/lib/kunit/debugfs.c
> @@ -37,14 +37,21 @@ void kunit_debugfs_init(void)
>                 debugfs_rootdir = debugfs_create_dir(KUNIT_DEBUGFS_ROOT, NULL);
>  }
>
> -static void debugfs_print_result(struct seq_file *seq,
> -                                struct kunit_suite *suite,
> -                                struct kunit_case *test_case)
> +static void debugfs_print_result(struct seq_file *seq, struct string_stream *log)
>  {
> -       if (!test_case || !test_case->log)
> +       struct string_stream_fragment *frag_container;
> +
> +       if (!log)
>                 return;
>
> -       seq_printf(seq, "%s", test_case->log);
> +       /*
> +        * Walk the fragments so we don't need to allocate a temporary
> +        * buffer to hold the entire string.
> +        */
> +       spin_lock(&log->lock);
> +       list_for_each_entry(frag_container, &log->fragments, node)
> +               seq_printf(seq, "%s", frag_container->fragment);
> +       spin_unlock(&log->lock);
>  }
>
>  /*
> @@ -69,10 +76,9 @@ static int debugfs_print_results(struct seq_file *seq, void *v)
>         seq_printf(seq, KUNIT_SUBTEST_INDENT "1..%zd\n", kunit_suite_num_test_cases(suite));
>
>         kunit_suite_for_each_test_case(suite, test_case)
> -               debugfs_print_result(seq, suite, test_case);
> +               debugfs_print_result(seq, test_case->log);
>
> -       if (suite->log)
> -               seq_printf(seq, "%s", suite->log);
> +       debugfs_print_result(seq, suite->log);
>
>         seq_printf(seq, "%s %d %s\n",
>                    kunit_status_to_ok_not_ok(success), 1, suite->name);
> @@ -105,9 +111,13 @@ void kunit_debugfs_create_suite(struct kunit_suite *suite)
>         struct kunit_case *test_case;
>
>         /* Allocate logs before creating debugfs representation. */
> -       suite->log = kzalloc(KUNIT_LOG_SIZE, GFP_KERNEL);
> -       kunit_suite_for_each_test_case(suite, test_case)
> -               test_case->log = kzalloc(KUNIT_LOG_SIZE, GFP_KERNEL);
> +       suite->log = alloc_string_stream(GFP_KERNEL);
> +       string_stream_set_append_newlines(suite->log, true);
> +
> +       kunit_suite_for_each_test_case(suite, test_case) {
> +               test_case->log = alloc_string_stream(GFP_KERNEL);
> +               string_stream_set_append_newlines(test_case->log, true);
> +       }
>
>         suite->debugfs = debugfs_create_dir(suite->name, debugfs_rootdir);
>
> @@ -121,7 +131,7 @@ void kunit_debugfs_destroy_suite(struct kunit_suite *suite)
>         struct kunit_case *test_case;
>
>         debugfs_remove_recursive(suite->debugfs);
> -       kfree(suite->log);
> +       string_stream_destroy(suite->log);
>         kunit_suite_for_each_test_case(suite, test_case)
> -               kfree(test_case->log);
> +               string_stream_destroy(test_case->log);
>  }
> diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
> index 83d8e90ca7a2..616e287aa4bf 100644
> --- a/lib/kunit/kunit-test.c
> +++ b/lib/kunit/kunit-test.c
> @@ -8,6 +8,7 @@
>  #include <kunit/test.h>
>  #include <kunit/test-bug.h>
>
> +#include "string-stream.h"
>  #include "try-catch-impl.h"
>
>  struct kunit_try_catch_test_context {
> @@ -530,12 +531,19 @@ static struct kunit_suite kunit_resource_test_suite = {
>         .test_cases = kunit_resource_test_cases,
>  };
>
> +/*
> + * Log tests call string_stream functions, which aren't exported. So only
> + * build this code if this test is built-in.
> + */
> +#if IS_BUILTIN(CONFIG_KUNIT_TEST)
>  static void kunit_log_test(struct kunit *test)
>  {
>         struct kunit_suite suite;
> +       char *full_log;
>
> -       suite.log = kunit_kzalloc(test, KUNIT_LOG_SIZE, GFP_KERNEL);
> +       suite.log = kunit_alloc_string_stream(test, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, suite.log);
> +       string_stream_set_append_newlines(suite.log, true);
>
>         kunit_log(KERN_INFO, test, "put this in log.");
>         kunit_log(KERN_INFO, test, "this too.");
> @@ -543,14 +551,21 @@ static void kunit_log_test(struct kunit *test)
>         kunit_log(KERN_INFO, &suite, "along with this.");
>
>  #ifdef CONFIG_KUNIT_DEBUGFS
> +       KUNIT_EXPECT_TRUE(test, test->log->append_newlines);
> +
> +       full_log = string_stream_get_string(test->log);
> +       kunit_add_action(test, (kunit_action_t *)kfree, full_log);
>         KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
> -                                    strstr(test->log, "put this in log."));
> +                                    strstr(full_log, "put this in log."));
>         KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
> -                                    strstr(test->log, "this too."));
> +                                    strstr(full_log, "this too."));
> +
> +       full_log = string_stream_get_string(suite.log);
> +       kunit_add_action(test, (kunit_action_t *)kfree, full_log);
>         KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
> -                                    strstr(suite.log, "add to suite log."));
> +                                    strstr(full_log, "add to suite log."));
>         KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
> -                                    strstr(suite.log, "along with this."));
> +                                    strstr(full_log, "along with this."));
>  #else
>         KUNIT_EXPECT_NULL(test, test->log);
>  #endif
> @@ -558,15 +573,30 @@ static void kunit_log_test(struct kunit *test)
>
>  static void kunit_log_newline_test(struct kunit *test)
>  {
> +       char *full_log;
> +
>         kunit_info(test, "Add newline\n");
>         if (test->log) {
> -               KUNIT_ASSERT_NOT_NULL_MSG(test, strstr(test->log, "Add newline\n"),
> -                       "Missing log line, full log:\n%s", test->log);
> -               KUNIT_EXPECT_NULL(test, strstr(test->log, "Add newline\n\n"));
> +               full_log = string_stream_get_string(test->log);
> +               kunit_add_action(test, (kunit_action_t *)kfree, full_log);
> +               KUNIT_ASSERT_NOT_NULL_MSG(test, strstr(full_log, "Add newline\n"),
> +                       "Missing log line, full log:\n%s", full_log);
> +               KUNIT_EXPECT_NULL(test, strstr(full_log, "Add newline\n\n"));
>         } else {
>                 kunit_skip(test, "only useful when debugfs is enabled");
>         }
>  }
> +#else
> +static void kunit_log_test(struct kunit *test)
> +{
> +       kunit_skip(test, "Log tests only run when built-in");
> +}
> +
> +static void kunit_log_newline_test(struct kunit *test)
> +{
> +       kunit_skip(test, "Log tests only run when built-in");
> +}
> +#endif /* IS_BUILTIN(CONFIG_KUNIT_TEST) */
>
>  static struct kunit_case kunit_log_test_cases[] = {
>         KUNIT_CASE(kunit_log_test),
> diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
> index d2ded5207e9e..a6f3616c2048 100644
> --- a/lib/kunit/string-stream.c
> +++ b/lib/kunit/string-stream.c
> @@ -99,7 +99,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;
>
> diff --git a/lib/kunit/string-stream.h b/lib/kunit/string-stream.h
> index c55925a9b67f..7be2450c7079 100644
> --- a/lib/kunit/string-stream.h
> +++ b/lib/kunit/string-stream.h
> @@ -42,6 +42,8 @@ int __printf(2, 0) string_stream_vadd(struct string_stream *stream,
>                                       const char *fmt,
>                                       va_list args);
>
> +void string_stream_clear(struct string_stream *stream);
> +
>  char *string_stream_get_string(struct string_stream *stream);
>
>  int string_stream_append(struct string_stream *stream,
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 2ad45a4ac06a..b153808ff1ec 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -109,51 +109,17 @@ static void kunit_print_test_stats(struct kunit *test,
>                   stats.total);
>  }
>
> -/**
> - * kunit_log_newline() - Add newline to the end of log if one is not
> - * already present.
> - * @log: The log to add the newline to.
> - */
> -static void kunit_log_newline(char *log)
> -{
> -       int log_len, len_left;
> -
> -       log_len = strlen(log);
> -       len_left = KUNIT_LOG_SIZE - log_len - 1;
> -
> -       if (log_len > 0 && log[log_len - 1] != '\n')
> -               strncat(log, "\n", len_left);
> -}
> -
> -/*
> - * Append formatted message to log, size of which is limited to
> - * KUNIT_LOG_SIZE bytes (including null terminating byte).
> - */
> -void kunit_log_append(char *log, const char *fmt, ...)
> +/* Append formatted message to log. */
> +void kunit_log_append(struct string_stream *log, const char *fmt, ...)
>  {
>         va_list args;
> -       int len, log_len, len_left;
>
>         if (!log)
>                 return;
>
> -       log_len = strlen(log);
> -       len_left = KUNIT_LOG_SIZE - log_len - 1;
> -       if (len_left <= 0)
> -               return;
> -
> -       /* Evaluate length of line to add to log */
>         va_start(args, fmt);
> -       len = vsnprintf(NULL, 0, fmt, args) + 1;
> +       string_stream_vadd(log, fmt, args);
>         va_end(args);
> -
> -       /* Print formatted line to the log */
> -       va_start(args, fmt);
> -       vsnprintf(log + log_len, min(len, len_left), fmt, args);
> -       va_end(args);
> -
> -       /* Add newline to end of log if not already present. */
> -       kunit_log_newline(log);
>  }
>  EXPORT_SYMBOL_GPL(kunit_log_append);
>
> @@ -359,14 +325,14 @@ void __kunit_do_failed_assertion(struct kunit *test,
>  }
>  EXPORT_SYMBOL_GPL(__kunit_do_failed_assertion);
>
> -void kunit_init_test(struct kunit *test, const char *name, char *log)
> +void kunit_init_test(struct kunit *test, const char *name, struct string_stream *log)
>  {
>         spin_lock_init(&test->lock);
>         INIT_LIST_HEAD(&test->resources);
>         test->name = name;
>         test->log = log;
>         if (test->log)
> -               test->log[0] = '\0';
> +               string_stream_clear(log);
>         test->status = KUNIT_SUCCESS;
>         test->status_comment[0] = '\0';
>  }
> --
> 2.30.2
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH v5 09/10] kunit: Use string_stream for test log

[kernel test robot]

Hi Richard,

kernel test robot noticed the following build warnings:

[auto build test WARNING on shuah-kselftest/kunit]
[also build test WARNING on next-20230824]
[cannot apply to shuah-kselftest/kunit-fixes linus/master v6.5-rc7]
[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-10-rf%40opensource.cirrus.com
patch subject: [PATCH v5 09/10] kunit: Use string_stream for test log
config: hexagon-randconfig-001-20230825 (https://download.01.org/0day-ci/archive/20230825/202308251509.VjWK804c-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/202308251509.VjWK804c-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/202308251509.VjWK804c-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> lib/kunit/kunit-test.c:557:25: warning: cast from 'void (*)(const void *)' to 'kunit_action_t *' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict]
     557 |         kunit_add_action(test, (kunit_action_t *)kfree, full_log);
         |                                ^~~~~~~~~~~~~~~~~~~~~~~
   lib/kunit/kunit-test.c:564:25: warning: cast from 'void (*)(const void *)' to 'kunit_action_t *' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict]
     564 |         kunit_add_action(test, (kunit_action_t *)kfree, full_log);
         |                                ^~~~~~~~~~~~~~~~~~~~~~~
   lib/kunit/kunit-test.c:581:26: warning: cast from 'void (*)(const void *)' to 'kunit_action_t *' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict]
     581 |                 kunit_add_action(test, (kunit_action_t *)kfree, full_log);
         |                                        ^~~~~~~~~~~~~~~~~~~~~~~
   3 warnings generated.


vim +557 lib/kunit/kunit-test.c

   533	
   534	/*
   535	 * Log tests call string_stream functions, which aren't exported. So only
   536	 * build this code if this test is built-in.
   537	 */
   538	#if IS_BUILTIN(CONFIG_KUNIT_TEST)
   539	static void kunit_log_test(struct kunit *test)
   540	{
   541		struct kunit_suite suite;
   542		char *full_log;
   543	
   544		suite.log = kunit_alloc_string_stream(test, GFP_KERNEL);
   545		KUNIT_ASSERT_NOT_ERR_OR_NULL(test, suite.log);
   546		string_stream_set_append_newlines(suite.log, true);
   547	
   548		kunit_log(KERN_INFO, test, "put this in log.");
   549		kunit_log(KERN_INFO, test, "this too.");
   550		kunit_log(KERN_INFO, &suite, "add to suite log.");
   551		kunit_log(KERN_INFO, &suite, "along with this.");
   552	
   553	#ifdef CONFIG_KUNIT_DEBUGFS
   554		KUNIT_EXPECT_TRUE(test, test->log->append_newlines);
   555	
   556		full_log = string_stream_get_string(test->log);
 > 557		kunit_add_action(test, (kunit_action_t *)kfree, full_log);
   558		KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
   559					     strstr(full_log, "put this in log."));
   560		KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
   561					     strstr(full_log, "this too."));
   562	
   563		full_log = string_stream_get_string(suite.log);
   564		kunit_add_action(test, (kunit_action_t *)kfree, full_log);
   565		KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
   566					     strstr(full_log, "add to suite log."));
   567		KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
   568					     strstr(full_log, "along with this."));
   569	#else
   570		KUNIT_EXPECT_NULL(test, test->log);
   571	#endif
   572	}
   573	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v5 10/10] kunit: string-stream: Test performance of string_stream

[Richard Fitzgerald]

Add a test of the speed and memory use of string_stream.

string_stream_performance_test() doesn't actually "test" anything (it
cannot fail unless the system has run out of allocatable memory) but it
measures the speed and memory consumption of the string_stream and reports
the result.

This allows changes in the string_stream implementation to be compared.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 lib/kunit/string-stream-test.c | 54 ++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
index 6897c57e0db7..7d81d139b8f8 100644
--- a/lib/kunit/string-stream-test.c
+++ b/lib/kunit/string-stream-test.c
@@ -8,7 +8,9 @@
 
 #include <kunit/static_stub.h>
 #include <kunit/test.h>
+#include <linux/ktime.h>
 #include <linux/slab.h>
+#include <linux/timekeeping.h>
 
 #include "string-stream.h"
 
@@ -437,6 +439,57 @@ static void string_stream_auto_newline_test(struct kunit *test)
 			   "One\nTwo\nThree\nFour\nFive\nSix\nSeven\n\nEight\n");
 }
 
+/*
+ * This doesn't actually "test" anything. It reports time taken
+ * and memory used for logging a large number of lines.
+ */
+static void string_stream_performance_test(struct kunit *test)
+{
+	struct string_stream_fragment *frag_container;
+	struct string_stream *stream;
+	char test_line[101];
+	ktime_t start_time, end_time;
+	size_t len, bytes_requested, actual_bytes_used, total_string_length;
+	int offset, i;
+
+	stream = kunit_alloc_string_stream(test, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
+
+	memset(test_line, 'x', sizeof(test_line) - 1);
+	test_line[sizeof(test_line) - 1] = '\0';
+
+	start_time = ktime_get();
+	for (i = 0; i < 10000; i++) {
+		offset = i % (sizeof(test_line) - 1);
+		string_stream_add(stream, "%s: %d\n", &test_line[offset], i);
+	}
+	end_time = ktime_get();
+
+	/*
+	 * Calculate memory used. This doesn't include invisible
+	 * overhead due to kernel allocator fragment size rounding.
+	 */
+	bytes_requested = sizeof(*stream);
+	actual_bytes_used = ksize(stream);
+	total_string_length = 0;
+
+	list_for_each_entry(frag_container, &stream->fragments, node) {
+		bytes_requested += sizeof(*frag_container);
+		actual_bytes_used += ksize(frag_container);
+
+		len = strlen(frag_container->fragment);
+		total_string_length += len;
+		bytes_requested += len + 1; /* +1 for '\0' */
+		actual_bytes_used += ksize(frag_container->fragment);
+	}
+
+	kunit_info(test, "Time elapsed:           %lld us\n",
+		   ktime_us_delta(end_time, start_time));
+	kunit_info(test, "Total string length:    %zu\n", total_string_length);
+	kunit_info(test, "Bytes requested:        %zu\n", bytes_requested);
+	kunit_info(test, "Actual bytes allocated: %zu\n", actual_bytes_used);
+}
+
 static int string_stream_test_init(struct kunit *test)
 {
 	struct string_stream_test_priv *priv;
@@ -462,6 +515,7 @@ static struct kunit_case string_stream_test_cases[] = {
 	KUNIT_CASE(string_stream_append_empty_string_test),
 	KUNIT_CASE(string_stream_no_auto_newline_test),
 	KUNIT_CASE(string_stream_auto_newline_test),
+	KUNIT_CASE(string_stream_performance_test),
 	{}
 };
 
-- 
2.30.2

Re: [PATCH v5 10/10] kunit: string-stream: Test performance of string_stream

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 4177 bytes --]

On Thu, 24 Aug 2023 at 22:33, Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> Add a test of the speed and memory use of string_stream.
>
> string_stream_performance_test() doesn't actually "test" anything (it
> cannot fail unless the system has run out of allocatable memory) but it
> measures the speed and memory consumption of the string_stream and reports
> the result.
>
> This allows changes in the string_stream implementation to be compared.
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---

Still looks good to me, so still:

Reviewed-by: David Gow <davidgow@google.com>

My results are:
   # string_stream_performance_test: Time elapsed:           4457 us
   # string_stream_performance_test: Total string length:    573890
   # string_stream_performance_test: Bytes requested:        823922
   # string_stream_performance_test: Actual bytes allocated: 1048280

Cheers,
-- David



>  lib/kunit/string-stream-test.c | 54 ++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
>
> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
> index 6897c57e0db7..7d81d139b8f8 100644
> --- a/lib/kunit/string-stream-test.c
> +++ b/lib/kunit/string-stream-test.c
> @@ -8,7 +8,9 @@
>
>  #include <kunit/static_stub.h>
>  #include <kunit/test.h>
> +#include <linux/ktime.h>
>  #include <linux/slab.h>
> +#include <linux/timekeeping.h>
>
>  #include "string-stream.h"
>
> @@ -437,6 +439,57 @@ static void string_stream_auto_newline_test(struct kunit *test)
>                            "One\nTwo\nThree\nFour\nFive\nSix\nSeven\n\nEight\n");
>  }
>
> +/*
> + * This doesn't actually "test" anything. It reports time taken
> + * and memory used for logging a large number of lines.
> + */
> +static void string_stream_performance_test(struct kunit *test)
> +{
> +       struct string_stream_fragment *frag_container;
> +       struct string_stream *stream;
> +       char test_line[101];
> +       ktime_t start_time, end_time;
> +       size_t len, bytes_requested, actual_bytes_used, total_string_length;
> +       int offset, i;
> +
> +       stream = kunit_alloc_string_stream(test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
> +
> +       memset(test_line, 'x', sizeof(test_line) - 1);
> +       test_line[sizeof(test_line) - 1] = '\0';
> +
> +       start_time = ktime_get();
> +       for (i = 0; i < 10000; i++) {
> +               offset = i % (sizeof(test_line) - 1);
> +               string_stream_add(stream, "%s: %d\n", &test_line[offset], i);
> +       }
> +       end_time = ktime_get();
> +
> +       /*
> +        * Calculate memory used. This doesn't include invisible
> +        * overhead due to kernel allocator fragment size rounding.
> +        */
> +       bytes_requested = sizeof(*stream);
> +       actual_bytes_used = ksize(stream);
> +       total_string_length = 0;
> +
> +       list_for_each_entry(frag_container, &stream->fragments, node) {
> +               bytes_requested += sizeof(*frag_container);
> +               actual_bytes_used += ksize(frag_container);
> +
> +               len = strlen(frag_container->fragment);
> +               total_string_length += len;
> +               bytes_requested += len + 1; /* +1 for '\0' */
> +               actual_bytes_used += ksize(frag_container->fragment);
> +       }
> +
> +       kunit_info(test, "Time elapsed:           %lld us\n",
> +                  ktime_us_delta(end_time, start_time));
> +       kunit_info(test, "Total string length:    %zu\n", total_string_length);
> +       kunit_info(test, "Bytes requested:        %zu\n", bytes_requested);
> +       kunit_info(test, "Actual bytes allocated: %zu\n", actual_bytes_used);
> +}
> +
>  static int string_stream_test_init(struct kunit *test)
>  {
>         struct string_stream_test_priv *priv;
> @@ -462,6 +515,7 @@ static struct kunit_case string_stream_test_cases[] = {
>         KUNIT_CASE(string_stream_append_empty_string_test),
>         KUNIT_CASE(string_stream_no_auto_newline_test),
>         KUNIT_CASE(string_stream_auto_newline_test),
> +       KUNIT_CASE(string_stream_performance_test),
>         {}
>  };
>
> --
> 2.30.2
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH v5 00/10] kunit: Add dynamically-extending log

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 3347 bytes --]

On Thu, 24 Aug 2023 at 22:32, 'Richard Fitzgerald' via KUnit
Development <kunit-dev@googlegroups.com> wrote:
>
> This patch chain changes the logging implementation to use string_stream
> so that the log will grow dynamically.
>
> The first 8 patches add test code for string_stream, and make some
> changes to string_stream needed to be able to use it for the log.
>
> The final patch adds a performance report of string_stream.
>
> CHANGES SINCE V4:
> - Re-ordered the first 3 patches from V4 to squash the first two sets
>   of string_stream tests into a single patch.
> - Changed is_literal() so it doesn't need a struct kunit.
> - Split out the new resource-managed alloc and free functions into
>   a pre-patch to reduce the amount of code churn when the string_stream
>   is decoupled from kunit.
> - Wrapped the call to string_stream_geT_string() in string-stream-test
>   in a local function to reduce the amount of code churn when the
>   string_stream is decoupled from kunit.
> - Some minor changes to test implementations.
> - string_stream is now completely separated from kunit and the 'test'
>   member of struct string_stream has been eliminated.
>
> Richard Fitzgerald (10):
>   kunit: string-stream: Don't create a fragment for empty strings
>   kunit: string-stream: Improve testing of string_stream
>   kunit: string-stream: Add option to make all lines end with newline
>   kunit: string-stream: Add cases for string_stream newline appending
>   kunit: Don't use a managed alloc in is_literal()
>   kunit: string-stream: Add kunit_alloc_string_stream()
>   kunit: string-stream: Decouple string_stream from kunit
>   kunit: string-stream: Add tests for freeing resource-managed
>     string_stream
>   kunit: Use string_stream for test log
>   kunit: string-stream: Test performance of string_stream
>

Thanks a lot for sticking with this. I think we're in pretty good
shape now. There are a few minor comments, only one of which really
concerns me. That's the freeing of string streams in the
resource-managed string stream tests. I can't quite see how the actual
stream is freed after being "fake freed" by the stub. Is there
something I'm missing?

Otherwise, this seems good enough to go. I fear we're probably past
the point where it can make it into 6.6 (pull requests are already
being sent out, and I'd really rather have the final version of this
soak in linux-next for a while before sending it to Linus. But we'll
make it the first thing to go into 6.7, I think.

Cheers,
-- David


>  include/kunit/test.h           |  14 +-
>  lib/kunit/assert.c             |  14 +-
>  lib/kunit/debugfs.c            |  36 ++-
>  lib/kunit/kunit-test.c         |  46 ++-
>  lib/kunit/string-stream-test.c | 508 +++++++++++++++++++++++++++++++--
>  lib/kunit/string-stream.c      | 100 +++++--
>  lib/kunit/string-stream.h      |  16 +-
>  lib/kunit/test.c               |  50 +---
>  8 files changed, 662 insertions(+), 122 deletions(-)
>
> --
> 2.30.2
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230824143129.1957914-1-rf%40opensource.cirrus.com.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]