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

[PATCH v4 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 V3:

Completely rewritten to use string_stream instead of implementing a
separate extending-buffer implementation for logging.

I have used the performance test from the final patch on my original
fixed-size-fragment implementation from V3 to get a comparison of the
two implementations (run on i3-8145UE CPU @ 2.20GHz):

                        string_stream     V3 fixed-size-buffer
Time elapsed:           7748 us           3251 us
Total string length:    573890            573890
Bytes requested:        823994            728336
Actual bytes allocated: 1061440           728352

I don't think the difference is enough to be worth complicating the
string_stream implementation with my fixed-fragment implementation from
V3 of this patch chain.

Richard Fitzgerald (10):
  kunit: string-stream: Improve testing of string_stream
  kunit: string-stream: Don't create a fragment for empty strings
  kunit: string-stream: Add cases for adding empty strings to a
    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: string-stream: Pass struct kunit to string_stream_get_string()
  kunit: string-stream: Decouple string_stream from kunit
  kunit: string-stream: Add test 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/Makefile             |   5 +-
 lib/kunit/debugfs.c            |  36 ++-
 lib/kunit/kunit-test.c         |  52 +---
 lib/kunit/log-test.c           |  72 ++++++
 lib/kunit/string-stream-test.c | 447 +++++++++++++++++++++++++++++++--
 lib/kunit/string-stream.c      | 129 +++++++---
 lib/kunit/string-stream.h      |  22 +-
 lib/kunit/test.c               |  48 +---
 9 files changed, 656 insertions(+), 169 deletions(-)
 create mode 100644 lib/kunit/log-test.c

-- 
2.30.2

[PATCH v4 01/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.

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

diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
index 110f3a993250..1d46d5f06d2a 100644
--- a/lib/kunit/string-stream-test.c
+++ b/lib/kunit/string-stream-test.c
@@ -11,38 +11,206 @@
 
 #include "string-stream.h"
 
-static void string_stream_test_empty_on_creation(struct kunit *test)
+/* string_stream object is initialized correctly. */
+static void string_stream_init_test(struct kunit *test)
 {
-	struct string_stream *stream = alloc_string_stream(test, GFP_KERNEL);
+	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 = string_stream_get_string(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 = string_stream_get_string(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[] = {
+		"ONE", "TWO", "THREE", "FOUR", "FIVE", "SIZE",
+		"SEVEN", "EIGHT", "NINE", "TEN",
+	};
+	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(string_stream_get_string(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 = string_stream_get_string(stream_1);
+
+	/* Append content of empty stream to non-empty stream */
+	string_stream_append(stream_1, stream_2);
+	KUNIT_EXPECT_STREQ(test, string_stream_get_string(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 = string_stream_get_string(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, string_stream_get_string(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, string_stream_get_string(stream_1), stream_2_content);
 }
 
 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),
 	{}
 };
 
-- 
2.30.2

[PATCH v4 02/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

[PATCH v4 03/10] kunit: string-stream: Add cases for adding empty strings to a string_stream

[Richard Fitzgerald]

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>
---
 lib/kunit/string-stream-test.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
index 1d46d5f06d2a..efe13e3322b5 100644
--- a/lib/kunit/string-stream-test.c
+++ b/lib/kunit/string-stream-test.c
@@ -206,11 +206,32 @@ static void string_stream_append_test(struct kunit *test)
 	KUNIT_EXPECT_STREQ(test, string_stream_get_string(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;
+
+	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");
+	string_stream_add(stream, "%s", "");
+	KUNIT_EXPECT_EQ(test, list_count_nodes(&stream->fragments), 1);
+	KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream), "Add this line");
+}
+
 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_empty_string_test),
 	{}
 };
 
-- 
2.30.2

[PATCH v4 04/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

[PATCH v4 05/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.

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

diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
index efe13e3322b5..46c2ac162fe8 100644
--- a/lib/kunit/string-stream-test.c
+++ b/lib/kunit/string-stream-test.c
@@ -23,6 +23,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));
 }
@@ -226,12 +227,62 @@ static void string_stream_append_empty_string_test(struct kunit *test)
 	KUNIT_EXPECT_STREQ(test, string_stream_get_string(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. No extra newlines should be
+	 * added.
+	 */
+	string_stream_add(stream, "One");
+	string_stream_add(stream, "Two\n");
+	string_stream_add(stream, "%s\n", "Three");
+	string_stream_add(stream, "Four");
+	KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream),
+			   "OneTwo\nThree\nFour");
+}
+
+/* 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, string_stream_get_string(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_empty_string_test),
+	KUNIT_CASE(string_stream_no_auto_newline_test),
+	KUNIT_CASE(string_stream_auto_newline_test),
 	{}
 };
 
-- 
2.30.2

[PATCH v4 06/10] kunit: string-stream: Pass struct kunit to string_stream_get_string()

[Richard Fitzgerald]

Pass a struct kunit* and gfp_t to string_stream_get_string(). Allocate
the returned buffer using these instead of using the stream->test and
stream->gfp.

This is preparation for removing the dependence of string_stream on
struct kunit, so that string_stream can be used for the debugfs log.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 lib/kunit/string-stream-test.c | 26 +++++++++++++++-----------
 lib/kunit/string-stream.c      |  8 ++++----
 lib/kunit/string-stream.h      |  3 ++-
 lib/kunit/test.c               |  2 +-
 4 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
index 46c2ac162fe8..8a30bb7d5fb7 100644
--- a/lib/kunit/string-stream-test.c
+++ b/lib/kunit/string-stream-test.c
@@ -57,7 +57,7 @@ static void string_stream_line_add_test(struct kunit *test)
 	}
 	num_lines = i;
 
-	concat_string = string_stream_get_string(stream);
+	concat_string = string_stream_get_string(test, stream, GFP_KERNEL);
 	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, concat_string);
 	KUNIT_EXPECT_EQ(test, strlen(concat_string), total_len);
 
@@ -113,7 +113,7 @@ static void string_stream_variable_length_line_test(struct kunit *test)
 	}
 	num_lines = i;
 
-	concat_string = string_stream_get_string(stream);
+	concat_string = string_stream_get_string(test, stream, GFP_KERNEL);
 	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, concat_string);
 	KUNIT_EXPECT_EQ(test, strlen(concat_string), total_len);
 
@@ -165,17 +165,18 @@ static void string_stream_append_test(struct kunit *test)
 
 	/* Append content of empty stream to empty stream */
 	string_stream_append(stream_1, stream_2);
-	KUNIT_EXPECT_EQ(test, strlen(string_stream_get_string(stream_1)), 0);
+	KUNIT_EXPECT_EQ(test, strlen(string_stream_get_string(test, stream_1, GFP_KERNEL)), 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 = string_stream_get_string(stream_1);
+	original_content = string_stream_get_string(test, stream_1, GFP_KERNEL);
 
 	/* Append content of empty stream to non-empty stream */
 	string_stream_append(stream_1, stream_2);
-	KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream_1), original_content);
+	KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream_1, GFP_KERNEL),
+			   original_content);
 
 	/* Add some data to stream_2 */
 	for (i = 0; i < ARRAY_SIZE(strings_2); ++i)
@@ -188,14 +189,15 @@ static void string_stream_append_test(struct kunit *test)
 	 * End result should be the original content of stream_1 plus
 	 * the content of stream_2.
 	 */
-	stream_2_content = string_stream_get_string(stream_2);
+	stream_2_content = string_stream_get_string(test, stream_2, GFP_KERNEL);
 	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, string_stream_get_string(stream_1), combined_content);
+	KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream_1, GFP_KERNEL),
+			   combined_content);
 
 	/* Append content of non-empty stream to empty stream */
 	string_stream_destroy(stream_1);
@@ -204,7 +206,8 @@ static void string_stream_append_test(struct kunit *test)
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
 
 	string_stream_append(stream_1, stream_2);
-	KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream_1), stream_2_content);
+	KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream_1, GFP_KERNEL),
+			   stream_2_content);
 }
 
 /* Adding an empty string should not create a fragment. */
@@ -224,7 +227,8 @@ static void string_stream_append_empty_string_test(struct kunit *test)
 	string_stream_add(stream, "Add this line");
 	string_stream_add(stream, "%s", "");
 	KUNIT_EXPECT_EQ(test, list_count_nodes(&stream->fragments), 1);
-	KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream), "Add this line");
+	KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream, GFP_KERNEL),
+			   "Add this line");
 }
 
 /* Adding strings without automatic newline appending */
@@ -244,7 +248,7 @@ static void string_stream_no_auto_newline_test(struct kunit *test)
 	string_stream_add(stream, "Two\n");
 	string_stream_add(stream, "%s\n", "Three");
 	string_stream_add(stream, "Four");
-	KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream),
+	KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream, GFP_KERNEL),
 			   "OneTwo\nThree\nFour");
 }
 
@@ -271,7 +275,7 @@ static void string_stream_auto_newline_test(struct kunit *test)
 	string_stream_add(stream, "Five\n%s", "Six");
 	string_stream_add(stream, "Seven\n\n");
 	string_stream_add(stream, "Eight");
-	KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream),
+	KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream, GFP_KERNEL),
 			   "One\nTwo\nThree\nFour\nFive\nSix\nSeven\n\nEight\n");
 }
 
diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
index 1dcf6513b692..eb673d3ea3bd 100644
--- a/lib/kunit/string-stream.c
+++ b/lib/kunit/string-stream.c
@@ -117,13 +117,14 @@ static void string_stream_clear(struct string_stream *stream)
 	spin_unlock(&stream->lock);
 }
 
