| Message ID | 20230814132309.32641-2-rf@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | kunit: Add dynamically-extending log | expand |
Hi Richard, kernel test robot noticed the following build warnings: [auto build test WARNING on shuah-kselftest/kunit] [also build test WARNING on shuah-kselftest/kunit-fixes linus/master v6.5-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
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 >
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), {} };
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(-)