From 5c54c9ebb1f4758a4da7731a809148ff1a3a1844 Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Mon, 28 Aug 2023 11:41:02 +0100 Subject: kunit: string-stream: Don't create a fragment for empty strings If the result of the formatted string is an empty string just return instead of creating an empty fragment. Signed-off-by: Richard Fitzgerald Reviewed-by: Rae Moar Reviewed-by: David Gow Signed-off-by: Shuah Khan --- lib/kunit/string-stream.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) (limited to 'lib') 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); -- cgit v1.2.3 From 4551caca6ab67fb0b5199ca43580c4f8d27bf28a Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Mon, 28 Aug 2023 11:41:03 +0100 Subject: kunit: string-stream: Improve testing of string_stream Replace the minimal tests with more-thorough testing. string_stream_init_test() tests that struct string_stream is initialized correctly. string_stream_line_add_test() adds a series of numbered lines and checks that the resulting string contains all the lines. string_stream_variable_length_line_test() adds a large number of lines of varying length to create many fragments, then tests that all lines are present. string_stream_append_test() tests various cases of using string_stream_append() to append the content of one stream to another. Adds string_stream_append_empty_string_test() to test that adding an empty string to a string_stream doesn't create a new empty fragment. Signed-off-by: Richard Fitzgerald Reviewed-by: Rae Moar Reviewed-by: David Gow Signed-off-by: Shuah Khan --- lib/kunit/string-stream-test.c | 233 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 217 insertions(+), 16 deletions(-) (limited to 'lib') diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c index 110f3a993250..7e17307ca78c 100644 --- a/lib/kunit/string-stream-test.c +++ b/lib/kunit/string-stream-test.c @@ -11,38 +11,239 @@ #include "string-stream.h" -static void string_stream_test_empty_on_creation(struct kunit *test) +static char *get_concatenated_string(struct kunit *test, struct string_stream *stream) { - struct string_stream *stream = alloc_string_stream(test, GFP_KERNEL); + char *str = string_stream_get_string(stream); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, str); + + return str; +} + +/* string_stream object is initialized correctly. */ +static void string_stream_init_test(struct kunit *test) +{ + struct string_stream *stream; + + stream = alloc_string_stream(test, GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream); + + KUNIT_EXPECT_EQ(test, stream->length, 0); + KUNIT_EXPECT_TRUE(test, list_empty(&stream->fragments)); + KUNIT_EXPECT_PTR_EQ(test, stream->test, test); + KUNIT_EXPECT_TRUE(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; + char line[60]; + char *concat_string, *pos, *string_end; + size_t len, total_len; + int num_lines, i; + + stream = alloc_string_stream(test, GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream); + + /* Add series of sequence numbered lines */ + total_len = 0; + for (i = 0; i < 100; ++i) { + len = snprintf(line, sizeof(line), + "The quick brown fox jumps over the lazy penguin %d\n", i); + + /* Sanity-check that our test string isn't truncated */ + KUNIT_ASSERT_LT(test, len, sizeof(line)); + + string_stream_add(stream, line); + total_len += len; + } + num_lines = i; + + concat_string = get_concatenated_string(test, stream); + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, concat_string); + KUNIT_EXPECT_EQ(test, strlen(concat_string), total_len); + + /* + * Split the concatenated string at the newlines and check that + * all the original added strings are present. + */ + pos = concat_string; + for (i = 0; i < num_lines; ++i) { + string_end = strchr(pos, '\n'); + KUNIT_EXPECT_NOT_NULL(test, string_end); + + /* Convert to NULL-terminated string */ + *string_end = '\0'; + + snprintf(line, sizeof(line), + "The quick brown fox jumps over the lazy penguin %d", i); + KUNIT_EXPECT_STREQ(test, pos, line); + + pos = string_end + 1; + } + + /* There shouldn't be any more data after this */ + KUNIT_EXPECT_EQ(test, strlen(pos), 0); +} + +/* Add a series of lines of variable length to a string_stream. */ +static void string_stream_variable_length_line_test(struct kunit *test) +{ + 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; + + stream = alloc_string_stream(test, GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream); + + /* + * Log many lines of varying lengths until we have created + * many fragments. + * The "randomness" must be repeatable. + */ + prandom_seed_state(&rnd, 3141592653589793238ULL); + total_len = 0; + for (i = 0; i < 100; ++i) { + offset = prandom_u32_state(&rnd) % (sizeof(line) - 1); + string_stream_add(stream, "%s\n", &line[offset]); + total_len += sizeof(line) - offset; + } + num_lines = i; + + concat_string = get_concatenated_string(test, stream); + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, concat_string); + KUNIT_EXPECT_EQ(test, strlen(concat_string), total_len); + + /* + * Split the concatenated string at the newlines and check that + * all the original added strings are present. + */ + prandom_seed_state(&rnd, 3141592653589793238ULL); + pos = concat_string; + for (i = 0; i < num_lines; ++i) { + string_end = strchr(pos, '\n'); + KUNIT_EXPECT_NOT_NULL(test, string_end); + + /* Convert to NULL-terminated string */ + *string_end = '\0'; + + offset = prandom_u32_state(&rnd) % (sizeof(line) - 1); + KUNIT_EXPECT_STREQ(test, pos, &line[offset]); + + pos = string_end + 1; + } + + /* There shouldn't be any more data after this */ + KUNIT_EXPECT_EQ(test, strlen(pos), 0); +} + +/* Appending the content of one string stream to another. */ +static void string_stream_append_test(struct kunit *test) { - struct string_stream *stream = alloc_string_stream(test, GFP_KERNEL); + static const char * const strings_1[] = { + "one", "two", "three", "four", "five", "six", + "seven", "eight", "nine", "ten", + }; + static const char * const strings_2[] = { + "Apple", "Pear", "Orange", "Banana", "Grape", "Apricot", + }; + struct string_stream *stream_1, *stream_2; + const char *stream1_content_before_append, *stream_2_content; + char *combined_content; + size_t combined_length; + int i; + + stream_1 = alloc_string_stream(test, GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1); + + stream_2 = alloc_string_stream(test, GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_2); + + /* Append content of empty stream to empty stream */ + string_stream_append(stream_1, stream_2); + KUNIT_EXPECT_EQ(test, strlen(get_concatenated_string(test, stream_1)), 0); - string_stream_add(stream, "Foo"); + /* 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]); - KUNIT_EXPECT_FALSE(test, string_stream_is_empty(stream)); + stream1_content_before_append = get_concatenated_string(test, stream_1); + + /* Append content of empty stream to non-empty stream */ + string_stream_append(stream_1, stream_2); + KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1), + stream1_content_before_append); + + /* Add some data to stream_2 */ + for (i = 0; i < ARRAY_SIZE(strings_2); ++i) + string_stream_add(stream_2, "%s\n", strings_2[i]); + + /* Append content of non-empty stream to non-empty stream */ + string_stream_append(stream_1, stream_2); + + /* + * End result should be the original content of stream_1 plus + * the content of stream_2. + */ + stream_2_content = get_concatenated_string(test, stream_2); + combined_length = strlen(stream1_content_before_append) + 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", + stream1_content_before_append, stream_2_content); + + KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1), combined_content); + + /* Append content of non-empty stream to empty stream */ + string_stream_destroy(stream_1); + + stream_1 = alloc_string_stream(test, GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1); + + string_stream_append(stream_1, stream_2); + KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1), stream_2_content); } -static void string_stream_test_get_string(struct kunit *test) +/* Adding an empty string should not create a fragment. */ +static void string_stream_append_empty_string_test(struct kunit *test) { - struct string_stream *stream = alloc_string_stream(test, GFP_KERNEL); - char *output; + struct string_stream *stream; + int original_frag_count; + + stream = alloc_string_stream(test, GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream); + + /* Formatted empty string */ + string_stream_add(stream, "%s", ""); + KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream)); + KUNIT_EXPECT_TRUE(test, list_empty(&stream->fragments)); - string_stream_add(stream, "Foo"); - string_stream_add(stream, " %s", "bar"); + /* Adding an empty string to a non-empty stream */ + string_stream_add(stream, "Add this line"); + original_frag_count = list_count_nodes(&stream->fragments); - output = string_stream_get_string(stream); - KUNIT_ASSERT_STREQ(test, output, "Foo bar"); + string_stream_add(stream, "%s", ""); + KUNIT_EXPECT_EQ(test, list_count_nodes(&stream->fragments), original_frag_count); + KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream), "Add this line"); } static struct kunit_case string_stream_test_cases[] = { - KUNIT_CASE(string_stream_test_empty_on_creation), - KUNIT_CASE(string_stream_test_not_empty_after_add), - KUNIT_CASE(string_stream_test_get_string), + KUNIT_CASE(string_stream_init_test), + KUNIT_CASE(string_stream_line_add_test), + KUNIT_CASE(string_stream_variable_length_line_test), + KUNIT_CASE(string_stream_append_test), + KUNIT_CASE(string_stream_append_empty_string_test), {} }; -- cgit v1.2.3 From a5abe7b201779b0000f1e8ab522e5c6fc0c413bf Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Mon, 28 Aug 2023 11:41:04 +0100 Subject: kunit: string-stream: Add option to make all lines end with newline 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 Reviewed-by: Rae Moar Reviewed-by: David Gow Signed-off-by: Shuah Khan --- lib/kunit/string-stream.c | 28 +++++++++++++++++++++------- lib/kunit/string-stream.h | 7 +++++++ 2 files changed, 28 insertions(+), 7 deletions(-) (limited to 'lib') 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 */ -- cgit v1.2.3 From 1f58cdb173a4ce1bc670fa4662af0fe3556253cf Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Mon, 28 Aug 2023 11:41:05 +0100 Subject: kunit: string-stream-test: Add cases for string_stream newline appending Add test cases for testing the string_stream feature that appends a newline to strings that do not already end with a newline. string_stream_no_auto_newline_test() tests with this feature disabled. Newlines should not be added or dropped. string_stream_auto_newline_test() tests with this feature enabled. Newlines should be added to lines that do not end with a newline. string_stream_append_auto_newline_test() tests appending the content of one stream to another stream when the target stream has newline appending enabled. Signed-off-by: Richard Fitzgerald Reviewed-by: Rae Moar Reviewed-by: David Gow Signed-off-by: Shuah Khan --- lib/kunit/string-stream-test.c | 93 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) (limited to 'lib') diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c index 7e17307ca78c..f117c4b7389b 100644 --- a/lib/kunit/string-stream-test.c +++ b/lib/kunit/string-stream-test.c @@ -32,6 +32,7 @@ static void string_stream_init_test(struct kunit *test) KUNIT_EXPECT_TRUE(test, list_empty(&stream->fragments)); KUNIT_EXPECT_PTR_EQ(test, stream->test, test); KUNIT_EXPECT_TRUE(test, (stream->gfp == GFP_KERNEL)); + KUNIT_EXPECT_FALSE(test, stream->append_newlines); KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream)); } @@ -215,6 +216,45 @@ static void string_stream_append_test(struct kunit *test) KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1), stream_2_content); } +/* Appending the content of one string stream to one with auto-newlining. */ +static void string_stream_append_auto_newline_test(struct kunit *test) +{ + struct string_stream *stream_1, *stream_2; + + /* Stream 1 has newline appending enabled */ + stream_1 = alloc_string_stream(test, GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1); + string_stream_set_append_newlines(stream_1, true); + KUNIT_EXPECT_TRUE(test, stream_1->append_newlines); + + /* Stream 2 does not append newlines */ + stream_2 = alloc_string_stream(test, GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_2); + + /* Appending a stream with a newline should not add another newline */ + string_stream_add(stream_1, "Original string\n"); + string_stream_add(stream_2, "Appended content\n"); + string_stream_add(stream_2, "More stuff\n"); + string_stream_append(stream_1, stream_2); + KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1), + "Original string\nAppended content\nMore stuff\n"); + + string_stream_destroy(stream_2); + stream_2 = alloc_string_stream(test, GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_2); + + /* + * Appending a stream without newline should add a final newline. + * The appended string_stream is treated as a single string so newlines + * should not be inserted between fragments. + */ + string_stream_add(stream_2, "Another"); + string_stream_add(stream_2, "And again"); + string_stream_append(stream_1, stream_2); + KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1), + "Original string\nAppended content\nMore stuff\nAnotherAnd again\n"); +} + /* Adding an empty string should not create a fragment. */ static void string_stream_append_empty_string_test(struct kunit *test) { @@ -238,12 +278,65 @@ static void string_stream_append_empty_string_test(struct kunit *test) KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream), "Add this line"); } +/* Adding strings without automatic newline appending */ +static void string_stream_no_auto_newline_test(struct kunit *test) +{ + struct string_stream *stream; + + stream = alloc_string_stream(test, GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream); + + /* + * Add some strings with and without newlines. All formatted newlines + * should be preserved. It should not add any extra newlines. + */ + string_stream_add(stream, "One"); + string_stream_add(stream, "Two\n"); + string_stream_add(stream, "%s\n", "Three"); + string_stream_add(stream, "%s", "Four\n"); + string_stream_add(stream, "Five\n%s", "Six"); + string_stream_add(stream, "Seven\n\n"); + string_stream_add(stream, "Eight"); + KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream), + "OneTwo\nThree\nFour\nFive\nSixSeven\n\nEight"); +} + +/* Adding strings with automatic newline appending */ +static void string_stream_auto_newline_test(struct kunit *test) +{ + struct string_stream *stream; + + stream = alloc_string_stream(test, GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream); + + string_stream_set_append_newlines(stream, true); + KUNIT_EXPECT_TRUE(test, stream->append_newlines); + + /* + * Add some strings with and without newlines. Newlines should + * be appended to lines that do not end with \n, but newlines + * resulting from the formatting should not be changed. + */ + string_stream_add(stream, "One"); + string_stream_add(stream, "Two\n"); + string_stream_add(stream, "%s\n", "Three"); + string_stream_add(stream, "%s", "Four\n"); + string_stream_add(stream, "Five\n%s", "Six"); + string_stream_add(stream, "Seven\n\n"); + string_stream_add(stream, "Eight"); + KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream), + "One\nTwo\nThree\nFour\nFive\nSix\nSeven\n\nEight\n"); +} + static struct kunit_case string_stream_test_cases[] = { KUNIT_CASE(string_stream_init_test), KUNIT_CASE(string_stream_line_add_test), KUNIT_CASE(string_stream_variable_length_line_test), KUNIT_CASE(string_stream_append_test), + KUNIT_CASE(string_stream_append_auto_newline_test), KUNIT_CASE(string_stream_append_empty_string_test), + KUNIT_CASE(string_stream_no_auto_newline_test), + KUNIT_CASE(string_stream_auto_newline_test), {} }; -- cgit v1.2.3 From 7b4481cbe7e6bde275aa5e5f2aaa21455915e07c Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Mon, 28 Aug 2023 11:41:06 +0100 Subject: kunit: Don't use a managed alloc in is_literal() There is no need to use a test-managed alloc in is_literal(). The function frees the temporary buffer before returning. This removes the only use of the test and gfp members of struct string_stream outside of the string_stream implementation. Signed-off-by: Richard Fitzgerald Reviewed-by: David Gow Signed-off-by: Shuah Khan --- lib/kunit/assert.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) (limited to 'lib') diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c index 05a09652f5a1..dd1d633d0fe2 100644 --- a/lib/kunit/assert.c +++ b/lib/kunit/assert.c @@ -89,8 +89,7 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert, EXPORT_SYMBOL_GPL(kunit_ptr_not_err_assert_format); /* Checks if `text` is a literal representing `value`, e.g. "5" and 5 */ -static bool is_literal(struct kunit *test, const char *text, long long value, - gfp_t gfp) +static bool is_literal(const char *text, long long value) { char *buffer; int len; @@ -100,14 +99,15 @@ static bool is_literal(struct kunit *test, const char *text, long long value, if (strlen(text) != len) return false; - buffer = kunit_kmalloc(test, len+1, gfp); + buffer = kmalloc(len+1, GFP_KERNEL); if (!buffer) return false; snprintf(buffer, len+1, "%lld", value); ret = strncmp(buffer, text, len) == 0; - kunit_kfree(test, buffer); + kfree(buffer); + return ret; } @@ -125,14 +125,12 @@ void kunit_binary_assert_format(const struct kunit_assert *assert, binary_assert->text->left_text, binary_assert->text->operation, binary_assert->text->right_text); - if (!is_literal(stream->test, binary_assert->text->left_text, - binary_assert->left_value, stream->gfp)) + if (!is_literal(binary_assert->text->left_text, binary_assert->left_value)) string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld (0x%llx)\n", binary_assert->text->left_text, binary_assert->left_value, binary_assert->left_value); - if (!is_literal(stream->test, binary_assert->text->right_text, - binary_assert->right_value, stream->gfp)) + if (!is_literal(binary_assert->text->right_text, binary_assert->right_value)) string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld (0x%llx)", binary_assert->text->right_text, binary_assert->right_value, -- cgit v1.2.3 From 20631e154c78f4140fffe111f5c79464fae3c38c Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Mon, 28 Aug 2023 11:41:07 +0100 Subject: kunit: string-stream: Add kunit_alloc_string_stream() Add function kunit_alloc_string_stream() to do a resource-managed allocation of a string stream, and corresponding kunit_free_string_stream() to free the resource-managed stream. This is preparing for decoupling the string_stream implementation from struct kunit, to reduce the amount of code churn when that happens. Currently: - kunit_alloc_string_stream() only calls alloc_string_stream(). - kunit_free_string_stream() takes a struct kunit* which isn't used yet. Callers of the old alloc_string_stream() and string_stream_destroy() are all requesting a managed allocation so have been changed to use the new functions. alloc_string_stream() has been temporarily made static because its current behavior has been replaced with kunit_alloc_string_stream(). Signed-off-by: Richard Fitzgerald Reviewed-by: David Gow Signed-off-by: Shuah Khan --- lib/kunit/string-stream-test.c | 28 ++++++++++++++-------------- lib/kunit/string-stream.c | 12 +++++++++++- lib/kunit/string-stream.h | 3 ++- lib/kunit/test.c | 4 ++-- 4 files changed, 29 insertions(+), 18 deletions(-) (limited to 'lib') diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c index f117c4b7389b..46b18e940b73 100644 --- a/lib/kunit/string-stream-test.c +++ b/lib/kunit/string-stream-test.c @@ -25,7 +25,7 @@ static void string_stream_init_test(struct kunit *test) { struct string_stream *stream; - stream = alloc_string_stream(test, GFP_KERNEL); + stream = kunit_alloc_string_stream(test, GFP_KERNEL); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream); KUNIT_EXPECT_EQ(test, stream->length, 0); @@ -48,7 +48,7 @@ static void string_stream_line_add_test(struct kunit *test) size_t len, total_len; int num_lines, i; - stream = alloc_string_stream(test, GFP_KERNEL); + stream = kunit_alloc_string_stream(test, GFP_KERNEL); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream); /* Add series of sequence numbered lines */ @@ -104,7 +104,7 @@ static void string_stream_variable_length_line_test(struct kunit *test) size_t offset, total_len; int num_lines, i; - stream = alloc_string_stream(test, GFP_KERNEL); + stream = kunit_alloc_string_stream(test, GFP_KERNEL); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream); /* @@ -164,10 +164,10 @@ static void string_stream_append_test(struct kunit *test) size_t combined_length; int i; - stream_1 = alloc_string_stream(test, GFP_KERNEL); + stream_1 = kunit_alloc_string_stream(test, GFP_KERNEL); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1); - stream_2 = alloc_string_stream(test, GFP_KERNEL); + stream_2 = kunit_alloc_string_stream(test, GFP_KERNEL); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_2); /* Append content of empty stream to empty stream */ @@ -207,9 +207,9 @@ static void string_stream_append_test(struct kunit *test) KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1), combined_content); /* Append content of non-empty stream to empty stream */ - string_stream_destroy(stream_1); + kunit_free_string_stream(test, stream_1); - stream_1 = alloc_string_stream(test, GFP_KERNEL); + stream_1 = kunit_alloc_string_stream(test, GFP_KERNEL); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1); string_stream_append(stream_1, stream_2); @@ -222,13 +222,13 @@ static void string_stream_append_auto_newline_test(struct kunit *test) struct string_stream *stream_1, *stream_2; /* Stream 1 has newline appending enabled */ - stream_1 = alloc_string_stream(test, GFP_KERNEL); + stream_1 = kunit_alloc_string_stream(test, GFP_KERNEL); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1); string_stream_set_append_newlines(stream_1, true); KUNIT_EXPECT_TRUE(test, stream_1->append_newlines); /* Stream 2 does not append newlines */ - stream_2 = alloc_string_stream(test, GFP_KERNEL); + stream_2 = kunit_alloc_string_stream(test, GFP_KERNEL); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_2); /* Appending a stream with a newline should not add another newline */ @@ -239,8 +239,8 @@ static void string_stream_append_auto_newline_test(struct kunit *test) KUNIT_EXPECT_STREQ(test, get_concatenated_string(test, stream_1), "Original string\nAppended content\nMore stuff\n"); - string_stream_destroy(stream_2); - stream_2 = alloc_string_stream(test, GFP_KERNEL); + kunit_free_string_stream(test, stream_2); + stream_2 = kunit_alloc_string_stream(test, GFP_KERNEL); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_2); /* @@ -261,7 +261,7 @@ static void string_stream_append_empty_string_test(struct kunit *test) struct string_stream *stream; int original_frag_count; - stream = alloc_string_stream(test, GFP_KERNEL); + stream = kunit_alloc_string_stream(test, GFP_KERNEL); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream); /* Formatted empty string */ @@ -283,7 +283,7 @@ static void string_stream_no_auto_newline_test(struct kunit *test) { struct string_stream *stream; - stream = alloc_string_stream(test, GFP_KERNEL); + stream = kunit_alloc_string_stream(test, GFP_KERNEL); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream); /* @@ -306,7 +306,7 @@ static void string_stream_auto_newline_test(struct kunit *test) { struct string_stream *stream; - stream = alloc_string_stream(test, GFP_KERNEL); + stream = kunit_alloc_string_stream(test, GFP_KERNEL); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream); string_stream_set_append_newlines(stream, true); diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c index 1dcf6513b692..12ecf15e1f6b 100644 --- a/lib/kunit/string-stream.c +++ b/lib/kunit/string-stream.c @@ -153,7 +153,7 @@ bool string_stream_is_empty(struct string_stream *stream) return list_empty(&stream->fragments); } -struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp) +static struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp) { struct string_stream *stream; @@ -173,3 +173,13 @@ void string_stream_destroy(struct string_stream *stream) { string_stream_clear(stream); } + +struct string_stream *kunit_alloc_string_stream(struct kunit *test, gfp_t gfp) +{ + return alloc_string_stream(test, gfp); +} + +void kunit_free_string_stream(struct kunit *test, struct string_stream *stream) +{ + string_stream_destroy(stream); +} diff --git a/lib/kunit/string-stream.h b/lib/kunit/string-stream.h index 048930bf97f0..3e70ee9d66e9 100644 --- a/lib/kunit/string-stream.h +++ b/lib/kunit/string-stream.h @@ -30,7 +30,8 @@ struct string_stream { struct kunit; -struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp); +struct string_stream *kunit_alloc_string_stream(struct kunit *test, gfp_t gfp); +void kunit_free_string_stream(struct kunit *test, struct string_stream *stream); int __printf(2, 3) string_stream_add(struct string_stream *stream, const char *fmt, ...); diff --git a/lib/kunit/test.c b/lib/kunit/test.c index 421f13981412..30807694c459 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -308,7 +308,7 @@ static void kunit_fail(struct kunit *test, const struct kunit_loc *loc, kunit_set_failure(test); - stream = alloc_string_stream(test, GFP_KERNEL); + stream = kunit_alloc_string_stream(test, GFP_KERNEL); if (IS_ERR(stream)) { WARN(true, "Could not allocate stream to print failed assertion in %s:%d\n", @@ -322,7 +322,7 @@ static void kunit_fail(struct kunit *test, const struct kunit_loc *loc, kunit_print_string_stream(test, stream); - string_stream_destroy(stream); + kunit_free_string_stream(test, stream); } void __noreturn __kunit_abort(struct kunit *test) -- cgit v1.2.3 From a3fdf784780ccb0008d630e8722d1389c49c7499 Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Mon, 28 Aug 2023 11:41:08 +0100 Subject: kunit: string-stream: Decouple string_stream from kunit Re-work string_stream so that it is not tied to a struct kunit. This is to allow using it for the log of struct kunit_suite. Instead of resource-managing individual allocations the whole string_stream can be resource-managed, if required. alloc_string_stream() now allocates a string stream that is not resource-managed. string_stream_destroy() now works on an unmanaged string_stream allocated by alloc_string_stream() and frees the entire string_stream (previously it only freed the fragments). string_stream_clear() has been made public for callers that want to free the fragments without destroying the string_stream. For resource-managed allocations use kunit_alloc_string_stream() and kunit_free_string_stream(). In addition to this, string_stream_get_string() now returns an unmanaged buffer that the caller must kfree(). Signed-off-by: Richard Fitzgerald Reviewed-by: David Gow Signed-off-by: Shuah Khan --- lib/kunit/string-stream-test.c | 8 +++++- lib/kunit/string-stream.c | 61 +++++++++++++++++++++++++++--------------- lib/kunit/string-stream.h | 6 ++++- lib/kunit/test.c | 2 +- 4 files changed, 53 insertions(+), 24 deletions(-) (limited to 'lib') diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c index 46b18e940b73..58ba1ef5207f 100644 --- a/lib/kunit/string-stream-test.c +++ b/lib/kunit/string-stream-test.c @@ -11,11 +11,18 @@ #include "string-stream.h" +/* This avoids a cast warning if kfree() is passed direct to kunit_add_action(). */ +static void kfree_wrapper(void *p) +{ + kfree(p); +} + static char *get_concatenated_string(struct kunit *test, struct string_stream *stream) { char *str = string_stream_get_string(stream); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, str); + kunit_add_action(test, kfree_wrapper, (void *)str); return str; } @@ -30,7 +37,6 @@ static void string_stream_init_test(struct kunit *test) KUNIT_EXPECT_EQ(test, stream->length, 0); KUNIT_EXPECT_TRUE(test, list_empty(&stream->fragments)); - KUNIT_EXPECT_PTR_EQ(test, stream->test, test); KUNIT_EXPECT_TRUE(test, (stream->gfp == GFP_KERNEL)); KUNIT_EXPECT_FALSE(test, stream->append_newlines); KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream)); diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c index 12ecf15e1f6b..64abceb7b716 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,7 +107,7 @@ static void string_stream_clear(struct string_stream *stream) frag_container_safe, &stream->fragments, node) { - string_stream_fragment_destroy(stream->test, frag_container); + string_stream_fragment_destroy(frag_container); } stream->length = 0; spin_unlock(&stream->lock); @@ -123,7 +119,7 @@ char *string_stream_get_string(struct string_stream *stream) size_t buf_len = stream->length + 1; /* +1 for null byte. */ char *buf; - buf = kunit_kzalloc(stream->test, buf_len, stream->gfp); + buf = kzalloc(buf_len, stream->gfp); if (!buf) return NULL; @@ -139,13 +135,17 @@ int string_stream_append(struct string_stream *stream, struct string_stream *other) { const char *other_content; + int ret; other_content = string_stream_get_string(other); if (!other_content) return -ENOMEM; - return string_stream_add(stream, other_content); + ret = string_stream_add(stream, other_content); + kfree(other_content); + + return ret; } bool string_stream_is_empty(struct string_stream *stream) @@ -153,16 +153,15 @@ bool string_stream_is_empty(struct string_stream *stream) return list_empty(&stream->fragments); } -static struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp) +struct string_stream *alloc_string_stream(gfp_t gfp) { struct string_stream *stream; - stream = kunit_kzalloc(test, sizeof(*stream), gfp); + stream = kzalloc(sizeof(*stream), gfp); if (!stream) return ERR_PTR(-ENOMEM); stream->gfp = gfp; - stream->test = test; INIT_LIST_HEAD(&stream->fragments); spin_lock_init(&stream->lock); @@ -171,15 +170,35 @@ static struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp) void string_stream_destroy(struct string_stream *stream) { + if (!stream) + return; + string_stream_clear(stream); + kfree(stream); +} + +static void resource_free_string_stream(void *p) +{ + struct string_stream *stream = p; + + string_stream_destroy(stream); } struct string_stream *kunit_alloc_string_stream(struct kunit *test, gfp_t gfp) { - return alloc_string_stream(test, gfp); + struct string_stream *stream; + + stream = alloc_string_stream(gfp); + if (IS_ERR(stream)) + return stream; + + if (kunit_add_action_or_reset(test, resource_free_string_stream, stream) != 0) + return ERR_PTR(-ENOMEM); + + return stream; } void kunit_free_string_stream(struct kunit *test, struct string_stream *stream) { - string_stream_destroy(stream); + kunit_release_action(test, resource_free_string_stream, (void *)stream); } diff --git a/lib/kunit/string-stream.h b/lib/kunit/string-stream.h index 3e70ee9d66e9..7be2450c7079 100644 --- a/lib/kunit/string-stream.h +++ b/lib/kunit/string-stream.h @@ -23,7 +23,6 @@ struct string_stream { struct list_head fragments; /* length and fragments are protected by this lock */ spinlock_t lock; - struct kunit *test; gfp_t gfp; bool append_newlines; }; @@ -33,6 +32,9 @@ struct kunit; struct string_stream *kunit_alloc_string_stream(struct kunit *test, gfp_t gfp); void kunit_free_string_stream(struct kunit *test, struct string_stream *stream); +struct string_stream *alloc_string_stream(gfp_t gfp); +void free_string_stream(struct string_stream *stream); + int __printf(2, 3) string_stream_add(struct string_stream *stream, const char *fmt, ...); @@ -40,6 +42,8 @@ int __printf(2, 0) string_stream_vadd(struct string_stream *stream, const char *fmt, va_list args); +void string_stream_clear(struct string_stream *stream); + char *string_stream_get_string(struct string_stream *stream); int string_stream_append(struct string_stream *stream, diff --git a/lib/kunit/test.c b/lib/kunit/test.c index 30807694c459..bd1ef86fcd71 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -296,7 +296,7 @@ static void kunit_print_string_stream(struct kunit *test, kunit_err(test, "\n"); } else { kunit_err(test, "%s", buf); - kunit_kfree(test, buf); + kfree(buf); } } -- cgit v1.2.3 From d1a0d699bfc00ae5b5e74bb640d791a93e825b68 Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Mon, 28 Aug 2023 11:41:09 +0100 Subject: kunit: string-stream: Add tests for freeing resource-managed string_stream string_stream_managed_free_test() allocates a resource-managed string_stream and tests that kunit_free_string_stream() calls string_stream_destroy(). string_stream_resource_free_test() allocates a resource-managed string_stream and tests that string_stream_destroy() is called when the test resources are cleaned up. The old string_stream_init_test() has been split into two tests, one for kunit_alloc_string_stream() and the other for alloc_string_stream(). Signed-off-by: Richard Fitzgerald Reviewed-by: David Gow Signed-off-by: Shuah Khan --- lib/kunit/string-stream-test.c | 147 +++++++++++++++++++++++++++++++++++++++-- lib/kunit/string-stream.c | 3 + 2 files changed, 145 insertions(+), 5 deletions(-) (limited to 'lib') diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c index 58ba1ef5207f..b759974d9cec 100644 --- a/lib/kunit/string-stream-test.c +++ b/lib/kunit/string-stream-test.c @@ -6,17 +6,33 @@ * Author: Brendan Higgins */ +#include #include #include #include "string-stream.h" -/* This avoids a cast warning if kfree() is passed direct to kunit_add_action(). */ +struct string_stream_test_priv { + /* For testing resource-managed free. */ + struct string_stream *expected_free_stream; + bool stream_was_freed; + bool stream_free_again; +}; + +/* Avoids a cast warning if kfree() is passed direct to kunit_add_action(). */ static void kfree_wrapper(void *p) { kfree(p); } +/* Avoids a cast warning if string_stream_destroy() is passed direct to kunit_add_action(). */ +static void cleanup_raw_stream(void *p) +{ + struct string_stream *stream = p; + + string_stream_destroy(stream); +} + static char *get_concatenated_string(struct kunit *test, struct string_stream *stream) { char *str = string_stream_get_string(stream); @@ -27,11 +43,12 @@ static char *get_concatenated_string(struct kunit *test, struct string_stream *s return str; } -/* string_stream object is initialized correctly. */ -static void string_stream_init_test(struct kunit *test) +/* Managed string_stream object is initialized correctly. */ +static void string_stream_managed_init_test(struct kunit *test) { struct string_stream *stream; + /* Resource-managed initialization. */ stream = kunit_alloc_string_stream(test, GFP_KERNEL); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream); @@ -42,6 +59,109 @@ static void string_stream_init_test(struct kunit *test) KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream)); } +/* Unmanaged string_stream object is initialized correctly. */ +static void string_stream_unmanaged_init_test(struct kunit *test) +{ + struct string_stream *stream; + + stream = alloc_string_stream(GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream); + kunit_add_action(test, cleanup_raw_stream, stream); + + KUNIT_EXPECT_EQ(test, stream->length, 0); + KUNIT_EXPECT_TRUE(test, list_empty(&stream->fragments)); + KUNIT_EXPECT_EQ(test, stream->gfp, GFP_KERNEL); + KUNIT_EXPECT_FALSE(test, stream->append_newlines); + + KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream)); +} + +static void string_stream_destroy_stub(struct string_stream *stream) +{ + struct kunit *fake_test = kunit_get_current_test(); + struct string_stream_test_priv *priv = fake_test->priv; + + /* The kunit could own string_streams other than the one we are testing. */ + if (stream == priv->expected_free_stream) { + if (priv->stream_was_freed) + priv->stream_free_again = true; + else + priv->stream_was_freed = true; + } + + /* + * Calling string_stream_destroy() will only call this function again + * because the redirection stub is still active. + * Avoid calling deactivate_static_stub() or changing current->kunit_test + * during cleanup. + */ + string_stream_clear(stream); + kfree(stream); +} + +/* kunit_free_string_stream() calls string_stream_desrtoy() */ +static void string_stream_managed_free_test(struct kunit *test) +{ + struct string_stream_test_priv *priv = test->priv; + + priv->expected_free_stream = NULL; + priv->stream_was_freed = false; + priv->stream_free_again = false; + + kunit_activate_static_stub(test, + string_stream_destroy, + string_stream_destroy_stub); + + priv->expected_free_stream = kunit_alloc_string_stream(test, GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->expected_free_stream); + + /* This should call the stub function. */ + kunit_free_string_stream(test, priv->expected_free_stream); + + KUNIT_EXPECT_TRUE(test, priv->stream_was_freed); + KUNIT_EXPECT_FALSE(test, priv->stream_free_again); +} + +/* string_stream object is freed when test is cleaned up. */ +static void string_stream_resource_free_test(struct kunit *test) +{ + struct string_stream_test_priv *priv = test->priv; + struct kunit *fake_test; + + 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->expected_free_stream = NULL; + priv->stream_was_freed = false; + priv->stream_free_again = false; + + kunit_activate_static_stub(fake_test, + string_stream_destroy, + string_stream_destroy_stub); + + priv->expected_free_stream = kunit_alloc_string_stream(fake_test, GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->expected_free_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_TRUE(test, priv->stream_was_freed); + KUNIT_EXPECT_FALSE(test, priv->stream_free_again); +} + /* * Add a series of lines to a string_stream. Check that all lines * appear in the correct order and no characters are dropped. @@ -334,8 +454,24 @@ static void string_stream_auto_newline_test(struct kunit *test) "One\nTwo\nThree\nFour\nFive\nSix\nSeven\n\nEight\n"); } +static int string_stream_test_init(struct kunit *test) +{ + struct string_stream_test_priv *priv; + + priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + test->priv = priv; + + return 0; +} + static struct kunit_case string_stream_test_cases[] = { - KUNIT_CASE(string_stream_init_test), + KUNIT_CASE(string_stream_managed_init_test), + KUNIT_CASE(string_stream_unmanaged_init_test), + KUNIT_CASE(string_stream_managed_free_test), + KUNIT_CASE(string_stream_resource_free_test), KUNIT_CASE(string_stream_line_add_test), KUNIT_CASE(string_stream_variable_length_line_test), KUNIT_CASE(string_stream_append_test), @@ -348,6 +484,7 @@ static struct kunit_case string_stream_test_cases[] = { static struct kunit_suite string_stream_test_suite = { .name = "string-stream-test", - .test_cases = string_stream_test_cases + .test_cases = string_stream_test_cases, + .init = string_stream_test_init, }; kunit_test_suites(&string_stream_test_suite); diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c index 64abceb7b716..a6f3616c2048 100644 --- a/lib/kunit/string-stream.c +++ b/lib/kunit/string-stream.c @@ -6,6 +6,7 @@ * Author: Brendan Higgins */ +#include #include #include #include @@ -170,6 +171,8 @@ struct string_stream *alloc_string_stream(gfp_t gfp) void string_stream_destroy(struct string_stream *stream) { + KUNIT_STATIC_STUB_REDIRECT(string_stream_destroy, stream); + if (!stream) return; -- cgit v1.2.3 From 05e2006ce493cb6fb5e5b4b8317f82754dfa2b1e Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Mon, 28 Aug 2023 11:41:10 +0100 Subject: kunit: Use string_stream for test log 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. No new test have been added because there are already tests for the underlying string_stream. As the log tests now depend on string_stream functions they cannot build when kunit-test is a module. They have been surrounded by a #if to replace them with skipping version when the test is build as a module. Though this isn't pretty, it avoids moving code to another file while that code is also being changed. Signed-off-by: Richard Fitzgerald Reviewed-by: David Gow Signed-off-by: Shuah Khan --- include/kunit/test.h | 14 ++++++------- lib/kunit/debugfs.c | 36 ++++++++++++++++++++------------ lib/kunit/kunit-test.c | 56 ++++++++++++++++++++++++++++++++++++++++++-------- lib/kunit/test.c | 44 +++++---------------------------------- 4 files changed, 81 insertions(+), 69 deletions(-) (limited to 'lib') diff --git a/include/kunit/test.h b/include/kunit/test.h index 68ff01aee244..20ed9f9275c9 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -33,9 +33,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 @@ -133,7 +131,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) @@ -253,7 +251,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; }; @@ -279,7 +277,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; @@ -315,7 +313,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); @@ -473,7 +471,7 @@ static inline void *kunit_kcalloc(struct kunit *test, size_t n, size_t size, gfp void kunit_cleanup(struct kunit *test); -void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...); +void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt, ...); /** * kunit_mark_skipped() - Marks @test_or_suite as skipped diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c index 22c5c496a68f..270d185737e6 100644 --- a/lib/kunit/debugfs.c +++ b/lib/kunit/debugfs.c @@ -37,14 +37,21 @@ void kunit_debugfs_init(void) debugfs_rootdir = debugfs_create_dir(KUNIT_DEBUGFS_ROOT, NULL); } -static void debugfs_print_result(struct seq_file *seq, - struct kunit_suite *suite, - struct kunit_case *test_case) +static void debugfs_print_result(struct seq_file *seq, struct string_stream *log) { - if (!test_case || !test_case->log) + struct string_stream_fragment *frag_container; + + if (!log) return; - seq_printf(seq, "%s", test_case->log); + /* + * Walk the fragments so we don't need to allocate a temporary + * buffer to hold the entire string. + */ + spin_lock(&log->lock); + list_for_each_entry(frag_container, &log->fragments, node) + seq_printf(seq, "%s", frag_container->fragment); + spin_unlock(&log->lock); } /* @@ -69,10 +76,9 @@ static int debugfs_print_results(struct seq_file *seq, void *v) seq_printf(seq, KUNIT_SUBTEST_INDENT "1..%zd\n", kunit_suite_num_test_cases(suite)); kunit_suite_for_each_test_case(suite, test_case) - debugfs_print_result(seq, suite, test_case); + debugfs_print_result(seq, test_case->log); - if (suite->log) - seq_printf(seq, "%s", suite->log); + debugfs_print_result(seq, suite->log); seq_printf(seq, "%s %d %s\n", kunit_status_to_ok_not_ok(success), 1, suite->name); @@ -105,9 +111,13 @@ void kunit_debugfs_create_suite(struct kunit_suite *suite) struct kunit_case *test_case; /* Allocate logs before creating debugfs representation. */ - suite->log = kzalloc(KUNIT_LOG_SIZE, GFP_KERNEL); - kunit_suite_for_each_test_case(suite, test_case) - test_case->log = kzalloc(KUNIT_LOG_SIZE, GFP_KERNEL); + suite->log = alloc_string_stream(GFP_KERNEL); + string_stream_set_append_newlines(suite->log, true); + + kunit_suite_for_each_test_case(suite, test_case) { + test_case->log = alloc_string_stream(GFP_KERNEL); + string_stream_set_append_newlines(test_case->log, true); + } suite->debugfs = debugfs_create_dir(suite->name, debugfs_rootdir); @@ -121,7 +131,7 @@ void kunit_debugfs_destroy_suite(struct kunit_suite *suite) struct kunit_case *test_case; debugfs_remove_recursive(suite->debugfs); - kfree(suite->log); + string_stream_destroy(suite->log); kunit_suite_for_each_test_case(suite, test_case) - kfree(test_case->log); + string_stream_destroy(test_case->log); } diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c index 83d8e90ca7a2..99d2a3a528e1 100644 --- a/lib/kunit/kunit-test.c +++ b/lib/kunit/kunit-test.c @@ -8,6 +8,7 @@ #include #include +#include "string-stream.h" #include "try-catch-impl.h" struct kunit_try_catch_test_context { @@ -530,12 +531,27 @@ static struct kunit_suite kunit_resource_test_suite = { .test_cases = kunit_resource_test_cases, }; +/* + * Log tests call string_stream functions, which aren't exported. So only + * build this code if this test is built-in. + */ +#if IS_BUILTIN(CONFIG_KUNIT_TEST) + +/* This avoids a cast warning if kfree() is passed direct to kunit_add_action(). */ +static void kfree_wrapper(void *p) +{ + kfree(p); +} + static void kunit_log_test(struct kunit *test) { struct kunit_suite suite; - - suite.log = kunit_kzalloc(test, KUNIT_LOG_SIZE, GFP_KERNEL); +#ifdef CONFIG_KUNIT_DEBUGFS + char *full_log; +#endif + suite.log = kunit_alloc_string_stream(test, GFP_KERNEL); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, suite.log); + string_stream_set_append_newlines(suite.log, true); kunit_log(KERN_INFO, test, "put this in log."); kunit_log(KERN_INFO, test, "this too."); @@ -543,14 +559,21 @@ static void kunit_log_test(struct kunit *test) kunit_log(KERN_INFO, &suite, "along with this."); #ifdef CONFIG_KUNIT_DEBUGFS + KUNIT_EXPECT_TRUE(test, test->log->append_newlines); + + full_log = string_stream_get_string(test->log); + kunit_add_action(test, (kunit_action_t *)kfree, full_log); KUNIT_EXPECT_NOT_ERR_OR_NULL(test, - strstr(test->log, "put this in log.")); + strstr(full_log, "put this in log.")); KUNIT_EXPECT_NOT_ERR_OR_NULL(test, - strstr(test->log, "this too.")); + strstr(full_log, "this too.")); + + full_log = string_stream_get_string(suite.log); + kunit_add_action(test, kfree_wrapper, full_log); KUNIT_EXPECT_NOT_ERR_OR_NULL(test, - strstr(suite.log, "add to suite log.")); + strstr(full_log, "add to suite log.")); KUNIT_EXPECT_NOT_ERR_OR_NULL(test, - strstr(suite.log, "along with this.")); + strstr(full_log, "along with this.")); #else KUNIT_EXPECT_NULL(test, test->log); #endif @@ -558,15 +581,30 @@ static void kunit_log_test(struct kunit *test) static void kunit_log_newline_test(struct kunit *test) { + char *full_log; + kunit_info(test, "Add newline\n"); if (test->log) { - KUNIT_ASSERT_NOT_NULL_MSG(test, strstr(test->log, "Add newline\n"), - "Missing log line, full log:\n%s", test->log); - KUNIT_EXPECT_NULL(test, strstr(test->log, "Add newline\n\n")); + full_log = string_stream_get_string(test->log); + kunit_add_action(test, kfree_wrapper, full_log); + KUNIT_ASSERT_NOT_NULL_MSG(test, strstr(full_log, "Add newline\n"), + "Missing log line, full log:\n%s", full_log); + KUNIT_EXPECT_NULL(test, strstr(full_log, "Add newline\n\n")); } else { kunit_skip(test, "only useful when debugfs is enabled"); } } +#else +static void kunit_log_test(struct kunit *test) +{ + kunit_skip(test, "Log tests only run when built-in"); +} + +static void kunit_log_newline_test(struct kunit *test) +{ + kunit_skip(test, "Log tests only run when built-in"); +} +#endif /* IS_BUILTIN(CONFIG_KUNIT_TEST) */ static struct kunit_case kunit_log_test_cases[] = { KUNIT_CASE(kunit_log_test), diff --git a/lib/kunit/test.c b/lib/kunit/test.c index bd1ef86fcd71..651cbda9f250 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'; } -- cgit v1.2.3 From 53568b720c4dfb70d8db3da3587120fd0c378cae Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Mon, 28 Aug 2023 11:41:11 +0100 Subject: kunit: string-stream: Test performance of string_stream 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 Reviewed-by: David Gow Signed-off-by: Shuah Khan --- lib/kunit/string-stream-test.c | 54 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) (limited to 'lib') diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c index b759974d9cec..06822766f29a 100644 --- a/lib/kunit/string-stream-test.c +++ b/lib/kunit/string-stream-test.c @@ -8,7 +8,9 @@ #include #include +#include #include +#include #include "string-stream.h" @@ -454,6 +456,57 @@ static void string_stream_auto_newline_test(struct kunit *test) "One\nTwo\nThree\nFour\nFive\nSix\nSeven\n\nEight\n"); } +/* + * This doesn't actually "test" anything. It reports time taken + * and memory used for logging a large number of lines. + */ +static void string_stream_performance_test(struct kunit *test) +{ + struct string_stream_fragment *frag_container; + struct string_stream *stream; + char test_line[101]; + ktime_t start_time, end_time; + size_t len, bytes_requested, actual_bytes_used, total_string_length; + int offset, i; + + stream = kunit_alloc_string_stream(test, GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream); + + memset(test_line, 'x', sizeof(test_line) - 1); + test_line[sizeof(test_line) - 1] = '\0'; + + start_time = ktime_get(); + for (i = 0; i < 10000; i++) { + offset = i % (sizeof(test_line) - 1); + string_stream_add(stream, "%s: %d\n", &test_line[offset], i); + } + end_time = ktime_get(); + + /* + * Calculate memory used. This doesn't include invisible + * overhead due to kernel allocator fragment size rounding. + */ + bytes_requested = sizeof(*stream); + actual_bytes_used = ksize(stream); + total_string_length = 0; + + list_for_each_entry(frag_container, &stream->fragments, node) { + bytes_requested += sizeof(*frag_container); + actual_bytes_used += ksize(frag_container); + + len = strlen(frag_container->fragment); + total_string_length += len; + bytes_requested += len + 1; /* +1 for '\0' */ + actual_bytes_used += ksize(frag_container->fragment); + } + + kunit_info(test, "Time elapsed: %lld us\n", + ktime_us_delta(end_time, start_time)); + kunit_info(test, "Total string length: %zu\n", total_string_length); + kunit_info(test, "Bytes requested: %zu\n", bytes_requested); + kunit_info(test, "Actual bytes allocated: %zu\n", actual_bytes_used); +} + static int string_stream_test_init(struct kunit *test) { struct string_stream_test_priv *priv; @@ -479,6 +532,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), {} }; -- cgit v1.2.3 From ee5f8cc2770b2f0f7cfefb5bf7534e2859e39485 Mon Sep 17 00:00:00 2001 From: Michal Wajdeczko Date: Thu, 31 Aug 2023 23:48:47 +0200 Subject: kunit: Reset test status on each param iteration If we skip one parametrized test case then test status remains SKIP for all subsequent test params leading to wrong reports: $ ./tools/testing/kunit/kunit.py run \ --kunitconfig ./lib/kunit/.kunitconfig *.example_params* --raw_output \ [ ] Starting KUnit Kernel (1/1)... KTAP version 1 1..1 # example: initializing suite KTAP version 1 # Subtest: example # module: kunit_example_test 1..1 KTAP version 1 # Subtest: example_params_test # example_params_test: initializing # example_params_test: cleaning up ok 1 example value 3 # SKIP unsupported param value 3 # example_params_test: initializing # example_params_test: cleaning up ok 2 example value 2 # SKIP unsupported param value 3 # example_params_test: initializing # example_params_test: cleaning up ok 3 example value 1 # SKIP unsupported param value 3 # example_params_test: initializing # example_params_test: cleaning up ok 4 example value 0 # SKIP unsupported param value 0 # example_params_test: pass:0 fail:0 skip:4 total:4 ok 1 example_params_test # SKIP unsupported param value 0 # example: exiting suite ok 1 example # SKIP Reset test status and status comment after each param iteration to avoid using stale results. Signed-off-by: Michal Wajdeczko Cc: David Gow Cc: Rae Moar Reviewed-by: David Gow Signed-off-by: Shuah Khan --- lib/kunit/kunit-example-test.c | 5 +++-- lib/kunit/test.c | 6 ++++-- 2 files changed, 7 insertions(+), 4 deletions(-) (limited to 'lib') diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c index 01a769f35e1d..6bb5c2ef6696 100644 --- a/lib/kunit/kunit-example-test.c +++ b/lib/kunit/kunit-example-test.c @@ -190,6 +190,7 @@ static void example_static_stub_test(struct kunit *test) static const struct example_param { int value; } example_params_array[] = { + { .value = 3, }, { .value = 2, }, { .value = 1, }, { .value = 0, }, @@ -213,8 +214,8 @@ static void example_params_test(struct kunit *test) KUNIT_ASSERT_NOT_NULL(test, param); /* Test can be skipped on unsupported param values */ - if (!param->value) - kunit_skip(test, "unsupported param value"); + if (!is_power_of_2(param->value)) + kunit_skip(test, "unsupported param value %d", param->value); /* You can use param values for parameterized testing */ KUNIT_EXPECT_EQ(test, param->value % param->value, 0); diff --git a/lib/kunit/test.c b/lib/kunit/test.c index 651cbda9f250..f2eb71f1a66c 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -614,12 +614,14 @@ int kunit_run_tests(struct kunit_suite *suite) param_desc, test.status_comment); + kunit_update_stats(¶m_stats, test.status); + /* Get next param. */ param_desc[0] = '\0'; test.param_value = test_case->generate_params(test.param_value, param_desc); test.param_index++; - - kunit_update_stats(¶m_stats, test.status); + test.status = KUNIT_SUCCESS; + test.status_comment[0] = '\0'; } } -- cgit v1.2.3 From a6074cf0126b0bee51ab77a15930dc24a4d5db90 Mon Sep 17 00:00:00 2001 From: Jinjie Ruan Date: Wed, 27 Sep 2023 17:03:47 +0800 Subject: kunit: Fix missed memory release in kunit_free_suite_set() modprobe cpumask_kunit and rmmod cpumask_kunit, kmemleak detect a suspected memory leak as below. If kunit_filter_suites() in kunit_module_init() succeeds, the suite_set.start will not be NULL and the kunit_free_suite_set() in kunit_module_exit() should free all the memory which has not been freed. However the test_cases in suites is left out. unreferenced object 0xffff54ac47e83200 (size 512): comm "modprobe", pid 592, jiffies 4294913238 (age 1367.612s) hex dump (first 32 bytes): 84 13 1a f0 d3 b6 ff ff 30 68 1a f0 d3 b6 ff ff ........0h...... 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [<000000008dec63a2>] slab_post_alloc_hook+0xb8/0x368 [<00000000ec280d8e>] __kmem_cache_alloc_node+0x174/0x290 [<00000000896c7740>] __kmalloc+0x60/0x2c0 [<000000007a50fa06>] kunit_filter_suites+0x254/0x5b8 [<0000000078cc98e2>] kunit_module_notify+0xf4/0x240 [<0000000033cea952>] notifier_call_chain+0x98/0x17c [<00000000973d05cc>] notifier_call_chain_robust+0x4c/0xa4 [<000000005f95895f>] blocking_notifier_call_chain_robust+0x4c/0x74 [<0000000048e36fa7>] load_module+0x1a2c/0x1c40 [<0000000004eb8a91>] init_module_from_file+0x94/0xcc [<0000000037dbba28>] idempotent_init_module+0x184/0x278 [<00000000161b75cb>] __arm64_sys_finit_module+0x68/0xa8 [<000000006dc1669b>] invoke_syscall+0x44/0x100 [<00000000fa87e304>] el0_svc_common.constprop.1+0x68/0xe0 [<000000009d8ad866>] do_el0_svc+0x1c/0x28 [<000000005b83c607>] el0_svc+0x3c/0xc4 Fixes: a127b154a8f2 ("kunit: tool: allow filtering test cases via glob") Signed-off-by: Jinjie Ruan Reviewed-by: Rae Moar Reviewed-by: David Gow Signed-off-by: Shuah Khan --- lib/kunit/executor.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'lib') diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c index a6348489d45f..a037a46fae5e 100644 --- a/lib/kunit/executor.c +++ b/lib/kunit/executor.c @@ -137,8 +137,10 @@ void kunit_free_suite_set(struct kunit_suite_set suite_set) { struct kunit_suite * const *suites; - for (suites = suite_set.start; suites < suite_set.end; suites++) + for (suites = suite_set.start; suites < suite_set.end; suites++) { + kfree((*suites)->test_cases); kfree(*suites); + } kfree(suite_set.start); } -- cgit v1.2.3 From e44679515a7b803cf0143dc9de3d2ecbe907f939 Mon Sep 17 00:00:00 2001 From: Jinjie Ruan Date: Wed, 27 Sep 2023 17:03:48 +0800 Subject: kunit: Fix the wrong kfree of copy for kunit_filter_suites() If the outer layer for loop is iterated more than once and it fails not in the first iteration, the copy pointer has been moved. So it should free the original copy's backup copy_start. Fixes: abbf73816b6f ("kunit: fix possible memory leak in kunit_filter_suites()") Signed-off-by: Jinjie Ruan Reviewed-by: Rae Moar Reviewed-by: David Gow Signed-off-by: Shuah Khan --- lib/kunit/executor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib') diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c index a037a46fae5e..9358ed2df839 100644 --- a/lib/kunit/executor.c +++ b/lib/kunit/executor.c @@ -243,7 +243,7 @@ free_parsed_glob: free_copy: if (*err) - kfree(copy); + kfree(copy_start); return filtered; } -- cgit v1.2.3 From 24de14c98b37ea40a7e493dfd0d93b400b6efbca Mon Sep 17 00:00:00 2001 From: Jinjie Ruan Date: Wed, 27 Sep 2023 17:03:49 +0800 Subject: kunit: Fix possible memory leak in kunit_filter_suites() If the outer layer for loop is iterated more than once and it fails not in the first iteration, the filtered_suite and filtered_suite->test_cases allocated in the last kunit_filter_attr_tests() in last inner for loop is leaked. So add a new free_filtered_suite err label and free the filtered_suite and filtered_suite->test_cases so far. And change kmalloc_array of copy to kcalloc to Clear the copy to make the kfree safe. Fixes: 529534e8cba3 ("kunit: Add ability to filter attributes") Signed-off-by: Jinjie Ruan Reviewed-by: Rae Moar Reviewed-by: David Gow Signed-off-by: Shuah Khan --- lib/kunit/executor.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) (limited to 'lib') diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c index 9358ed2df839..1236b3cd2fbb 100644 --- a/lib/kunit/executor.c +++ b/lib/kunit/executor.c @@ -157,10 +157,11 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set, struct kunit_suite_set filtered = {NULL, NULL}; struct kunit_glob_filter parsed_glob; struct kunit_attr_filter *parsed_filters = NULL; + struct kunit_suite * const *suites; const size_t max = suite_set->end - suite_set->start; - copy = kmalloc_array(max, sizeof(*filtered.start), GFP_KERNEL); + copy = kcalloc(max, sizeof(*filtered.start), GFP_KERNEL); if (!copy) { /* won't be able to run anything, return an empty set */ return filtered; } @@ -195,7 +196,7 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set, parsed_glob.test_glob); if (IS_ERR(filtered_suite)) { *err = PTR_ERR(filtered_suite); - goto free_parsed_filters; + goto free_filtered_suite; } } if (filter_count > 0 && parsed_filters != NULL) { @@ -212,11 +213,11 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set, filtered_suite = new_filtered_suite; if (*err) - goto free_parsed_filters; + goto free_filtered_suite; if (IS_ERR(filtered_suite)) { *err = PTR_ERR(filtered_suite); - goto free_parsed_filters; + goto free_filtered_suite; } if (!filtered_suite) break; @@ -231,6 +232,14 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set, filtered.start = copy_start; filtered.end = copy; +free_filtered_suite: + if (*err) { + for (suites = copy_start; suites < copy; suites++) { + kfree((*suites)->test_cases); + kfree(*suites); + } + } + free_parsed_filters: if (filter_count) kfree(parsed_filters); -- cgit v1.2.3 From 8040345fdae4cb256c5d981f91ae0f22bea8adcc Mon Sep 17 00:00:00 2001 From: Jinjie Ruan Date: Wed, 27 Sep 2023 17:03:50 +0800 Subject: kunit: test: Fix the possible memory leak in executor_test When CONFIG_KUNIT_ALL_TESTS=y, making CONFIG_DEBUG_KMEMLEAK=y and CONFIG_DEBUG_KMEMLEAK_AUTO_SCAN=y, the below memory leak is detected. If kunit_filter_suites() succeeds, not only copy but also filtered_suite and filtered_suite->test_cases should be freed. So as Rae suggested, to avoid the suite set never be freed when KUNIT_ASSERT_EQ() fails and exits after kunit_filter_suites() succeeds, update kfree_at_end() func to free_suite_set_at_end() to use kunit_free_suite_set() to free them as kunit_module_exit() and kunit_run_all_tests() do it. As the second arg got of free_suite_set_at_end() is a local variable, copy it for free to avoid wild-memory-access. After applying this patch, the following memory leak is never detected. unreferenced object 0xffff8881001de400 (size 1024): comm "kunit_try_catch", pid 1396, jiffies 4294720452 (age 932.801s) hex dump (first 32 bytes): 73 75 69 74 65 32 00 00 00 00 00 00 00 00 00 00 suite2.......... 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [] __kmalloc_node_track_caller+0x53/0x150 [] kmemdup+0x22/0x50 [] kunit_filter_suites+0x44d/0xcc0 [] filter_suites_test+0x12f/0x360 [] kunit_generic_run_threadfn_adapter+0x4a/0x90 [] kthread+0x2b6/0x380 [] ret_from_fork+0x2d/0x70 [] ret_from_fork_asm+0x11/0x20 unreferenced object 0xffff8881052cd388 (size 192): comm "kunit_try_catch", pid 1396, jiffies 4294720452 (age 932.801s) hex dump (first 32 bytes): a0 85 9e 82 ff ff ff ff 80 cd 7c 84 ff ff ff ff ..........|..... 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [] __kmalloc+0x52/0x150 [] kunit_filter_suites+0x481/0xcc0 [] filter_suites_test+0x12f/0x360 [] kunit_generic_run_threadfn_adapter+0x4a/0x90 [] kthread+0x2b6/0x380 [] ret_from_fork+0x2d/0x70 [] ret_from_fork_asm+0x11/0x20 unreferenced object 0xffff888100da8400 (size 1024): comm "kunit_try_catch", pid 1398, jiffies 4294720454 (age 781.945s) hex dump (first 32 bytes): 73 75 69 74 65 32 00 00 00 00 00 00 00 00 00 00 suite2.......... 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [] __kmalloc_node_track_caller+0x53/0x150 [] kmemdup+0x22/0x50 [] kunit_filter_suites+0x44d/0xcc0 [] filter_suites_test_glob_test+0x12f/0x560 [] kunit_generic_run_threadfn_adapter+0x4a/0x90 [] kthread+0x2b6/0x380 [] ret_from_fork+0x2d/0x70 [] ret_from_fork_asm+0x11/0x20 unreferenced object 0xffff888105117878 (size 96): comm "kunit_try_catch", pid 1398, jiffies 4294720454 (age 781.945s) hex dump (first 32 bytes): a0 85 9e 82 ff ff ff ff a0 ac 7c 84 ff ff ff ff ..........|..... 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [] __kmalloc+0x52/0x150 [] kunit_filter_suites+0x481/0xcc0 [] filter_suites_test_glob_test+0x12f/0x560 [] kunit_generic_run_threadfn_adapter+0x4a/0x90 [] kthread+0x2b6/0x380 [] ret_from_fork+0x2d/0x70 [] ret_from_fork_asm+0x11/0x20 unreferenced object 0xffff888102c31c00 (size 1024): comm "kunit_try_catch", pid 1404, jiffies 4294720460 (age 781.948s) hex dump (first 32 bytes): 6e 6f 72 6d 61 6c 5f 73 75 69 74 65 00 00 00 00 normal_suite.... 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [] __kmalloc_node_track_caller+0x53/0x150 [] kmemdup+0x22/0x50 [] kunit_filter_attr_tests+0xf7/0x860 [] kunit_filter_suites+0x82f/0xcc0 [] filter_attr_test+0x195/0x5f0 [] kunit_generic_run_threadfn_adapter+0x4a/0x90 [] kthread+0x2b6/0x380 [] ret_from_fork+0x2d/0x70 [] ret_from_fork_asm+0x11/0x20 unreferenced object 0xffff8881052cd250 (size 192): comm "kunit_try_catch", pid 1404, jiffies 4294720460 (age 781.948s) hex dump (first 32 bytes): a0 85 9e 82 ff ff ff ff 00 a9 7c 84 ff ff ff ff ..........|..... 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [] __kmalloc+0x52/0x150 [] kunit_filter_attr_tests+0x1a1/0x860 [] kunit_filter_suites+0x82f/0xcc0 [] filter_attr_test+0x195/0x5f0 [] kunit_generic_run_threadfn_adapter+0x4a/0x90 [] kthread+0x2b6/0x380 [] ret_from_fork+0x2d/0x70 [] ret_from_fork_asm+0x11/0x20 unreferenced object 0xffff888104f4e400 (size 1024): comm "kunit_try_catch", pid 1408, jiffies 4294720464 (age 781.944s) hex dump (first 32 bytes): 73 75 69 74 65 00 00 00 00 00 00 00 00 00 00 00 suite........... 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [] __kmalloc_node_track_caller+0x53/0x150 [] kmemdup+0x22/0x50 [] kunit_filter_attr_tests+0xf7/0x860 [] kunit_filter_suites+0x82f/0xcc0 [] filter_attr_skip_test+0x133/0x6e0 [] kunit_generic_run_threadfn_adapter+0x4a/0x90 [] kthread+0x2b6/0x380 [] ret_from_fork+0x2d/0x70 [] ret_from_fork_asm+0x11/0x20 unreferenced object 0xffff8881052cc620 (size 192): comm "kunit_try_catch", pid 1408, jiffies 4294720464 (age 781.944s) hex dump (first 32 bytes): a0 85 9e 82 ff ff ff ff c0 a8 7c 84 ff ff ff ff ..........|..... 00 00 00 00 00 00 00 00 02 00 00 00 02 00 00 00 ................ backtrace: [] __kmalloc+0x52/0x150 [] kunit_filter_attr_tests+0x1a1/0x860 [] kunit_filter_suites+0x82f/0xcc0 [] filter_attr_skip_test+0x133/0x6e0 [] kunit_generic_run_threadfn_adapter+0x4a/0x90 [] kthread+0x2b6/0x380 [] ret_from_fork+0x2d/0x70 [] ret_from_fork_asm+0x11/0x20 Fixes: e5857d396f35 ("kunit: flatten kunit_suite*** to kunit_suite** in .kunit_test_suites") Fixes: 76066f93f1df ("kunit: add tests for filtering attributes") Signed-off-by: Jinjie Ruan Suggested-by: Rae Moar Reviewed-by: David Gow Suggested-by: David Gow Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202309142251.uJ8saAZv-lkp@intel.com/ Closes: https://lore.kernel.org/oe-kbuild-all/202309270433.wGmFRGjd-lkp@intel.com/ Signed-off-by: Shuah Khan --- lib/kunit/executor_test.c | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-) (limited to 'lib') diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c index b4f6f96b2844..22d4ee86dbed 100644 --- a/lib/kunit/executor_test.c +++ b/lib/kunit/executor_test.c @@ -9,7 +9,7 @@ #include #include -static void kfree_at_end(struct kunit *test, const void *to_free); +static void free_suite_set_at_end(struct kunit *test, const void *to_free); static struct kunit_suite *alloc_fake_suite(struct kunit *test, const char *suite_name, struct kunit_case *test_cases); @@ -56,7 +56,7 @@ static void filter_suites_test(struct kunit *test) got = kunit_filter_suites(&suite_set, "suite2", NULL, NULL, &err); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start); KUNIT_ASSERT_EQ(test, err, 0); - kfree_at_end(test, got.start); + free_suite_set_at_end(test, &got); /* Validate we just have suite2 */ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]); @@ -82,7 +82,7 @@ static void filter_suites_test_glob_test(struct kunit *test) got = kunit_filter_suites(&suite_set, "suite2.test2", NULL, NULL, &err); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start); KUNIT_ASSERT_EQ(test, err, 0); - kfree_at_end(test, got.start); + free_suite_set_at_end(test, &got); /* Validate we just have suite2 */ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]); @@ -109,7 +109,7 @@ static void filter_suites_to_empty_test(struct kunit *test) got = kunit_filter_suites(&suite_set, "not_found", NULL, NULL, &err); KUNIT_ASSERT_EQ(test, err, 0); - kfree_at_end(test, got.start); /* just in case */ + free_suite_set_at_end(test, &got); /* just in case */ KUNIT_EXPECT_PTR_EQ_MSG(test, got.start, got.end, "should be empty to indicate no match"); @@ -172,7 +172,7 @@ static void filter_attr_test(struct kunit *test) got = kunit_filter_suites(&suite_set, NULL, filter, NULL, &err); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start); KUNIT_ASSERT_EQ(test, err, 0); - kfree_at_end(test, got.start); + free_suite_set_at_end(test, &got); /* Validate we just have normal_suite */ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]); @@ -200,7 +200,7 @@ static void filter_attr_empty_test(struct kunit *test) got = kunit_filter_suites(&suite_set, NULL, filter, NULL, &err); KUNIT_ASSERT_EQ(test, err, 0); - kfree_at_end(test, got.start); /* just in case */ + free_suite_set_at_end(test, &got); /* just in case */ KUNIT_EXPECT_PTR_EQ_MSG(test, got.start, got.end, "should be empty to indicate no match"); @@ -222,7 +222,7 @@ static void filter_attr_skip_test(struct kunit *test) got = kunit_filter_suites(&suite_set, NULL, filter, "skip", &err); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start); KUNIT_ASSERT_EQ(test, err, 0); - kfree_at_end(test, got.start); + free_suite_set_at_end(test, &got); /* Validate we have both the slow and normal test */ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]->test_cases); @@ -256,18 +256,26 @@ kunit_test_suites(&executor_test_suite); /* Test helpers */ -/* Use the resource API to register a call to kfree(to_free). +static void free_suite_set(void *suite_set) +{ + kunit_free_suite_set(*(struct kunit_suite_set *)suite_set); + kfree(suite_set); +} + +/* Use the resource API to register a call to free_suite_set. * Since we never actually use the resource, it's safe to use on const data. */ -static void kfree_at_end(struct kunit *test, const void *to_free) +static void free_suite_set_at_end(struct kunit *test, const void *to_free) { - /* kfree() handles NULL already, but avoid allocating a no-op cleanup. */ - if (IS_ERR_OR_NULL(to_free)) + struct kunit_suite_set *free; + + if (!((struct kunit_suite_set *)to_free)->start) return; - kunit_add_action(test, - (kunit_action_t *)kfree, - (void *)to_free); + free = kzalloc(sizeof(struct kunit_suite_set), GFP_KERNEL); + *free = *(struct kunit_suite_set *)to_free; + + kunit_add_action(test, free_suite_set, (void *)free); } static struct kunit_suite *alloc_fake_suite(struct kunit *test, -- cgit v1.2.3