-char *string_stream_get_string(struct string_stream *stream)
+char *string_stream_get_string(struct kunit *test, struct string_stream *stream,
+			       gfp_t gfp)
 {
 	struct string_stream_fragment *frag_container;
 	size_t buf_len = stream->length + 1; /* +1 for null byte. */
 	char *buf;
 
-	buf = kunit_kzalloc(stream->test, buf_len, stream->gfp);
+	buf = kunit_kzalloc(test, buf_len, gfp);
 	if (!buf)
 		return NULL;
 
@@ -140,8 +141,7 @@ int string_stream_append(struct string_stream *stream,
 {
 	const char *other_content;
 
-	other_content = string_stream_get_string(other);
-
+	other_content = string_stream_get_string(other->test, other, other->gfp);
 	if (!other_content)
 		return -ENOMEM;
 
diff --git a/lib/kunit/string-stream.h b/lib/kunit/string-stream.h
index 048930bf97f0..6b4a747881c5 100644
--- a/lib/kunit/string-stream.h
+++ b/lib/kunit/string-stream.h
@@ -39,7 +39,8 @@ int __printf(2, 0) string_stream_vadd(struct string_stream *stream,
 				      const char *fmt,
 				      va_list args);
 
-char *string_stream_get_string(struct string_stream *stream);
+char *string_stream_get_string(struct kunit *test, struct string_stream *stream,
+			       gfp_t gfp);
 
 int string_stream_append(struct string_stream *stream,
 			 struct string_stream *other);
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 49698a168437..520e15f49d0d 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -286,7 +286,7 @@ static void kunit_print_string_stream(struct kunit *test,
 	if (string_stream_is_empty(stream))
 		return;
 
-	buf = string_stream_get_string(stream);
+	buf = string_stream_get_string(test, stream, GFP_KERNEL);
 	if (!buf) {
 		kunit_err(test,
 			  "Could not allocate buffer, dumping stream:\n");
-- 
2.30.2

[PATCH v4 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
object can be resource-managed as a single object:

    alloc_string_stream() API is unchanged and takes a pointer to a
    struct kunit but it now registers the returned string_stream object to
    be resource-managed.

    raw_alloc_string_stream() is a new function that allocates a
    bare string_stream without any association to a struct kunit.

    free_string_stream() is a new function that frees a resource-managed
    string_stream allocated by alloc_string_stream().

    raw_free_string_stream() is a new function that frees a non-managed
    string_stream allocated by raw_alloc_string_stream().

The confusing function string_stream_destroy() has been removed. This only
called string_stream_clear() but didn't free the struct string_stream.
Instead string_stream_clear() has been exported, and the new functions use
the more conventional naming of "free" as the opposite of "alloc".

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

diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
index 8a30bb7d5fb7..437aa4b3179d 100644
--- a/lib/kunit/string-stream-test.c
+++ b/lib/kunit/string-stream-test.c
@@ -200,7 +200,7 @@ static void string_stream_append_test(struct kunit *test)
 			   combined_content);
 
 	/* Append content of non-empty stream to empty stream */
-	string_stream_destroy(stream_1);
+	string_stream_clear(stream_1);
 
 	stream_1 = alloc_string_stream(test, GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
index eb673d3ea3bd..06104a729b45 100644
--- a/lib/kunit/string-stream.c
+++ b/lib/kunit/string-stream.c
@@ -13,30 +13,28 @@
 #include "string-stream.h"
 
 
-static struct string_stream_fragment *alloc_string_stream_fragment(
-		struct kunit *test, int len, gfp_t gfp)
+static struct string_stream_fragment *alloc_string_stream_fragment(int len, gfp_t gfp)
 {
 	struct string_stream_fragment *frag;
 
-	frag = kunit_kzalloc(test, sizeof(*frag), gfp);
+	frag = kzalloc(sizeof(*frag), gfp);
 	if (!frag)
 		return ERR_PTR(-ENOMEM);
 
-	frag->fragment = kunit_kmalloc(test, len, gfp);
+	frag->fragment = kmalloc(len, gfp);
 	if (!frag->fragment) {
-		kunit_kfree(test, frag);
+		kfree(frag);
 		return ERR_PTR(-ENOMEM);
 	}
 
 	return frag;
 }
 
-static void string_stream_fragment_destroy(struct kunit *test,
-					   struct string_stream_fragment *frag)
+static void string_stream_fragment_destroy(struct string_stream_fragment *frag)
 {
 	list_del(&frag->node);
-	kunit_kfree(test, frag->fragment);
-	kunit_kfree(test, frag);
+	kfree(frag->fragment);
+	kfree(frag);
 }
 
 int string_stream_vadd(struct string_stream *stream,
@@ -65,9 +63,7 @@ int string_stream_vadd(struct string_stream *stream,
 	/* Need space for null byte. */
 	buf_len++;
 
-	frag_container = alloc_string_stream_fragment(stream->test,
-						      buf_len,
-						      stream->gfp);
+	frag_container = alloc_string_stream_fragment(buf_len, stream->gfp);
 	if (IS_ERR(frag_container))
 		return PTR_ERR(frag_container);
 
@@ -102,7 +98,7 @@ int string_stream_add(struct string_stream *stream, const char *fmt, ...)
 	return result;
 }
 
-static void string_stream_clear(struct string_stream *stream)
+void string_stream_clear(struct string_stream *stream)
 {
 	struct string_stream_fragment *frag_container, *frag_container_safe;
 
@@ -111,16 +107,28 @@ static void string_stream_clear(struct string_stream *stream)
 				 frag_container_safe,
 				 &stream->fragments,
 				 node) {
-		string_stream_fragment_destroy(stream->test, frag_container);
+		string_stream_fragment_destroy(frag_container);
 	}
 	stream->length = 0;
 	spin_unlock(&stream->lock);
 }
 
+static void _string_stream_concatenate_to_buf(struct string_stream *stream,
+					      char *buf, size_t buf_len)
+{
+	struct string_stream_fragment *frag_container;
+
+	buf[0] = '\0';
+
+	spin_lock(&stream->lock);
+	list_for_each_entry(frag_container, &stream->fragments, node)
+		strlcat(buf, frag_container->fragment, buf_len);
+	spin_unlock(&stream->lock);
+}
+
 char *string_stream_get_string(struct kunit *test, struct string_stream *stream,
 			       gfp_t gfp)
 {
-	struct string_stream_fragment *frag_container;
 	size_t buf_len = stream->length + 1; /* +1 for null byte. */
 	char *buf;
 
@@ -128,10 +136,7 @@ char *string_stream_get_string(struct kunit *test, struct string_stream *stream,
 	if (!buf)
 		return NULL;
 
-	spin_lock(&stream->lock);
-	list_for_each_entry(frag_container, &stream->fragments, node)
-		strlcat(buf, frag_container->fragment, buf_len);
-	spin_unlock(&stream->lock);
+	_string_stream_concatenate_to_buf(stream, buf, buf_len);
 
 	return buf;
 }
@@ -139,13 +144,20 @@ char *string_stream_get_string(struct kunit *test, struct string_stream *stream,
 int string_stream_append(struct string_stream *stream,
 			 struct string_stream *other)
 {
-	const char *other_content;
+	size_t other_buf_len = other->length + 1; /* +1 for null byte. */
+	char *other_buf;
+	int ret;
 
-	other_content = string_stream_get_string(other->test, other, other->gfp);
-	if (!other_content)
+	other_buf = kmalloc(other_buf_len, GFP_KERNEL);
+	if (!other_buf)
 		return -ENOMEM;
 
-	return string_stream_add(stream, other_content);
+	_string_stream_concatenate_to_buf(other, other_buf, other_buf_len);
+
+	ret = string_stream_add(stream, other_buf);
+	kfree(other_buf);
+
+	return ret;
 }
 
 bool string_stream_is_empty(struct string_stream *stream)
@@ -153,23 +165,47 @@ bool string_stream_is_empty(struct string_stream *stream)
 	return list_empty(&stream->fragments);
 }
 
-struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp)
+void raw_free_string_stream(struct string_stream *stream)
+{
+	string_stream_clear(stream);
+	kfree(stream);
+}
+
+struct string_stream *raw_alloc_string_stream(gfp_t gfp)
 {
 	struct string_stream *stream;
 
-	stream = kunit_kzalloc(test, sizeof(*stream), gfp);
+	stream = kzalloc(sizeof(*stream), gfp);
 	if (!stream)
 		return ERR_PTR(-ENOMEM);
 
 	stream->gfp = gfp;
-	stream->test = test;
 	INIT_LIST_HEAD(&stream->fragments);
 	spin_lock_init(&stream->lock);
 
 	return stream;
 }
 
-void string_stream_destroy(struct string_stream *stream)
+struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp)
 {
-	string_stream_clear(stream);
+	struct string_stream *stream;
+
+	stream = raw_alloc_string_stream(gfp);
+	if (IS_ERR(stream))
+		return stream;
+
+	stream->test = test;
+
+	if (kunit_add_action_or_reset(test, (kunit_action_t *)raw_free_string_stream, stream) != 0)
+		return ERR_PTR(-ENOMEM);
+
+	return stream;
+}
+
+void free_string_stream(struct kunit *test, struct string_stream *stream)
+{
+	if (!stream)
+		return;
+
+	kunit_release_action(test, (kunit_action_t *)raw_free_string_stream, (void *)stream);
 }
diff --git a/lib/kunit/string-stream.h b/lib/kunit/string-stream.h
index 6b4a747881c5..fbeda486382a 100644
--- a/lib/kunit/string-stream.h
+++ b/lib/kunit/string-stream.h
@@ -23,14 +23,24 @@ struct string_stream {
 	struct list_head fragments;
 	/* length and fragments are protected by this lock */
 	spinlock_t lock;
+
+	/*
+	 * Pointer to kunit this stream is associated with, or NULL if
+	 * not associated with a kunit.
+	 */
 	struct kunit *test;
+
 	gfp_t gfp;
 	bool append_newlines;
 };
 
 struct kunit;
 
+struct string_stream *raw_alloc_string_stream(gfp_t gfp);
+void raw_free_string_stream(struct string_stream *stream);
+
 struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp);
+void free_string_stream(struct kunit *test, struct string_stream *stream);
 
 int __printf(2, 3) string_stream_add(struct string_stream *stream,
 				     const char *fmt, ...);
@@ -47,7 +57,7 @@ int string_stream_append(struct string_stream *stream,
 
 bool string_stream_is_empty(struct string_stream *stream);
 
-void string_stream_destroy(struct string_stream *stream);
+void string_stream_clear(struct string_stream *stream);
 
 static inline void string_stream_set_append_newlines(struct string_stream *stream,
 						     bool append_newlines)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 520e15f49d0d..4b69d12dfc96 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -322,7 +322,7 @@ static void kunit_fail(struct kunit *test, const struct kunit_loc *loc,
 
 	kunit_print_string_stream(test, stream);
 
-	string_stream_destroy(stream);
+	free_string_stream(test, stream);
 }
 
 void __noreturn __kunit_abort(struct kunit *test)
-- 
2.30.2

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

[Richard Fitzgerald]

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

string_stream_init_test() is extended to test allocating a
string_stream that is not resource-managed.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 lib/kunit/string-stream-test.c | 117 ++++++++++++++++++++++++++++++++-
 lib/kunit/string-stream.c      |   3 +
 2 files changed, 119 insertions(+), 1 deletion(-)

diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
index 437aa4b3179d..05bfade2bd8a 100644
--- a/lib/kunit/string-stream-test.c
+++ b/lib/kunit/string-stream-test.c
@@ -6,16 +6,27 @@
  * 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 {
+	struct string_stream *raw_stream;
+
+	/* For testing resource-managed free */
+	struct string_stream *freed_stream;
+	bool stream_free_again;
+};
+
 /* string_stream object is initialized correctly. */
 static void string_stream_init_test(struct kunit *test)
 {
+	struct string_stream_test_priv *priv = test->priv;
 	struct string_stream *stream;
 
+	/* Resource-managed initialization */
 	stream = alloc_string_stream(test, GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
 
@@ -26,6 +37,86 @@ static void string_stream_init_test(struct kunit *test)
 	KUNIT_EXPECT_FALSE(test, stream->append_newlines);
 
 	KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
+
+	free_string_stream(test, stream);
+
+	/*
+	 * Raw initialization. This allocation is not resource-managed
+	 * so store it in priv->raw_stream to be cleaned up by the
+	 * exit function.
+	 */
+	priv->raw_stream = raw_alloc_string_stream(GFP_KERNEL);
+	stream = priv->raw_stream;
+	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, NULL);
+	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_raw_free_string_stream_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.
+	 */
+}
+
+/* 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,
+				   raw_free_string_stream,
+				   string_stream_raw_free_string_stream_stub);
+
+	stream = alloc_string_stream(fake_test, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
+
+	/*
+	 * Ensure the stream is freed when this test terminates.
+	 */
+	priv->raw_stream = 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);
 }
 
 /*
@@ -279,8 +370,30 @@ 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 void string_stream_test_exit(struct kunit *test)
+{
+	struct string_stream_test_priv *priv = test->priv;
+
+	if (priv && priv->raw_stream)
+		raw_free_string_stream(priv->raw_stream);
+}
+
 static struct kunit_case string_stream_test_cases[] = {
 	KUNIT_CASE(string_stream_init_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),
@@ -292,6 +405,8 @@ 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,
+	.exit = string_stream_test_exit,
 };
 kunit_test_suites(&string_stream_test_suite);
diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
index 06104a729b45..1b55ac1be2e5 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>
@@ -167,6 +168,8 @@ bool string_stream_is_empty(struct string_stream *stream)
 
 void raw_free_string_stream(struct string_stream *stream)
 {
+	KUNIT_STATIC_STUB_REDIRECT(raw_free_string_stream, stream);
+
 	string_stream_clear(stream);
 	kfree(stream);
 }
-- 
2.30.2

[PATCH v4 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.

The existing kunit log tests have been updated for using a
string_stream as the log. As they now depend on string_stream
functions they cannot build when kunit-test is a module. They have
been moved to a separate source file to be built only if kunit-test
is built-in.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 include/kunit/test.h   | 14 ++++----
 lib/kunit/Makefile     |  5 +--
 lib/kunit/debugfs.c    | 36 +++++++++++++--------
 lib/kunit/kunit-test.c | 52 +-----------------------------
 lib/kunit/log-test.c   | 72 ++++++++++++++++++++++++++++++++++++++++++
 lib/kunit/test.c       | 44 +++-----------------------
 6 files changed, 110 insertions(+), 113 deletions(-)
 create mode 100644 lib/kunit/log-test.c

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/Makefile b/lib/kunit/Makefile
index 46f75f23dfe4..65735c2e1d14 100644
--- a/lib/kunit/Makefile
+++ b/lib/kunit/Makefile
@@ -18,9 +18,10 @@ obj-y +=				hooks.o
 
 obj-$(CONFIG_KUNIT_TEST) +=		kunit-test.o
 
-# string-stream-test compiles built-in only.
+# string-stream-test and log-test compiles built-in only.
 ifeq ($(CONFIG_KUNIT_TEST),y)
-obj-$(CONFIG_KUNIT_TEST) +=		string-stream-test.o
+obj-$(CONFIG_KUNIT_TEST) +=		string-stream-test.o \
+					log-test.o
 endif
 
 obj-$(CONFIG_KUNIT_EXAMPLE_TEST) +=	kunit-example-test.o
diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c
index 22c5c496a68f..ab53a7e5c898 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 = raw_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 = raw_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);
+	raw_free_string_stream(suite->log);
 	kunit_suite_for_each_test_case(suite, test_case)
-		kfree(test_case->log);
+		raw_free_string_stream(test_case->log);
 }
diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
index 83d8e90ca7a2..ecc39d5f7411 100644
--- a/lib/kunit/kunit-test.c
+++ b/lib/kunit/kunit-test.c
@@ -530,55 +530,6 @@ static struct kunit_suite kunit_resource_test_suite = {
 	.test_cases = kunit_resource_test_cases,
 };
 
-static void kunit_log_test(struct kunit *test)
-{
-	struct kunit_suite suite;
-
-	suite.log = kunit_kzalloc(test, KUNIT_LOG_SIZE, GFP_KERNEL);
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, suite.log);
-
-	kunit_log(KERN_INFO, test, "put this in log.");
-	kunit_log(KERN_INFO, test, "this too.");
-	kunit_log(KERN_INFO, &suite, "add to suite log.");
-	kunit_log(KERN_INFO, &suite, "along with this.");
-
-#ifdef CONFIG_KUNIT_DEBUGFS
-	KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
-				     strstr(test->log, "put this in log."));
-	KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
-				     strstr(test->log, "this too."));
-	KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
-				     strstr(suite.log, "add to suite log."));
-	KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
-				     strstr(suite.log, "along with this."));
-#else
-	KUNIT_EXPECT_NULL(test, test->log);
-#endif
-}
-
-static void kunit_log_newline_test(struct kunit *test)
-{
-	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"));
-	} else {
-		kunit_skip(test, "only useful when debugfs is enabled");
-	}
-}
-
-static struct kunit_case kunit_log_test_cases[] = {
-	KUNIT_CASE(kunit_log_test),
-	KUNIT_CASE(kunit_log_newline_test),
-	{}
-};
-
-static struct kunit_suite kunit_log_test_suite = {
-	.name = "kunit-log-test",
-	.test_cases = kunit_log_test_cases,
-};
-
 static void kunit_status_set_failure_test(struct kunit *test)
 {
 	struct kunit fake;
@@ -658,7 +609,6 @@ static struct kunit_suite kunit_current_test_suite = {
 };
 
 kunit_test_suites(&kunit_try_catch_test_suite, &kunit_resource_test_suite,
-		  &kunit_log_test_suite, &kunit_status_test_suite,
-		  &kunit_current_test_suite);
+		  &kunit_status_test_suite, &kunit_current_test_suite);
 
 MODULE_LICENSE("GPL v2");
diff --git a/lib/kunit/log-test.c b/lib/kunit/log-test.c
new file mode 100644
index 000000000000..a93d87112fea
--- /dev/null
+++ b/lib/kunit/log-test.c
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit test for logging.
+ *
+ * Based on code:
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+#include <kunit/test.h>
+
+#include "string-stream.h"
+
+static void kunit_log_test(struct kunit *test)
+{
+	struct kunit_suite suite;
+	char *full_log;
+
+	suite.log = 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.");
+	kunit_log(KERN_INFO, &suite, "add to suite log.");
+	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, test->log, GFP_KERNEL);
+	KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
+				     strstr(full_log, "put this in log."));
+	KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
+				     strstr(full_log, "this too."));
+
+	full_log = string_stream_get_string(test, suite.log, GFP_KERNEL);
+	KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
+				     strstr(full_log, "add to suite log."));
+	KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
+				     strstr(full_log, "along with this."));
+#else
+	KUNIT_EXPECT_NULL(test, test->log);
+#endif
+}
+
+static void kunit_log_newline_test(struct kunit *test)
+{
+	char *full_log;
+
+	kunit_info(test, "Add newline\n");
+	if (test->log) {
+		full_log = string_stream_get_string(test, test->log, GFP_KERNEL);
+		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");
+	}
+}
+
+static struct kunit_case kunit_log_test_cases[] = {
+	KUNIT_CASE(kunit_log_test),
+	KUNIT_CASE(kunit_log_newline_test),
+	{}
+};
+
+static struct kunit_suite kunit_log_test_suite = {
+	.name = "kunit-log-test",
+	.test_cases = kunit_log_test_cases,
+};
+
+kunit_test_suites(&kunit_log_test_suite);
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 4b69d12dfc96..14e889e80129 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

[PATCH v4 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 05bfade2bd8a..b55cc14f43fb 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"
 
@@ -370,6 +372,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 = 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;
@@ -400,6 +453,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 v4 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-rc6 next-20230809]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Richard-Fitzgerald/kunit-string-stream-Improve-testing-of-string_stream/20230814-212947
base:   https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git kunit
patch link:    https://lore.kernel.org/r/20230814132309.32641-8-rf%40opensource.cirrus.com
patch subject: [PATCH v4 07/10] kunit: string-stream: Decouple string_stream from kunit
config: hexagon-randconfig-r014-20230815 (https://download.01.org/0day-ci/archive/20230815/202308150347.LvFXkSdz-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce: (https://download.01.org/0day-ci/archive/20230815/202308150347.LvFXkSdz-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308150347.LvFXkSdz-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> lib/kunit/string-stream.c:199:38: warning: cast from 'void (*)(struct string_stream *)' to 'kunit_action_t *' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict]
     199 |         if (kunit_add_action_or_reset(test, (kunit_action_t *)raw_free_string_stream, stream) != 0)
         |                                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:55:47: note: expanded from macro 'if'
      55 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
         |                                               ^~~~
   include/linux/compiler.h:57:52: note: expanded from macro '__trace_if_var'
      57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                    ^~~~
>> lib/kunit/string-stream.c:199:38: warning: cast from 'void (*)(struct string_stream *)' to 'kunit_action_t *' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict]
     199 |         if (kunit_add_action_or_reset(test, (kunit_action_t *)raw_free_string_stream, stream) != 0)
         |                                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:55:47: note: expanded from macro 'if'
      55 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
         |                                               ^~~~
   include/linux/compiler.h:57:61: note: expanded from macro '__trace_if_var'
      57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                             ^~~~
>> lib/kunit/string-stream.c:199:38: warning: cast from 'void (*)(struct string_stream *)' to 'kunit_action_t *' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict]
     199 |         if (kunit_add_action_or_reset(test, (kunit_action_t *)raw_free_string_stream, stream) != 0)
         |                                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/compiler.h:55:47: note: expanded from macro 'if'
      55 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
         |                                               ^~~~
   include/linux/compiler.h:57:86: note: expanded from macro '__trace_if_var'
      57 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                                                      ^~~~
   include/linux/compiler.h:68:3: note: expanded from macro '__trace_if_value'
      68 |         (cond) ?                                        \
         |          ^~~~
   lib/kunit/string-stream.c:210:29: warning: cast from 'void (*)(struct string_stream *)' to 'kunit_action_t *' (aka 'void (*)(void *)') converts to incompatible function type [-Wcast-function-type-strict]
     210 |         kunit_release_action(test, (kunit_action_t *)raw_free_string_stream, (void *)stream);
         |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   4 warnings generated.


vim +199 lib/kunit/string-stream.c

   188	
   189	struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp)
   190	{
   191		struct string_stream *stream;
   192	
   193		stream = raw_alloc_string_stream(gfp);
   194		if (IS_ERR(stream))
   195			return stream;
   196	
   197		stream->test = test;
   198	
 > 199		if (kunit_add_action_or_reset(test, (kunit_action_t *)raw_free_string_stream, stream) != 0)
   200			return ERR_PTR(-ENOMEM);
   201	
   202		return stream;
   203	}
   204	

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

Re: [PATCH v4 01/10] kunit: string-stream: Improve testing of string_stream

[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-rc6 next-20230809]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Richard-Fitzgerald/kunit-string-stream-Improve-testing-of-string_stream/20230814-212947
base:   https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git kunit
patch link:    https://lore.kernel.org/r/20230814132309.32641-2-rf%40opensource.cirrus.com
patch subject: [PATCH v4 01/10] kunit: string-stream: Improve testing of string_stream
config: arc-randconfig-r073-20230815 (https://download.01.org/0day-ci/archive/20230815/202308151555.o0Ok5tyv-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230815/202308151555.o0Ok5tyv-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/202308151555.o0Ok5tyv-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> lib/kunit/string-stream-test.c:25:9: sparse: sparse: incorrect type in initializer (different base types) @@     expected long long left_value @@     got restricted gfp_t const __left @@
   lib/kunit/string-stream-test.c:25:9: sparse:     expected long long left_value
   lib/kunit/string-stream-test.c:25:9: sparse:     got restricted gfp_t const __left
>> lib/kunit/string-stream-test.c:25:9: sparse: sparse: incorrect type in initializer (different base types) @@     expected long long right_value @@     got restricted gfp_t const __right @@
   lib/kunit/string-stream-test.c:25:9: sparse:     expected long long right_value
   lib/kunit/string-stream-test.c:25:9: sparse:     got restricted gfp_t const __right

vim +25 lib/kunit/string-stream-test.c

    13	
    14	/* string_stream object is initialized correctly. */
    15	static void string_stream_init_test(struct kunit *test)
    16	{
    17		struct string_stream *stream;
    18	
    19		stream = alloc_string_stream(test, GFP_KERNEL);
    20		KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
    21	
    22		KUNIT_EXPECT_EQ(test, stream->length, 0);
    23		KUNIT_EXPECT_TRUE(test, list_empty(&stream->fragments));
    24		KUNIT_EXPECT_PTR_EQ(test, stream->test, test);
  > 25		KUNIT_EXPECT_EQ(test, stream->gfp, GFP_KERNEL);
    26	
    27		KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
    28	}
    29	

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

Re: [PATCH v4 01/10] kunit: string-stream: Improve testing of string_stream

[David Gow]

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

On Mon, 14 Aug 2023 at 21:23, 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.
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---

Thanks. These tests are much better than the original string stream ones.

It looks good to me. I left a few notes below (mostly to myself), but
nothing that would require a new version by itself.

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

Cheers,
-- David

>  lib/kunit/string-stream-test.c | 200 ++++++++++++++++++++++++++++++---
>  1 file changed, 184 insertions(+), 16 deletions(-)
>
> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
> index 110f3a993250..1d46d5f06d2a 100644
> --- a/lib/kunit/string-stream-test.c
> +++ b/lib/kunit/string-stream-test.c
> @@ -11,38 +11,206 @@
>
>  #include "string-stream.h"
>
> -static void string_stream_test_empty_on_creation(struct kunit *test)
> +/* string_stream object is initialized correctly. */
> +static void string_stream_init_test(struct kunit *test)
>  {
> -       struct string_stream *stream = alloc_string_stream(test, GFP_KERNEL);
> +       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 the kernel test robot notes, this may trigger a sparse warning, as
KUnit stores integer types as 'long long' internally in assertions.
Ignore it for now, we'll see if we can fix it in KUnit.

>
>         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);

Assuming the 'd' in '%d' counts, this still has every letter of the
alphabet in it. :-)

> +
> +               /* 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 = string_stream_get_string(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);

This being deterministic is good. There are a few tests which are
doing similar things with randomness, and I think we'll want to have a
shared framework for it at some point (to enable a 'fuzzing' mode
which is _not_ deterministic), but this is good for now.

> +       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 = string_stream_get_string(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[] = {
> +               "ONE", "TWO", "THREE", "FOUR", "FIVE", "SIZE",
> +               "SEVEN", "EIGHT", "NINE", "TEN",
> +       };

This is fine, but I wonder if it'd be more resilient to potential
changes to the string-string implementation if these arrays didn't
have the same shape (in terms of length and length of substrings).

e.g., if we made the 2nd one "NINE", "EIGHT", "SEVEN"..., so it
doesn't have the same number of strings (due to missing "TEN") and the
lengths of them are not equivalent (strlen("one") != strlen("NINE")).

I doubt it'd make much difference, but maybe it'd catch some nasty
use-after-free or similar errors, and having the strings be different
makes it more obvious that there's not something being tested which
relies on them being the same.

(Don't bother resending the series just for this, though. But if we
have to do a new version anyway...)



> +       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(string_stream_get_string(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 = string_stream_get_string(stream_1);
> +
> +       /* Append content of empty stream to non-empty stream */
> +       string_stream_append(stream_1, stream_2);
> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(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 = string_stream_get_string(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, string_stream_get_string(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, string_stream_get_string(stream_1), stream_2_content);
>  }
>
>  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),
>         {}
>  };
>
> --
> 2.30.2
>

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

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

[David Gow]

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

On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald
<rf@opensource.cirrus.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>
> ---

Nice catch!

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
>

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

Re: [PATCH v4 03/10] kunit: string-stream: Add cases for adding empty strings to a string_stream

[David Gow]

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

On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> 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>
> ---

Looks good. One minor note below (not worth resending the series to
fix by itself, though).

If you wanted to combine this with the previous patch, that'd be fine
too. (I don't mind either way.)

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

Cheers,
-- David

>  lib/kunit/string-stream-test.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
> index 1d46d5f06d2a..efe13e3322b5 100644
> --- a/lib/kunit/string-stream-test.c
> +++ b/lib/kunit/string-stream-test.c
> @@ -206,11 +206,32 @@ static void string_stream_append_test(struct kunit *test)
>         KUNIT_EXPECT_STREQ(test, string_stream_get_string(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;
> +
> +       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");
> +       string_stream_add(stream, "%s", "");
> +       KUNIT_EXPECT_EQ(test, list_count_nodes(&stream->fragments), 1);

While this is fine, I do wonder whether the more future-proof thing to
do would be to check that the number of fragments hasn't changed after
adding the empty string, rather than that it's definitely 1.

(In practice, even with a fancier allocation strategy, I can't imagine
us splitting "Add this line" into multiple fragments, but I think it's
slightly clearer that what we're testing is that the empty string
doesn't increase it.)


> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream), "Add this line");
> +}
> +
>  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_empty_string_test),
>         {}
>  };
>
> --
> 2.30.2
>

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

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

[David Gow]

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

On Mon, 14 Aug 2023 at 21:23, 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>
> ---

Looks good. I don't mind the extra 'wasted' byte if a message already
ends in a newline.

Reviewed-by: David Gow <davidgow@google.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
>

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

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

[David Gow]

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

On Mon, 14 Aug 2023 at 21:23, 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.
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---

Looks good to me. I wouldn't mind if you added the extra test strings
to the non-newline case, but can do without if you feel it's
excessive.

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


-- David

>  lib/kunit/string-stream-test.c | 51 ++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>
> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
> index efe13e3322b5..46c2ac162fe8 100644
> --- a/lib/kunit/string-stream-test.c
> +++ b/lib/kunit/string-stream-test.c
> @@ -23,6 +23,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));
>  }
> @@ -226,12 +227,62 @@ static void string_stream_append_empty_string_test(struct kunit *test)
>         KUNIT_EXPECT_STREQ(test, string_stream_get_string(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. No extra newlines should be
> +        * added.
> +        */
> +       string_stream_add(stream, "One");
> +       string_stream_add(stream, "Two\n");
> +       string_stream_add(stream, "%s\n", "Three");
> +       string_stream_add(stream, "Four");
> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream),
> +                          "OneTwo\nThree\nFour");
> +}
> +
> +/* 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, string_stream_get_string(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_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 --]

Re: [PATCH v4 06/10] kunit: string-stream: Pass struct kunit to string_stream_get_string()

[David Gow]

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

On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> Pass a struct kunit* and gfp_t to string_stream_get_string(). Allocate
> the returned buffer using these instead of using the stream->test and
> stream->gfp.
>
> This is preparation for removing the dependence of string_stream on
> struct kunit, so that string_stream can be used for the debugfs log.
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---

Makes sense to me.

I think that, if we weren't going to remove the struct kunit
dependency, we'd want to either keep a version of
string_stream_get_string() which uses it, or, e.g., fall back to it if
the struct passed in is NULL.

The other option is to have a version which doesn't manage the string
at all, so just takes a gfp and requires the caller to free it (or
register an action to do so). If we did that, we could get rid of the
struct kunit pointer totally (though it may be better to keep it and
have both versions).

So -- to switch to some stream-of-consciousness thoughts about this --
basically there are three possible variants of
string_stream_get_string():
- Current version: uses the stream's struct kunit. Useless if this is
NULL, but very convenient otherwise.
- This patch: manage the string using the struct kunit passed as an
argument. Still can't be used directly outside a test, but adds enough
flexibility to get _append to work.
- Totally unmanaged: the generated string is allocated separately, and
must be freed (directly or in a deferred action) by the caller. Works
well outside tests, but less convenient.

Personally, I feel that the design of string_stream is heading towards
the third option. By the end of this series, everything uses
_string_stream_concatenate_to_buf() anyway. There's only one call to
string_stream_get_string() outside of the logging / string_stream
tests, and all that does is log the results (and it has a fallback to
log each fragment separately if the allocation fails).

Then again, if this is only really being used in tests, then we can
probably just stick with string_stream_get_string() as-is, remove the
string_stream->test member totally, and if we need it, we can make
_string_stream_concatenate_to_buf() public, or add an unmanaged
version anyway.

So, after all that, I think this is probably good as-is. _Maybe_ we
could rename string_stream_get_string() to something like
string_stream_get_managed_string(), now that it's the only function
which is "managed" as a KUnit resource, but we can equally put that
off until we need to add an unmanaged version.

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

Cheers,
-- David


>  lib/kunit/string-stream-test.c | 26 +++++++++++++++-----------
>  lib/kunit/string-stream.c      |  8 ++++----
>  lib/kunit/string-stream.h      |  3 ++-
>  lib/kunit/test.c               |  2 +-
>  4 files changed, 22 insertions(+), 17 deletions(-)
>
> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
> index 46c2ac162fe8..8a30bb7d5fb7 100644
> --- a/lib/kunit/string-stream-test.c
> +++ b/lib/kunit/string-stream-test.c
> @@ -57,7 +57,7 @@ static void string_stream_line_add_test(struct kunit *test)
>         }
>         num_lines = i;
>
> -       concat_string = string_stream_get_string(stream);
> +       concat_string = string_stream_get_string(test, stream, GFP_KERNEL);
>         KUNIT_EXPECT_NOT_ERR_OR_NULL(test, concat_string);
>         KUNIT_EXPECT_EQ(test, strlen(concat_string), total_len);
>
> @@ -113,7 +113,7 @@ static void string_stream_variable_length_line_test(struct kunit *test)
>         }
>         num_lines = i;
>
> -       concat_string = string_stream_get_string(stream);
> +       concat_string = string_stream_get_string(test, stream, GFP_KERNEL);
>         KUNIT_EXPECT_NOT_ERR_OR_NULL(test, concat_string);
>         KUNIT_EXPECT_EQ(test, strlen(concat_string), total_len);
>
> @@ -165,17 +165,18 @@ static void string_stream_append_test(struct kunit *test)
>
>         /* Append content of empty stream to empty stream */
>         string_stream_append(stream_1, stream_2);
> -       KUNIT_EXPECT_EQ(test, strlen(string_stream_get_string(stream_1)), 0);
> +       KUNIT_EXPECT_EQ(test, strlen(string_stream_get_string(test, stream_1, GFP_KERNEL)), 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 = string_stream_get_string(stream_1);
> +       original_content = string_stream_get_string(test, stream_1, GFP_KERNEL);
>
>         /* Append content of empty stream to non-empty stream */
>         string_stream_append(stream_1, stream_2);
> -       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream_1), original_content);
> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream_1, GFP_KERNEL),
> +                          original_content);
>
>         /* Add some data to stream_2 */
>         for (i = 0; i < ARRAY_SIZE(strings_2); ++i)
> @@ -188,14 +189,15 @@ static void string_stream_append_test(struct kunit *test)
>          * End result should be the original content of stream_1 plus
>          * the content of stream_2.
>          */
> -       stream_2_content = string_stream_get_string(stream_2);
> +       stream_2_content = string_stream_get_string(test, stream_2, GFP_KERNEL);
>         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, string_stream_get_string(stream_1), combined_content);
> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream_1, GFP_KERNEL),
> +                          combined_content);
>
>         /* Append content of non-empty stream to empty stream */
>         string_stream_destroy(stream_1);
> @@ -204,7 +206,8 @@ static void string_stream_append_test(struct kunit *test)
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
>
>         string_stream_append(stream_1, stream_2);
> -       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream_1), stream_2_content);
> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream_1, GFP_KERNEL),
> +                          stream_2_content);
>  }
>
>  /* Adding an empty string should not create a fragment. */
> @@ -224,7 +227,8 @@ static void string_stream_append_empty_string_test(struct kunit *test)
>         string_stream_add(stream, "Add this line");
>         string_stream_add(stream, "%s", "");
>         KUNIT_EXPECT_EQ(test, list_count_nodes(&stream->fragments), 1);
> -       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream), "Add this line");
> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream, GFP_KERNEL),
> +                          "Add this line");
>  }
>
>  /* Adding strings without automatic newline appending */
> @@ -244,7 +248,7 @@ static void string_stream_no_auto_newline_test(struct kunit *test)
>         string_stream_add(stream, "Two\n");
>         string_stream_add(stream, "%s\n", "Three");
>         string_stream_add(stream, "Four");
> -       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream),
> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream, GFP_KERNEL),
>                            "OneTwo\nThree\nFour");
>  }
>
> @@ -271,7 +275,7 @@ static void string_stream_auto_newline_test(struct kunit *test)
>         string_stream_add(stream, "Five\n%s", "Six");
>         string_stream_add(stream, "Seven\n\n");
>         string_stream_add(stream, "Eight");
> -       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream),
> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream, GFP_KERNEL),
>                            "One\nTwo\nThree\nFour\nFive\nSix\nSeven\n\nEight\n");
>  }
>
> diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
> index 1dcf6513b692..eb673d3ea3bd 100644
> --- a/lib/kunit/string-stream.c
> +++ b/lib/kunit/string-stream.c
> @@ -117,13 +117,14 @@ static void string_stream_clear(struct string_stream *stream)
>         spin_unlock(&stream->lock);
>  }
>
> -char *string_stream_get_string(struct string_stream *stream)
> +char *string_stream_get_string(struct kunit *test, struct string_stream *stream,
> +                              gfp_t gfp)
>  {
>         struct string_stream_fragment *frag_container;
>         size_t buf_len = stream->length + 1; /* +1 for null byte. */
>         char *buf;
>
> -       buf = kunit_kzalloc(stream->test, buf_len, stream->gfp);
> +       buf = kunit_kzalloc(test, buf_len, gfp);
>         if (!buf)
>                 return NULL;
>
> @@ -140,8 +141,7 @@ int string_stream_append(struct string_stream *stream,
>  {
>         const char *other_content;
>
> -       other_content = string_stream_get_string(other);
> -
> +       other_content = string_stream_get_string(other->test, other, other->gfp);
>         if (!other_content)
>                 return -ENOMEM;
>
> diff --git a/lib/kunit/string-stream.h b/lib/kunit/string-stream.h
> index 048930bf97f0..6b4a747881c5 100644
> --- a/lib/kunit/string-stream.h
> +++ b/lib/kunit/string-stream.h
> @@ -39,7 +39,8 @@ int __printf(2, 0) string_stream_vadd(struct string_stream *stream,
>                                       const char *fmt,
>                                       va_list args);
>
> -char *string_stream_get_string(struct string_stream *stream);
> +char *string_stream_get_string(struct kunit *test, struct string_stream *stream,
> +                              gfp_t gfp);
>
>  int string_stream_append(struct string_stream *stream,
>                          struct string_stream *other);
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 49698a168437..520e15f49d0d 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -286,7 +286,7 @@ static void kunit_print_string_stream(struct kunit *test,
>         if (string_stream_is_empty(stream))
>                 return;
>
> -       buf = string_stream_get_string(stream);
> +       buf = string_stream_get_string(test, stream, GFP_KERNEL);
>         if (!buf) {
>                 kunit_err(test,
>                           "Could not allocate buffer, dumping stream:\n");
> --
> 2.30.2
>

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

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

[David Gow]

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

On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> Re-work string_stream so that it is not tied to a struct kunit. This is
> to allow using it for the log of struct kunit_suite.
>
> Instead of resource-managing individual allocations the whole string_stream
> object can be resource-managed as a single object:
>
>     alloc_string_stream() API is unchanged and takes a pointer to a
>     struct kunit but it now registers the returned string_stream object to
>     be resource-managed.
>
>     raw_alloc_string_stream() is a new function that allocates a
>     bare string_stream without any association to a struct kunit.
>
>     free_string_stream() is a new function that frees a resource-managed
>     string_stream allocated by alloc_string_stream().
>
>     raw_free_string_stream() is a new function that frees a non-managed
>     string_stream allocated by raw_alloc_string_stream().
>
> The confusing function string_stream_destroy() has been removed. This only
> called string_stream_clear() but didn't free the struct string_stream.
> Instead string_stream_clear() has been exported, and the new functions use
> the more conventional naming of "free" as the opposite of "alloc".
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---

I'm in favour of this. Should we go further and get rid of the struct
kunit member from string_stream totally?

Also, note that the kunit_action_t casting is causing warnings on some
clang configs (and technically isn't valid C). Personally, I still
like it, but expect more emails from the kernel test robot and others.

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

>  lib/kunit/string-stream-test.c |  2 +-
>  lib/kunit/string-stream.c      | 92 +++++++++++++++++++++++-----------
>  lib/kunit/string-stream.h      | 12 ++++-
>  lib/kunit/test.c               |  2 +-
>  4 files changed, 77 insertions(+), 31 deletions(-)
>
> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
> index 8a30bb7d5fb7..437aa4b3179d 100644
> --- a/lib/kunit/string-stream-test.c
> +++ b/lib/kunit/string-stream-test.c
> @@ -200,7 +200,7 @@ static void string_stream_append_test(struct kunit *test)
>                            combined_content);
>
>         /* Append content of non-empty stream to empty stream */
> -       string_stream_destroy(stream_1);
> +       string_stream_clear(stream_1);
>
>         stream_1 = alloc_string_stream(test, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
> diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
> index eb673d3ea3bd..06104a729b45 100644
> --- a/lib/kunit/string-stream.c
> +++ b/lib/kunit/string-stream.c
> @@ -13,30 +13,28 @@
>  #include "string-stream.h"
>
>
> -static struct string_stream_fragment *alloc_string_stream_fragment(
> -               struct kunit *test, int len, gfp_t gfp)
> +static struct string_stream_fragment *alloc_string_stream_fragment(int len, gfp_t gfp)
>  {
>         struct string_stream_fragment *frag;
>
> -       frag = kunit_kzalloc(test, sizeof(*frag), gfp);
> +       frag = kzalloc(sizeof(*frag), gfp);
>         if (!frag)
>                 return ERR_PTR(-ENOMEM);
>
> -       frag->fragment = kunit_kmalloc(test, len, gfp);
> +       frag->fragment = kmalloc(len, gfp);
>         if (!frag->fragment) {
> -               kunit_kfree(test, frag);
> +               kfree(frag);
>                 return ERR_PTR(-ENOMEM);
>         }
>
>         return frag;
>  }
>
> -static void string_stream_fragment_destroy(struct kunit *test,
> -                                          struct string_stream_fragment *frag)
> +static void string_stream_fragment_destroy(struct string_stream_fragment *frag)
>  {
>         list_del(&frag->node);
> -       kunit_kfree(test, frag->fragment);
> -       kunit_kfree(test, frag);
> +       kfree(frag->fragment);
> +       kfree(frag);
>  }
>
>  int string_stream_vadd(struct string_stream *stream,
> @@ -65,9 +63,7 @@ int string_stream_vadd(struct string_stream *stream,
>         /* Need space for null byte. */
>         buf_len++;
>
> -       frag_container = alloc_string_stream_fragment(stream->test,
> -                                                     buf_len,
> -                                                     stream->gfp);
> +       frag_container = alloc_string_stream_fragment(buf_len, stream->gfp);
>         if (IS_ERR(frag_container))
>                 return PTR_ERR(frag_container);
>
> @@ -102,7 +98,7 @@ int string_stream_add(struct string_stream *stream, const char *fmt, ...)
>         return result;
>  }
>
> -static void string_stream_clear(struct string_stream *stream)
> +void string_stream_clear(struct string_stream *stream)
>  {
>         struct string_stream_fragment *frag_container, *frag_container_safe;
>
> @@ -111,16 +107,28 @@ static void string_stream_clear(struct string_stream *stream)
>                                  frag_container_safe,
>                                  &stream->fragments,
>                                  node) {
> -               string_stream_fragment_destroy(stream->test, frag_container);
> +               string_stream_fragment_destroy(frag_container);
>         }
>         stream->length = 0;
>         spin_unlock(&stream->lock);
>  }
>
> +static void _string_stream_concatenate_to_buf(struct string_stream *stream,
> +                                             char *buf, size_t buf_len)
> +{
> +       struct string_stream_fragment *frag_container;
> +
> +       buf[0] = '\0';
> +
> +       spin_lock(&stream->lock);
> +       list_for_each_entry(frag_container, &stream->fragments, node)
> +               strlcat(buf, frag_container->fragment, buf_len);
> +       spin_unlock(&stream->lock);
> +}
> +
>  char *string_stream_get_string(struct kunit *test, struct string_stream *stream,
>                                gfp_t gfp)
>  {
> -       struct string_stream_fragment *frag_container;
>         size_t buf_len = stream->length + 1; /* +1 for null byte. */
>         char *buf;
>
> @@ -128,10 +136,7 @@ char *string_stream_get_string(struct kunit *test, struct string_stream *stream,
>         if (!buf)
>                 return NULL;
>
> -       spin_lock(&stream->lock);
> -       list_for_each_entry(frag_container, &stream->fragments, node)
> -               strlcat(buf, frag_container->fragment, buf_len);
> -       spin_unlock(&stream->lock);
> +       _string_stream_concatenate_to_buf(stream, buf, buf_len);
>
>         return buf;
>  }
> @@ -139,13 +144,20 @@ char *string_stream_get_string(struct kunit *test, struct string_stream *stream,
>  int string_stream_append(struct string_stream *stream,
>                          struct string_stream *other)
>  {
> -       const char *other_content;
> +       size_t other_buf_len = other->length + 1; /* +1 for null byte. */
> +       char *other_buf;
> +       int ret;
>
> -       other_content = string_stream_get_string(other->test, other, other->gfp);
> -       if (!other_content)
> +       other_buf = kmalloc(other_buf_len, GFP_KERNEL);
> +       if (!other_buf)
>                 return -ENOMEM;
>
> -       return string_stream_add(stream, other_content);
> +       _string_stream_concatenate_to_buf(other, other_buf, other_buf_len);
> +
> +       ret = string_stream_add(stream, other_buf);
> +       kfree(other_buf);
> +
> +       return ret;
>  }
>
>  bool string_stream_is_empty(struct string_stream *stream)
> @@ -153,23 +165,47 @@ bool string_stream_is_empty(struct string_stream *stream)
>         return list_empty(&stream->fragments);
>  }
>
> -struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp)
> +void raw_free_string_stream(struct string_stream *stream)
> +{
> +       string_stream_clear(stream);
> +       kfree(stream);
> +}
> +
> +struct string_stream *raw_alloc_string_stream(gfp_t gfp)
>  {
>         struct string_stream *stream;
>
> -       stream = kunit_kzalloc(test, sizeof(*stream), gfp);
> +       stream = kzalloc(sizeof(*stream), gfp);
>         if (!stream)
>                 return ERR_PTR(-ENOMEM);
>
>         stream->gfp = gfp;
> -       stream->test = test;
>         INIT_LIST_HEAD(&stream->fragments);
>         spin_lock_init(&stream->lock);
>
>         return stream;
>  }
>
> -void string_stream_destroy(struct string_stream *stream)
> +struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp)
>  {
> -       string_stream_clear(stream);
> +       struct string_stream *stream;
> +
> +       stream = raw_alloc_string_stream(gfp);
> +       if (IS_ERR(stream))
> +               return stream;
> +
> +       stream->test = test;
> +
> +       if (kunit_add_action_or_reset(test, (kunit_action_t *)raw_free_string_stream, stream) != 0)
> +               return ERR_PTR(-ENOMEM);

As the kernel test robot notes, this sort of function pointer casting
is not technically valid C, and some compiler setups are starting to
warn on it.

Personally, I'm still okay with it, but expect a bunch of robot email
complaining about it if it lands. If more compilers / configs start
warning, though, I'll try to fix all callers of the KUnit action
functions which are affected.

> +
> +       return stream;
> +}
> +
> +void free_string_stream(struct kunit *test, struct string_stream *stream)
> +{
> +       if (!stream)
> +               return;
> +
> +       kunit_release_action(test, (kunit_action_t *)raw_free_string_stream, (void *)stream);

(As above.)

>  }
> diff --git a/lib/kunit/string-stream.h b/lib/kunit/string-stream.h
> index 6b4a747881c5..fbeda486382a 100644
> --- a/lib/kunit/string-stream.h
> +++ b/lib/kunit/string-stream.h
> @@ -23,14 +23,24 @@ struct string_stream {
>         struct list_head fragments;
>         /* length and fragments are protected by this lock */
>         spinlock_t lock;
> +
> +       /*
> +        * Pointer to kunit this stream is associated with, or NULL if
> +        * not associated with a kunit.
> +        */
>         struct kunit *test;
> +

Can we just get rid of this totally? I don't think anything's actually
using it now. (Or have I missed something?)


>         gfp_t gfp;
>         bool append_newlines;
>  };
>
>  struct kunit;
>
> +struct string_stream *raw_alloc_string_stream(gfp_t gfp);
> +void raw_free_string_stream(struct string_stream *stream);
> +
>  struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp);
> +void free_string_stream(struct kunit *test, struct string_stream *stream);
>
>  int __printf(2, 3) string_stream_add(struct string_stream *stream,
>                                      const char *fmt, ...);
> @@ -47,7 +57,7 @@ int string_stream_append(struct string_stream *stream,
>
>  bool string_stream_is_empty(struct string_stream *stream);
>
> -void string_stream_destroy(struct string_stream *stream);
> +void string_stream_clear(struct string_stream *stream);
>
>  static inline void string_stream_set_append_newlines(struct string_stream *stream,
>                                                      bool append_newlines)
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 520e15f49d0d..4b69d12dfc96 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -322,7 +322,7 @@ static void kunit_fail(struct kunit *test, const struct kunit_loc *loc,
>
>         kunit_print_string_stream(test, stream);
>
> -       string_stream_destroy(stream);
> +       free_string_stream(test, stream);
>  }
>
>  void __noreturn __kunit_abort(struct kunit *test)
> --
> 2.30.2
>

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

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

[David Gow]

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

On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> string_stream_resource_free_test() allocates a resource-managed
> string_stream and tests that raw_free_string_stream() is called when
> the test resources are cleaned up.
>
> string_stream_init_test() is extended to test allocating a
> string_stream that is not resource-managed.
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---

Glad to see this tested. It's nice to see the static_stub being useful
here, too.

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

Cheers,
-- David

>  lib/kunit/string-stream-test.c | 117 ++++++++++++++++++++++++++++++++-
>  lib/kunit/string-stream.c      |   3 +
>  2 files changed, 119 insertions(+), 1 deletion(-)
>
> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
> index 437aa4b3179d..05bfade2bd8a 100644
> --- a/lib/kunit/string-stream-test.c
> +++ b/lib/kunit/string-stream-test.c
> @@ -6,16 +6,27 @@
>   * 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 {
> +       struct string_stream *raw_stream;
> +
> +       /* For testing resource-managed free */
> +       struct string_stream *freed_stream;
> +       bool stream_free_again;
> +};
> +
>  /* string_stream object is initialized correctly. */
>  static void string_stream_init_test(struct kunit *test)
>  {
> +       struct string_stream_test_priv *priv = test->priv;
>         struct string_stream *stream;
>
> +       /* Resource-managed initialization */
>         stream = alloc_string_stream(test, GFP_KERNEL);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
>
> @@ -26,6 +37,86 @@ static void string_stream_init_test(struct kunit *test)
>         KUNIT_EXPECT_FALSE(test, stream->append_newlines);
>
>         KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
> +
> +       free_string_stream(test, stream);
> +
> +       /*
> +        * Raw initialization. This allocation is not resource-managed
> +        * so store it in priv->raw_stream to be cleaned up by the
> +        * exit function.
> +        */
> +       priv->raw_stream = raw_alloc_string_stream(GFP_KERNEL);
> +       stream = priv->raw_stream;
> +       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, NULL);
> +       KUNIT_EXPECT_EQ(test, stream->gfp, GFP_KERNEL);

Again, this will probably annoy sparse, so expect an email from ktr.
We'll look into how to fix this in either KUnit or sparse separately.


> +       KUNIT_EXPECT_FALSE(test, stream->append_newlines);
> +
> +       KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
> +}
> +
> +static void string_stream_raw_free_string_stream_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.
> +        */
> +}
> +
> +/* 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,
> +                                  raw_free_string_stream,
> +                                  string_stream_raw_free_string_stream_stub);
> +
> +       stream = alloc_string_stream(fake_test, GFP_KERNEL);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
> +
> +       /*
> +        * Ensure the stream is freed when this test terminates.
> +        */
> +       priv->raw_stream = 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);
>  }
>
>  /*
> @@ -279,8 +370,30 @@ 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 void string_stream_test_exit(struct kunit *test)
> +{
> +       struct string_stream_test_priv *priv = test->priv;
> +
> +       if (priv && priv->raw_stream)
> +               raw_free_string_stream(priv->raw_stream);
> +}
> +
>  static struct kunit_case string_stream_test_cases[] = {
>         KUNIT_CASE(string_stream_init_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),
> @@ -292,6 +405,8 @@ 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,
> +       .exit = string_stream_test_exit,
>  };
>  kunit_test_suites(&string_stream_test_suite);
> diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
> index 06104a729b45..1b55ac1be2e5 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>
> @@ -167,6 +168,8 @@ bool string_stream_is_empty(struct string_stream *stream)
>
>  void raw_free_string_stream(struct string_stream *stream)
>  {
> +       KUNIT_STATIC_STUB_REDIRECT(raw_free_string_stream, stream);
> +
>         string_stream_clear(stream);
>         kfree(stream);
>  }
> --
> 2.30.2
>

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

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

[David Gow]

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

On Mon, 14 Aug 2023 at 21:23, 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.
>
> The existing kunit log tests have been updated for using a
> string_stream as the log. As they now depend on string_stream
> functions they cannot build when kunit-test is a module. They have
> been moved to a separate source file to be built only if kunit-test
> is built-in.
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---

Yay! This is much nicer.

A part of me wonders if it makes sense to do something to allow the
tests to continue to work as a module (either exporting the needed
string stream functions, or moving the tests into the main KUnit
module, and just exporting the suite definition to a stub module).
Probably not worth it, though...

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

Cheers,
-- David


>  include/kunit/test.h   | 14 ++++----
>  lib/kunit/Makefile     |  5 +--
>  lib/kunit/debugfs.c    | 36 +++++++++++++--------
>  lib/kunit/kunit-test.c | 52 +-----------------------------
>  lib/kunit/log-test.c   | 72 ++++++++++++++++++++++++++++++++++++++++++
>  lib/kunit/test.c       | 44 +++-----------------------
>  6 files changed, 110 insertions(+), 113 deletions(-)
>  create mode 100644 lib/kunit/log-test.c
>
> 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/Makefile b/lib/kunit/Makefile
> index 46f75f23dfe4..65735c2e1d14 100644
> --- a/lib/kunit/Makefile
> +++ b/lib/kunit/Makefile
> @@ -18,9 +18,10 @@ obj-y +=                             hooks.o
>
>  obj-$(CONFIG_KUNIT_TEST) +=            kunit-test.o
>
> -# string-stream-test compiles built-in only.
> +# string-stream-test and log-test compiles built-in only.
>  ifeq ($(CONFIG_KUNIT_TEST),y)
> -obj-$(CONFIG_KUNIT_TEST) +=            string-stream-test.o
> +obj-$(CONFIG_KUNIT_TEST) +=            string-stream-test.o \
> +                                       log-test.o
>  endif
>
>  obj-$(CONFIG_KUNIT_EXAMPLE_TEST) +=    kunit-example-test.o
> diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c
> index 22c5c496a68f..ab53a7e5c898 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 = raw_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 = raw_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);
> +       raw_free_string_stream(suite->log);
>         kunit_suite_for_each_test_case(suite, test_case)
> -               kfree(test_case->log);
> +               raw_free_string_stream(test_case->log);
>  }
> diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
> index 83d8e90ca7a2..ecc39d5f7411 100644
> --- a/lib/kunit/kunit-test.c
> +++ b/lib/kunit/kunit-test.c
> @@ -530,55 +530,6 @@ static struct kunit_suite kunit_resource_test_suite = {
>         .test_cases = kunit_resource_test_cases,
>  };
>
> -static void kunit_log_test(struct kunit *test)
> -{
> -       struct kunit_suite suite;
> -
> -       suite.log = kunit_kzalloc(test, KUNIT_LOG_SIZE, GFP_KERNEL);
> -       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, suite.log);
> -
> -       kunit_log(KERN_INFO, test, "put this in log.");
> -       kunit_log(KERN_INFO, test, "this too.");
> -       kunit_log(KERN_INFO, &suite, "add to suite log.");
> -       kunit_log(KERN_INFO, &suite, "along with this.");
> -
> -#ifdef CONFIG_KUNIT_DEBUGFS
> -       KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
> -                                    strstr(test->log, "put this in log."));
> -       KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
> -                                    strstr(test->log, "this too."));
> -       KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
> -                                    strstr(suite.log, "add to suite log."));
> -       KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
> -                                    strstr(suite.log, "along with this."));
> -#else
> -       KUNIT_EXPECT_NULL(test, test->log);
> -#endif
> -}
> -
> -static void kunit_log_newline_test(struct kunit *test)
> -{
> -       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"));
> -       } else {
> -               kunit_skip(test, "only useful when debugfs is enabled");
> -       }
> -}
> -
> -static struct kunit_case kunit_log_test_cases[] = {
> -       KUNIT_CASE(kunit_log_test),
> -       KUNIT_CASE(kunit_log_newline_test),
> -       {}
> -};
> -
> -static struct kunit_suite kunit_log_test_suite = {
> -       .name = "kunit-log-test",
> -       .test_cases = kunit_log_test_cases,
> -};
> -
>  static void kunit_status_set_failure_test(struct kunit *test)
>  {
>         struct kunit fake;
> @@ -658,7 +609,6 @@ static struct kunit_suite kunit_current_test_suite = {
>  };
>
>  kunit_test_suites(&kunit_try_catch_test_suite, &kunit_resource_test_suite,
> -                 &kunit_log_test_suite, &kunit_status_test_suite,
> -                 &kunit_current_test_suite);
> +                 &kunit_status_test_suite, &kunit_current_test_suite);
>
>  MODULE_LICENSE("GPL v2");
> diff --git a/lib/kunit/log-test.c b/lib/kunit/log-test.c
> new file mode 100644
> index 000000000000..a93d87112fea
> --- /dev/null
> +++ b/lib/kunit/log-test.c
> @@ -0,0 +1,72 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit test for logging.
> + *
> + * Based on code:
> + * Copyright (C) 2019, Google LLC.
> + * Author: Brendan Higgins <brendanhiggins@google.com>
> + */
> +#include <kunit/test.h>
> +
> +#include "string-stream.h"
> +
> +static void kunit_log_test(struct kunit *test)
> +{
> +       struct kunit_suite suite;
> +       char *full_log;
> +
> +       suite.log = 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.");
> +       kunit_log(KERN_INFO, &suite, "add to suite log.");
> +       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, test->log, GFP_KERNEL);
> +       KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
> +                                    strstr(full_log, "put this in log."));
> +       KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
> +                                    strstr(full_log, "this too."));
> +
> +       full_log = string_stream_get_string(test, suite.log, GFP_KERNEL);
> +       KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
> +                                    strstr(full_log, "add to suite log."));
> +       KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
> +                                    strstr(full_log, "along with this."));
> +#else
> +       KUNIT_EXPECT_NULL(test, test->log);
> +#endif
> +}
> +
> +static void kunit_log_newline_test(struct kunit *test)
> +{
> +       char *full_log;
> +
> +       kunit_info(test, "Add newline\n");
> +       if (test->log) {
> +               full_log = string_stream_get_string(test, test->log, GFP_KERNEL);
> +               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");
> +       }
> +}
> +
> +static struct kunit_case kunit_log_test_cases[] = {
> +       KUNIT_CASE(kunit_log_test),
> +       KUNIT_CASE(kunit_log_newline_test),
> +       {}
> +};
> +
> +static struct kunit_suite kunit_log_test_suite = {
> +       .name = "kunit-log-test",
> +       .test_cases = kunit_log_test_cases,
> +};
> +
> +kunit_test_suites(&kunit_log_test_suite);
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 4b69d12dfc96..14e889e80129 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 v4 10/10] kunit: string-stream: Test performance of string_stream

[David Gow]

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

On Mon, 14 Aug 2023 at 21:23, 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>
> ---

Thanks, this is very useful.

My results are (UML):
   # string_stream_performance_test: Time elapsed:           5021 us
   # string_stream_performance_test: Total string length:    573890
   # string_stream_performance_test: Bytes requested:        823930
   # string_stream_performance_test: Actual bytes allocated: 1048312

KUnit isn't really ideal for performance testing, but this works well
enough and fits in well alongside the other string stream tests.

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

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 05bfade2bd8a..b55cc14f43fb 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"
>
> @@ -370,6 +372,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 = 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;
> @@ -400,6 +453,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 v4 00/10] kunit: Add dynamically-extending log

[David Gow]

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

On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald
<rf@opensource.cirrus.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 V3:
>
> Completely rewritten to use string_stream instead of implementing a
> separate extending-buffer implementation for logging.
>
> I have used the performance test from the final patch on my original
> fixed-size-fragment implementation from V3 to get a comparison of the
> two implementations (run on i3-8145UE CPU @ 2.20GHz):
>
>                         string_stream     V3 fixed-size-buffer
> Time elapsed:           7748 us           3251 us
> Total string length:    573890            573890
> Bytes requested:        823994            728336
> Actual bytes allocated: 1061440           728352
>
> I don't think the difference is enough to be worth complicating the
> string_stream implementation with my fixed-fragment implementation from
> V3 of this patch chain.
>

Thanks very much! I think this is good to go: I've added a few small
comments on various patches, but none of them are serious enough to
make me feel we _need_ a new version.
I'll leave it for a day or two in case there are any other comments or
any changes you want to make, otherwise this can be merged.

I agree the performance difference isn't worth the effort. If we
change our minds and want to change the implementation over later,
that shouldn't be a problem either. So let's stick with it as is.

Cheers,
-- David


> Richard Fitzgerald (10):
>   kunit: string-stream: Improve testing of string_stream
>   kunit: string-stream: Don't create a fragment for empty strings
>   kunit: string-stream: Add cases for adding empty strings to a
>     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: string-stream: Pass struct kunit to string_stream_get_string()
>   kunit: string-stream: Decouple string_stream from kunit
>   kunit: string-stream: Add test 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/Makefile             |   5 +-
>  lib/kunit/debugfs.c            |  36 ++-
>  lib/kunit/kunit-test.c         |  52 +---
>  lib/kunit/log-test.c           |  72 ++++++
>  lib/kunit/string-stream-test.c | 447 +++++++++++++++++++++++++++++++--
>  lib/kunit/string-stream.c      | 129 +++++++---
>  lib/kunit/string-stream.h      |  22 +-
>  lib/kunit/test.c               |  48 +---
>  9 files changed, 656 insertions(+), 169 deletions(-)
>  create mode 100644 lib/kunit/log-test.c
>
> --
> 2.30.2
>

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

Re: [PATCH v4 06/10] kunit: string-stream: Pass struct kunit to string_stream_get_string()

[Richard Fitzgerald]

On 15/8/23 10:16, David Gow wrote:
> On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald
> <rf@opensource.cirrus.com> wrote:
>>
>> Pass a struct kunit* and gfp_t to string_stream_get_string(). Allocate
>> the returned buffer using these instead of using the stream->test and
>> stream->gfp.
>>
>> This is preparation for removing the dependence of string_stream on
>> struct kunit, so that string_stream can be used for the debugfs log.
>>
>> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
>> ---
> 
> Makes sense to me.
> 
> I think that, if we weren't going to remove the struct kunit
> dependency, we'd want to either keep a version of
> string_stream_get_string() which uses it, or, e.g., fall back to it if
> the struct passed in is NULL.
>

That was my first thought. But I thought that was open to subtle
accidental bugs in calling code - it might return you a managed
allocation, or it might return you an unmanaged allocation that you
must free.

As there weren't many callers of string_stream_get_string() I decided
to go with changing the API to pass in test and gfp like other managed
allocations. This makes it more generalized, since the returned buffer
is not part of the stream itself, it's a temporary buffer owned by the
caller. It also makes it clearer that what you are getting back is
likely to be a managed allocation.

If you'd prefer to leave string_stream_get_string() as it was, or make
it return an unmanged buffer, I can send a new version.

> The other option is to have a version which doesn't manage the string
> at all, so just takes a gfp and requires the caller to free it (or

I didn't add a companion function to get a raw unmanaged string buffer
because there's nothing that needs it. It could be added later if
it's needed.

> register an action to do so). If we did that, we could get rid of the
> struct kunit pointer totally (though it may be better to keep it and
> have both versions).
> 

Another option is to make string_stream_get_string() return a raw
buffer and add a kunit_string_stream_geT_string() that returns a
managed buffer. This follows some consistency with the normal mallocs
where kunit_xxxx() is the managed version.

> So -- to switch to some stream-of-consciousness thoughts about this --
> basically there are three possible variants of
> string_stream_get_string():
> - Current version: uses the stream's struct kunit. Useless if this is
> NULL, but very convenient otherwise.
> - This patch: manage the string using the struct kunit passed as an
> argument. Still can't be used directly outside a test, but adds enough
> flexibility to get _append to work.
> - Totally unmanaged: the generated string is allocated separately, and
> must be freed (directly or in a deferred action) by the caller. Works
> well outside tests, but less convenient.
> 
> Personally, I feel that the design of string_stream is heading towards
> the third option. By the end of this series, everything uses
> _string_stream_concatenate_to_buf() anyway. There's only one call to
> string_stream_get_string() outside of the logging / string_stream
> tests, and all that does is log the results (and it has a fallback to
> log each fragment separately if the allocation fails).
> 
> Then again, if this is only really being used in tests, then we can
> probably just stick with string_stream_get_string() as-is, remove the
> string_stream->test member totally, and if we need it, we can make
> _string_stream_concatenate_to_buf() public, or add an unmanaged
> version anyway.
>

I didn't remove ->test because there's some existing code in assert.c
that uses it, and I didn't want to break that.

> So, after all that, I think this is probably good as-is. _Maybe_ we
> could rename string_stream_get_string() to something like
> string_stream_get_managed_string(), now that it's the only function
> which is "managed" as a KUnit resource, but we can equally put that
> off until we need to add an unmanaged version.
> 
> Reviewed-by: David Gow <davidgow@google.com>
> 
> Cheers,
> -- David
> 
> 
>>   lib/kunit/string-stream-test.c | 26 +++++++++++++++-----------
>>   lib/kunit/string-stream.c      |  8 ++++----
>>   lib/kunit/string-stream.h      |  3 ++-
>>   lib/kunit/test.c               |  2 +-
>>   4 files changed, 22 insertions(+), 17 deletions(-)
>>
>> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
>> index 46c2ac162fe8..8a30bb7d5fb7 100644
>> --- a/lib/kunit/string-stream-test.c
>> +++ b/lib/kunit/string-stream-test.c
>> @@ -57,7 +57,7 @@ static void string_stream_line_add_test(struct kunit *test)
>>          }
>>          num_lines = i;
>>
>> -       concat_string = string_stream_get_string(stream);
>> +       concat_string = string_stream_get_string(test, stream, GFP_KERNEL);
>>          KUNIT_EXPECT_NOT_ERR_OR_NULL(test, concat_string);
>>          KUNIT_EXPECT_EQ(test, strlen(concat_string), total_len);
>>
>> @@ -113,7 +113,7 @@ static void string_stream_variable_length_line_test(struct kunit *test)
>>          }
>>          num_lines = i;
>>
>> -       concat_string = string_stream_get_string(stream);
>> +       concat_string = string_stream_get_string(test, stream, GFP_KERNEL);
>>          KUNIT_EXPECT_NOT_ERR_OR_NULL(test, concat_string);
>>          KUNIT_EXPECT_EQ(test, strlen(concat_string), total_len);
>>
>> @@ -165,17 +165,18 @@ static void string_stream_append_test(struct kunit *test)
>>
>>          /* Append content of empty stream to empty stream */
>>          string_stream_append(stream_1, stream_2);
>> -       KUNIT_EXPECT_EQ(test, strlen(string_stream_get_string(stream_1)), 0);
>> +       KUNIT_EXPECT_EQ(test, strlen(string_stream_get_string(test, stream_1, GFP_KERNEL)), 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 = string_stream_get_string(stream_1);
>> +       original_content = string_stream_get_string(test, stream_1, GFP_KERNEL);
>>
>>          /* Append content of empty stream to non-empty stream */
>>          string_stream_append(stream_1, stream_2);
>> -       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream_1), original_content);
>> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream_1, GFP_KERNEL),
>> +                          original_content);
>>
>>          /* Add some data to stream_2 */
>>          for (i = 0; i < ARRAY_SIZE(strings_2); ++i)
>> @@ -188,14 +189,15 @@ static void string_stream_append_test(struct kunit *test)
>>           * End result should be the original content of stream_1 plus
>>           * the content of stream_2.
>>           */
>> -       stream_2_content = string_stream_get_string(stream_2);
>> +       stream_2_content = string_stream_get_string(test, stream_2, GFP_KERNEL);
>>          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, string_stream_get_string(stream_1), combined_content);
>> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream_1, GFP_KERNEL),
>> +                          combined_content);
>>
>>          /* Append content of non-empty stream to empty stream */
>>          string_stream_destroy(stream_1);
>> @@ -204,7 +206,8 @@ static void string_stream_append_test(struct kunit *test)
>>          KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
>>
>>          string_stream_append(stream_1, stream_2);
>> -       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream_1), stream_2_content);
>> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream_1, GFP_KERNEL),
>> +                          stream_2_content);
>>   }
>>
>>   /* Adding an empty string should not create a fragment. */
>> @@ -224,7 +227,8 @@ static void string_stream_append_empty_string_test(struct kunit *test)
>>          string_stream_add(stream, "Add this line");
>>          string_stream_add(stream, "%s", "");
>>          KUNIT_EXPECT_EQ(test, list_count_nodes(&stream->fragments), 1);
>> -       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream), "Add this line");
>> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream, GFP_KERNEL),
>> +                          "Add this line");
>>   }
>>
>>   /* Adding strings without automatic newline appending */
>> @@ -244,7 +248,7 @@ static void string_stream_no_auto_newline_test(struct kunit *test)
>>          string_stream_add(stream, "Two\n");
>>          string_stream_add(stream, "%s\n", "Three");
>>          string_stream_add(stream, "Four");
>> -       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream),
>> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream, GFP_KERNEL),
>>                             "OneTwo\nThree\nFour");
>>   }
>>
>> @@ -271,7 +275,7 @@ static void string_stream_auto_newline_test(struct kunit *test)
>>          string_stream_add(stream, "Five\n%s", "Six");
>>          string_stream_add(stream, "Seven\n\n");
>>          string_stream_add(stream, "Eight");
>> -       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream),
>> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream, GFP_KERNEL),
>>                             "One\nTwo\nThree\nFour\nFive\nSix\nSeven\n\nEight\n");
>>   }
>>
>> diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
>> index 1dcf6513b692..eb673d3ea3bd 100644
>> --- a/lib/kunit/string-stream.c
>> +++ b/lib/kunit/string-stream.c
>> @@ -117,13 +117,14 @@ static void string_stream_clear(struct string_stream *stream)
>>          spin_unlock(&stream->lock);
>>   }
>>
>> -char *string_stream_get_string(struct string_stream *stream)
>> +char *string_stream_get_string(struct kunit *test, struct string_stream *stream,
>> +                              gfp_t gfp)
>>   {
>>          struct string_stream_fragment *frag_container;
>>          size_t buf_len = stream->length + 1; /* +1 for null byte. */
>>          char *buf;
>>
>> -       buf = kunit_kzalloc(stream->test, buf_len, stream->gfp);
>> +       buf = kunit_kzalloc(test, buf_len, gfp);
>>          if (!buf)
>>                  return NULL;
>>
>> @@ -140,8 +141,7 @@ int string_stream_append(struct string_stream *stream,
>>   {
>>          const char *other_content;
>>
>> -       other_content = string_stream_get_string(other);
>> -
>> +       other_content = string_stream_get_string(other->test, other, other->gfp);
>>          if (!other_content)
>>                  return -ENOMEM;
>>
>> diff --git a/lib/kunit/string-stream.h b/lib/kunit/string-stream.h
>> index 048930bf97f0..6b4a747881c5 100644
>> --- a/lib/kunit/string-stream.h
>> +++ b/lib/kunit/string-stream.h
>> @@ -39,7 +39,8 @@ int __printf(2, 0) string_stream_vadd(struct string_stream *stream,
>>                                        const char *fmt,
>>                                        va_list args);
>>
>> -char *string_stream_get_string(struct string_stream *stream);
>> +char *string_stream_get_string(struct kunit *test, struct string_stream *stream,
>> +                              gfp_t gfp);
>>
>>   int string_stream_append(struct string_stream *stream,
>>                           struct string_stream *other);
>> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
>> index 49698a168437..520e15f49d0d 100644
>> --- a/lib/kunit/test.c
>> +++ b/lib/kunit/test.c
>> @@ -286,7 +286,7 @@ static void kunit_print_string_stream(struct kunit *test,
>>          if (string_stream_is_empty(stream))
>>                  return;
>>
>> -       buf = string_stream_get_string(stream);
>> +       buf = string_stream_get_string(test, stream, GFP_KERNEL);
>>          if (!buf) {
>>                  kunit_err(test,
>>                            "Could not allocate buffer, dumping stream:\n");
>> --
>> 2.30.2
>>

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

[Richard Fitzgerald]

On 15/8/23 10:16, David Gow wrote:
> On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald
> <rf@opensource.cirrus.com> wrote:
>>
>> Re-work string_stream so that it is not tied to a struct kunit. This is
>> to allow using it for the log of struct kunit_suite.
>>
>> Instead of resource-managing individual allocations the whole string_stream
>> object can be resource-managed as a single object:
>>
>>      alloc_string_stream() API is unchanged and takes a pointer to a
>>      struct kunit but it now registers the returned string_stream object to
>>      be resource-managed.
>>
>>      raw_alloc_string_stream() is a new function that allocates a
>>      bare string_stream without any association to a struct kunit.
>>
>>      free_string_stream() is a new function that frees a resource-managed
>>      string_stream allocated by alloc_string_stream().
>>
>>      raw_free_string_stream() is a new function that frees a non-managed
>>      string_stream allocated by raw_alloc_string_stream().
>>
>> The confusing function string_stream_destroy() has been removed. This only
>> called string_stream_clear() but didn't free the struct string_stream.
>> Instead string_stream_clear() has been exported, and the new functions use
>> the more conventional naming of "free" as the opposite of "alloc".
>>
>> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
>> ---
> 
> I'm in favour of this. Should we go further and get rid of the struct
> kunit member from string_stream totally?
>

I can do that. I was worried about some hairy-looking code in assert.c
that used stream->test. But I've just looked at it again and it's
really quite simple, and doesn't even need ->test. is_literal()
allocates a temporary managed buffer, but it frees it before returning
so it doesn't need to be managed.

> Also, note that the kunit_action_t casting is causing warnings on some
> clang configs (and technically isn't valid C). Personally, I still
> like it, but expect more emails from the kernel test robot and others.
> 

I can send a new version to fix that.

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

[Rae Moar]

On Mon, Aug 14, 2023 at 9:23 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!

I just wanted to look over handling the newline and this looks good to
me. I agree with David that I don't mind one extra allocated byte.

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 v4 05/10] kunit: string-stream: Add cases for string_stream newline appending

[Rae Moar]

On Mon, Aug 14, 2023 at 9:23 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.
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>

Hello!

These two cases seem very clean to me. I appreciate the organization
and commenting on the tests.

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

Thanks!
-Rae

> ---
>  lib/kunit/string-stream-test.c | 51 ++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>
> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
> index efe13e3322b5..46c2ac162fe8 100644
> --- a/lib/kunit/string-stream-test.c
> +++ b/lib/kunit/string-stream-test.c
> @@ -23,6 +23,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));
>  }
> @@ -226,12 +227,62 @@ static void string_stream_append_empty_string_test(struct kunit *test)
>         KUNIT_EXPECT_STREQ(test, string_stream_get_string(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. No extra newlines should be
> +        * added.
> +        */
> +       string_stream_add(stream, "One");
> +       string_stream_add(stream, "Two\n");
> +       string_stream_add(stream, "%s\n", "Three");
> +       string_stream_add(stream, "Four");
> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream),
> +                          "OneTwo\nThree\nFour");
> +}
> +
> +/* 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, string_stream_get_string(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_empty_string_test),
> +       KUNIT_CASE(string_stream_no_auto_newline_test),
> +       KUNIT_CASE(string_stream_auto_newline_test),
>         {}
>  };
>
> --
> 2.30.2
>

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

[Rae Moar]

On Mon, Aug 14, 2023 at 9:23 AM Richard Fitzgerald
<rf@opensource.cirrus.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 V3:
>
> Completely rewritten to use string_stream instead of implementing a
> separate extending-buffer implementation for logging.
>
> I have used the performance test from the final patch on my original
> fixed-size-fragment implementation from V3 to get a comparison of the
> two implementations (run on i3-8145UE CPU @ 2.20GHz):
>
>                         string_stream     V3 fixed-size-buffer
> Time elapsed:           7748 us           3251 us
> Total string length:    573890            573890
> Bytes requested:        823994            728336
> Actual bytes allocated: 1061440           728352
>
> I don't think the difference is enough to be worth complicating the
> string_stream implementation with my fixed-fragment implementation from
> V3 of this patch chain.

Hello!

I just wanted to note that I have tested this series and it looks good
to me. I specifically looked over the newline handling and that looks
really good.

I understand you will likely be doing a new version of this. But other
than the things noted in comments by David, this seems to be working
really well.

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

Thanks for all the work you did in moving this framework to string-stream!
-Rae

>
> Richard Fitzgerald (10):
>   kunit: string-stream: Improve testing of string_stream
>   kunit: string-stream: Don't create a fragment for empty strings
>   kunit: string-stream: Add cases for adding empty strings to a
>     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: string-stream: Pass struct kunit to string_stream_get_string()
>   kunit: string-stream: Decouple string_stream from kunit
>   kunit: string-stream: Add test 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/Makefile             |   5 +-
>  lib/kunit/debugfs.c            |  36 ++-
>  lib/kunit/kunit-test.c         |  52 +---
>  lib/kunit/log-test.c           |  72 ++++++
>  lib/kunit/string-stream-test.c | 447 +++++++++++++++++++++++++++++++--
>  lib/kunit/string-stream.c      | 129 +++++++---
>  lib/kunit/string-stream.h      |  22 +-
>  lib/kunit/test.c               |  48 +---
>  9 files changed, 656 insertions(+), 169 deletions(-)
>  create mode 100644 lib/kunit/log-test.c
>
> --
> 2.30.2
>

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

[David Gow]

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

On Tue, 15 Aug 2023 at 18:51, Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> On 15/8/23 10:16, David Gow wrote:
> > On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald
> > <rf@opensource.cirrus.com> wrote:
> >>
> >> Re-work string_stream so that it is not tied to a struct kunit. This is
> >> to allow using it for the log of struct kunit_suite.
> >>
> >> Instead of resource-managing individual allocations the whole string_stream
> >> object can be resource-managed as a single object:
> >>
> >>      alloc_string_stream() API is unchanged and takes a pointer to a
> >>      struct kunit but it now registers the returned string_stream object to
> >>      be resource-managed.
> >>
> >>      raw_alloc_string_stream() is a new function that allocates a
> >>      bare string_stream without any association to a struct kunit.
> >>
> >>      free_string_stream() is a new function that frees a resource-managed
> >>      string_stream allocated by alloc_string_stream().
> >>
> >>      raw_free_string_stream() is a new function that frees a non-managed
> >>      string_stream allocated by raw_alloc_string_stream().
> >>
> >> The confusing function string_stream_destroy() has been removed. This only
> >> called string_stream_clear() but didn't free the struct string_stream.
> >> Instead string_stream_clear() has been exported, and the new functions use
> >> the more conventional naming of "free" as the opposite of "alloc".
> >>
> >> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> >> ---
> >
> > I'm in favour of this. Should we go further and get rid of the struct
> > kunit member from string_stream totally?
> >
>
> I can do that. I was worried about some hairy-looking code in assert.c
> that used stream->test. But I've just looked at it again and it's
> really quite simple, and doesn't even need ->test. is_literal()
> allocates a temporary managed buffer, but it frees it before returning
> so it doesn't need to be managed.
>

Yeah, let's get rid of that. Having a stream->kunit exist but be NULL
half the time is asking for issues down the line.

> > Also, note that the kunit_action_t casting is causing warnings on some
> > clang configs (and technically isn't valid C). Personally, I still
> > like it, but expect more emails from the kernel test robot and others.
> >
>
> I can send a new version to fix that.
>

That's probably best. If you want to keep it as-is, I'll fight for it,
but it's probably better to err on the side of not introducing the
warnings.

Thanks,
-- David

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

Re: [PATCH v4 06/10] kunit: string-stream: Pass struct kunit to string_stream_get_string()

[David Gow]

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

On Tue, 15 Aug 2023 at 17:57, Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> On 15/8/23 10:16, David Gow wrote:
> > On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald
> > <rf@opensource.cirrus.com> wrote:
> >>
> >> Pass a struct kunit* and gfp_t to string_stream_get_string(). Allocate
> >> the returned buffer using these instead of using the stream->test and
> >> stream->gfp.
> >>
> >> This is preparation for removing the dependence of string_stream on
> >> struct kunit, so that string_stream can be used for the debugfs log.
> >>
> >> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> >> ---
> >
> > Makes sense to me.
> >
> > I think that, if we weren't going to remove the struct kunit
> > dependency, we'd want to either keep a version of
> > string_stream_get_string() which uses it, or, e.g., fall back to it if
> > the struct passed in is NULL.
> >
>
> That was my first thought. But I thought that was open to subtle
> accidental bugs in calling code - it might return you a managed
> allocation, or it might return you an unmanaged allocation that you
> must free.
>
> As there weren't many callers of string_stream_get_string() I decided
> to go with changing the API to pass in test and gfp like other managed
> allocations. This makes it more generalized, since the returned buffer
> is not part of the stream itself, it's a temporary buffer owned by the
> caller. It also makes it clearer that what you are getting back is
> likely to be a managed allocation.
>
> If you'd prefer to leave string_stream_get_string() as it was, or make
> it return an unmanged buffer, I can send a new version.
>
> > The other option is to have a version which doesn't manage the string
> > at all, so just takes a gfp and requires the caller to free it (or
>
> I didn't add a companion function to get a raw unmanaged string buffer
> because there's nothing that needs it. It could be added later if
> it's needed.
>

Fair enough.

> > register an action to do so). If we did that, we could get rid of the
> > struct kunit pointer totally (though it may be better to keep it and
> > have both versions).
> >
>
> Another option is to make string_stream_get_string() return a raw
> buffer and add a kunit_string_stream_geT_string() that returns a
> managed buffer. This follows some consistency with the normal mallocs
> where kunit_xxxx() is the managed version.
>

Ooh... I like this best. Let's go with that.


> > So -- to switch to some stream-of-consciousness thoughts about this --
> > basically there are three possible variants of
> > string_stream_get_string():
> > - Current version: uses the stream's struct kunit. Useless if this is
> > NULL, but very convenient otherwise.
> > - This patch: manage the string using the struct kunit passed as an
> > argument. Still can't be used directly outside a test, but adds enough
> > flexibility to get _append to work.
> > - Totally unmanaged: the generated string is allocated separately, and
> > must be freed (directly or in a deferred action) by the caller. Works
> > well outside tests, but less convenient.
> >
> > Personally, I feel that the design of string_stream is heading towards
> > the third option. By the end of this series, everything uses
> > _string_stream_concatenate_to_buf() anyway. There's only one call to
> > string_stream_get_string() outside of the logging / string_stream
> > tests, and all that does is log the results (and it has a fallback to
> > log each fragment separately if the allocation fails).
> >
> > Then again, if this is only really being used in tests, then we can
> > probably just stick with string_stream_get_string() as-is, remove the
> > string_stream->test member totally, and if we need it, we can make
> > _string_stream_concatenate_to_buf() public, or add an unmanaged
> > version anyway.
> >
>
> I didn't remove ->test because there's some existing code in assert.c
> that uses it, and I didn't want to break that.
>

Since it seems the assert stuff isn't too difficult to make unmanaged,
can we go ahead and remove ->test?

If it turns out to be too nasty, let me know and we'll keep it (it's
not worth making major excavations), but I'd prefer to get rid of it
if we can.

Thanks,
-- David

> > So, after all that, I think this is probably good as-is. _Maybe_ we
> > could rename string_stream_get_string() to something like
> > string_stream_get_managed_string(), now that it's the only function
> > which is "managed" as a KUnit resource, but we can equally put that
> > off until we need to add an unmanaged version.
> >
> > Reviewed-by: David Gow <davidgow@google.com>
> >
> > Cheers,
> > -- David
> >
> >
> >>   lib/kunit/string-stream-test.c | 26 +++++++++++++++-----------
> >>   lib/kunit/string-stream.c      |  8 ++++----
> >>   lib/kunit/string-stream.h      |  3 ++-
> >>   lib/kunit/test.c               |  2 +-
> >>   4 files changed, 22 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
> >> index 46c2ac162fe8..8a30bb7d5fb7 100644
> >> --- a/lib/kunit/string-stream-test.c
> >> +++ b/lib/kunit/string-stream-test.c
> >> @@ -57,7 +57,7 @@ static void string_stream_line_add_test(struct kunit *test)
> >>          }
> >>          num_lines = i;
> >>
> >> -       concat_string = string_stream_get_string(stream);
> >> +       concat_string = string_stream_get_string(test, stream, GFP_KERNEL);
> >>          KUNIT_EXPECT_NOT_ERR_OR_NULL(test, concat_string);
> >>          KUNIT_EXPECT_EQ(test, strlen(concat_string), total_len);
> >>
> >> @@ -113,7 +113,7 @@ static void string_stream_variable_length_line_test(struct kunit *test)
> >>          }
> >>          num_lines = i;
> >>
> >> -       concat_string = string_stream_get_string(stream);
> >> +       concat_string = string_stream_get_string(test, stream, GFP_KERNEL);
> >>          KUNIT_EXPECT_NOT_ERR_OR_NULL(test, concat_string);
> >>          KUNIT_EXPECT_EQ(test, strlen(concat_string), total_len);
> >>
> >> @@ -165,17 +165,18 @@ static void string_stream_append_test(struct kunit *test)
> >>
> >>          /* Append content of empty stream to empty stream */
> >>          string_stream_append(stream_1, stream_2);
> >> -       KUNIT_EXPECT_EQ(test, strlen(string_stream_get_string(stream_1)), 0);
> >> +       KUNIT_EXPECT_EQ(test, strlen(string_stream_get_string(test, stream_1, GFP_KERNEL)), 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 = string_stream_get_string(stream_1);
> >> +       original_content = string_stream_get_string(test, stream_1, GFP_KERNEL);
> >>
> >>          /* Append content of empty stream to non-empty stream */
> >>          string_stream_append(stream_1, stream_2);
> >> -       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream_1), original_content);
> >> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream_1, GFP_KERNEL),
> >> +                          original_content);
> >>
> >>          /* Add some data to stream_2 */
> >>          for (i = 0; i < ARRAY_SIZE(strings_2); ++i)
> >> @@ -188,14 +189,15 @@ static void string_stream_append_test(struct kunit *test)
> >>           * End result should be the original content of stream_1 plus
> >>           * the content of stream_2.
> >>           */
> >> -       stream_2_content = string_stream_get_string(stream_2);
> >> +       stream_2_content = string_stream_get_string(test, stream_2, GFP_KERNEL);
> >>          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, string_stream_get_string(stream_1), combined_content);
> >> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream_1, GFP_KERNEL),
> >> +                          combined_content);
> >>
> >>          /* Append content of non-empty stream to empty stream */
> >>          string_stream_destroy(stream_1);
> >> @@ -204,7 +206,8 @@ static void string_stream_append_test(struct kunit *test)
> >>          KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
> >>
> >>          string_stream_append(stream_1, stream_2);
> >> -       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream_1), stream_2_content);
> >> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream_1, GFP_KERNEL),
> >> +                          stream_2_content);
> >>   }
> >>
> >>   /* Adding an empty string should not create a fragment. */
> >> @@ -224,7 +227,8 @@ static void string_stream_append_empty_string_test(struct kunit *test)
> >>          string_stream_add(stream, "Add this line");
> >>          string_stream_add(stream, "%s", "");
> >>          KUNIT_EXPECT_EQ(test, list_count_nodes(&stream->fragments), 1);
> >> -       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream), "Add this line");
> >> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream, GFP_KERNEL),
> >> +                          "Add this line");
> >>   }
> >>
> >>   /* Adding strings without automatic newline appending */
> >> @@ -244,7 +248,7 @@ static void string_stream_no_auto_newline_test(struct kunit *test)
> >>          string_stream_add(stream, "Two\n");
> >>          string_stream_add(stream, "%s\n", "Three");
> >>          string_stream_add(stream, "Four");
> >> -       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream),
> >> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream, GFP_KERNEL),
> >>                             "OneTwo\nThree\nFour");
> >>   }
> >>
> >> @@ -271,7 +275,7 @@ static void string_stream_auto_newline_test(struct kunit *test)
> >>          string_stream_add(stream, "Five\n%s", "Six");
> >>          string_stream_add(stream, "Seven\n\n");
> >>          string_stream_add(stream, "Eight");
> >> -       KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream),
> >> +       KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream, GFP_KERNEL),
> >>                             "One\nTwo\nThree\nFour\nFive\nSix\nSeven\n\nEight\n");
> >>   }
> >>
> >> diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
> >> index 1dcf6513b692..eb673d3ea3bd 100644
> >> --- a/lib/kunit/string-stream.c
> >> +++ b/lib/kunit/string-stream.c
> >> @@ -117,13 +117,14 @@ static void string_stream_clear(struct string_stream *stream)
> >>          spin_unlock(&stream->lock);
> >>   }
> >>
> >> -char *string_stream_get_string(struct string_stream *stream)
> >> +char *string_stream_get_string(struct kunit *test, struct string_stream *stream,
> >> +                              gfp_t gfp)
> >>   {
> >>          struct string_stream_fragment *frag_container;
> >>          size_t buf_len = stream->length + 1; /* +1 for null byte. */
> >>          char *buf;
> >>
> >> -       buf = kunit_kzalloc(stream->test, buf_len, stream->gfp);
> >> +       buf = kunit_kzalloc(test, buf_len, gfp);
> >>          if (!buf)
> >>                  return NULL;
> >>
> >> @@ -140,8 +141,7 @@ int string_stream_append(struct string_stream *stream,
> >>   {
> >>          const char *other_content;
> >>
> >> -       other_content = string_stream_get_string(other);
> >> -
> >> +       other_content = string_stream_get_string(other->test, other, other->gfp);
> >>          if (!other_content)
> >>                  return -ENOMEM;
> >>
> >> diff --git a/lib/kunit/string-stream.h b/lib/kunit/string-stream.h
> >> index 048930bf97f0..6b4a747881c5 100644
> >> --- a/lib/kunit/string-stream.h
> >> +++ b/lib/kunit/string-stream.h
> >> @@ -39,7 +39,8 @@ int __printf(2, 0) string_stream_vadd(struct string_stream *stream,
> >>                                        const char *fmt,
> >>                                        va_list args);
> >>
> >> -char *string_stream_get_string(struct string_stream *stream);
> >> +char *string_stream_get_string(struct kunit *test, struct string_stream *stream,
> >> +                              gfp_t gfp);
> >>
> >>   int string_stream_append(struct string_stream *stream,
> >>                           struct string_stream *other);
> >> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> >> index 49698a168437..520e15f49d0d 100644
> >> --- a/lib/kunit/test.c
> >> +++ b/lib/kunit/test.c
> >> @@ -286,7 +286,7 @@ static void kunit_print_string_stream(struct kunit *test,
> >>          if (string_stream_is_empty(stream))
> >>                  return;
> >>
> >> -       buf = string_stream_get_string(stream);
> >> +       buf = string_stream_get_string(test, stream, GFP_KERNEL);
> >>          if (!buf) {
> >>                  kunit_err(test,
> >>                            "Could not allocate buffer, dumping stream:\n");
> >> --
> >> 2.30.2
> >>

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

Re: [PATCH v4 06/10] kunit: string-stream: Pass struct kunit to string_stream_get_string()

[Richard Fitzgerald]

On 17/08/2023 07:26, David Gow wrote:
> On Tue, 15 Aug 2023 at 17:57, Richard Fitzgerald
> <rf@opensource.cirrus.com> wrote:
>>
>> On 15/8/23 10:16, David Gow wrote:
>>> On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald
>>> <rf@opensource.cirrus.com> wrote:
>>>>
>>>> Pass a struct kunit* and gfp_t to string_stream_get_string(). Allocate
>>>> the returned buffer using these instead of using the stream->test and
>>>> stream->gfp.
>>>>
>>>> This is preparation for removing the dependence of string_stream on
>>>> struct kunit, so that string_stream can be used for the debugfs log.
>>>>
>>>> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
>>>> ---
>>>
>>> Makes sense to me.
>>>
>>> I think that, if we weren't going to remove the struct kunit
>>> dependency, we'd want to either keep a version of
>>> string_stream_get_string() which uses it, or, e.g., fall back to it if
>>> the struct passed in is NULL.
>>>
>>
>> That was my first thought. But I thought that was open to subtle
>> accidental bugs in calling code - it might return you a managed
>> allocation, or it might return you an unmanaged allocation that you
>> must free.
>>
>> As there weren't many callers of string_stream_get_string() I decided
>> to go with changing the API to pass in test and gfp like other managed
>> allocations. This makes it more generalized, since the returned buffer
>> is not part of the stream itself, it's a temporary buffer owned by the
>> caller. It also makes it clearer that what you are getting back is
>> likely to be a managed allocation.
>>
>> If you'd prefer to leave string_stream_get_string() as it was, or make
>> it return an unmanged buffer, I can send a new version.
>>
>>> The other option is to have a version which doesn't manage the string
>>> at all, so just takes a gfp and requires the caller to free it (or
>>
>> I didn't add a companion function to get a raw unmanaged string buffer
>> because there's nothing that needs it. It could be added later if
>> it's needed.
>>
> 
> Fair enough.
> 
>>> register an action to do so). If we did that, we could get rid of the
>>> struct kunit pointer totally (though it may be better to keep it and
>>> have both versions).
>>>
>>
>> Another option is to make string_stream_get_string() return a raw
>> buffer and add a kunit_string_stream_geT_string() that returns a
>> managed buffer. This follows some consistency with the normal mallocs
>> where kunit_xxxx() is the managed version.
>>
> 
> Ooh... I like this best. Let's go with that.
> 

I was busy last week with other things and have only got back to looking
at this.

I'm trying to decide what to do with string_stream_get_string() and I'd
value an opinion.

The only use for a test-managed get_string() is the string_stream test.
So I've thought of 4 options:

1) Add a kunit_string_stream_get_string() anyway. But only the test code
uses it.

2) Change the tests to have a local function to do the same thing, and
add an explicit test case for the (unmanaged)
string_stream_get_string() that ensures it's freed.

3) Change the tests to have a local function to get the string, which
calls string_stream_get_string() then copies it to a test-managed buffer
and frees the unmanaged buffer.

4) Implement a kunit_kfree_on_exit() function that takes an already-
allocated buffer and kfree()s it when the test exists, so that we can
do:

    // on success kunit_kfree_on_exit() returns the buffer it was given
    str = kunit_kfree_on_exit(test, string_stream_get_string(stream));
    KUNIT_ASSERT_NOT_ERR_OR_NULL(test, str);

I'm leaning towards (2) but open to going with any of the other options.

Re: [PATCH v4 06/10] kunit: string-stream: Pass struct kunit to string_stream_get_string()

[David Gow]

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

On Tue, 22 Aug 2023 at 00:10, Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> On 17/08/2023 07:26, David Gow wrote:
> > On Tue, 15 Aug 2023 at 17:57, Richard Fitzgerald
> > <rf@opensource.cirrus.com> wrote:
> >>
> >> On 15/8/23 10:16, David Gow wrote:
> >>> On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald
> >>> <rf@opensource.cirrus.com> wrote:
> >>>>
> >>>> Pass a struct kunit* and gfp_t to string_stream_get_string(). Allocate
> >>>> the returned buffer using these instead of using the stream->test and
> >>>> stream->gfp.
> >>>>
> >>>> This is preparation for removing the dependence of string_stream on
> >>>> struct kunit, so that string_stream can be used for the debugfs log.
> >>>>
> >>>> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> >>>> ---
> >>>
> >>> Makes sense to me.
> >>>
> >>> I think that, if we weren't going to remove the struct kunit
> >>> dependency, we'd want to either keep a version of
> >>> string_stream_get_string() which uses it, or, e.g., fall back to it if
> >>> the struct passed in is NULL.
> >>>
> >>
> >> That was my first thought. But I thought that was open to subtle
> >> accidental bugs in calling code - it might return you a managed
> >> allocation, or it might return you an unmanaged allocation that you
> >> must free.
> >>
> >> As there weren't many callers of string_stream_get_string() I decided
> >> to go with changing the API to pass in test and gfp like other managed
> >> allocations. This makes it more generalized, since the returned buffer
> >> is not part of the stream itself, it's a temporary buffer owned by the
> >> caller. It also makes it clearer that what you are getting back is
> >> likely to be a managed allocation.
> >>
> >> If you'd prefer to leave string_stream_get_string() as it was, or make
> >> it return an unmanged buffer, I can send a new version.
> >>
> >>> The other option is to have a version which doesn't manage the string
> >>> at all, so just takes a gfp and requires the caller to free it (or
> >>
> >> I didn't add a companion function to get a raw unmanaged string buffer
> >> because there's nothing that needs it. It could be added later if
> >> it's needed.
> >>
> >
> > Fair enough.
> >
> >>> register an action to do so). If we did that, we could get rid of the
> >>> struct kunit pointer totally (though it may be better to keep it and
> >>> have both versions).
> >>>
> >>
> >> Another option is to make string_stream_get_string() return a raw
> >> buffer and add a kunit_string_stream_geT_string() that returns a
> >> managed buffer. This follows some consistency with the normal mallocs
> >> where kunit_xxxx() is the managed version.
> >>
> >
> > Ooh... I like this best. Let's go with that.
> >
>
> I was busy last week with other things and have only got back to looking
> at this.
>
> I'm trying to decide what to do with string_stream_get_string() and I'd
> value an opinion.
>
> The only use for a test-managed get_string() is the string_stream test.
> So I've thought of 4 options:
>
> 1) Add a kunit_string_stream_get_string() anyway. But only the test code
> uses it.
>
> 2) Change the tests to have a local function to do the same thing, and
> add an explicit test case for the (unmanaged)
> string_stream_get_string() that ensures it's freed.
>
> 3) Change the tests to have a local function to get the string, which
> calls string_stream_get_string() then copies it to a test-managed buffer
> and frees the unmanaged buffer.
>
> 4) Implement a kunit_kfree_on_exit() function that takes an already-
> allocated buffer and kfree()s it when the test exists, so that we can
> do:
>
>     // on success kunit_kfree_on_exit() returns the buffer it was given
>     str = kunit_kfree_on_exit(test, string_stream_get_string(stream));
>     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, str);
>
> I'm leaning towards (2) but open to going with any of the other options.

I'd be happy with any of those options (though 3 is my least favourite).

There is already a kfree_at_end() function in the executor test. It's
not quite as nice as your proposed kunit_kfree_on_exit() function (I
like this idea of passing the pointer through), but it proves the
concept. I think your version of this would be a useful thing to have
as an actual part of the KUnit API, rather than just a per-test hack:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/kunit/executor_test.c#n131

Cheers,
-- David

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