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-stri... 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@i...) compiler: arc-elf-gcc (GCC) 12.3.0 reproduce: (https://download.01.org/0day-ci/archive/20230815/202308151555.o0Ok5tyv-lkp@i...)

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

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

This is enabled/disabled by calling string_stream_set_append_newlines().

Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com

Hello!

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

Reviewed-by: Rae Moar rmoar@google.com

Thanks! -Rae

---

lib/kunit/string-stream.c | 28 +++++++++++++++++++++------- lib/kunit/string-stream.h | 7 +++++++ 2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c index ed24d86af9f5..1dcf6513b692 100644 --- a/lib/kunit/string-stream.c +++ b/lib/kunit/string-stream.c @@ -44,32 +44,46 @@ int string_stream_vadd(struct string_stream *stream, va_list args) { struct string_stream_fragment *frag_container;

•   int len;
  

•   int buf_len, result_len;
    va_list args_for_counting;
  
    /* Make a copy because `vsnprintf` could change it */
    va_copy(args_for_counting, args);
  
    /* Evaluate length of formatted string */
  

•   len = vsnprintf(NULL, 0, fmt, args_for_counting);
  

•   buf_len = vsnprintf(NULL, 0, fmt, args_for_counting);
  
    va_end(args_for_counting);
  
  

•   if (len == 0)
  

•   if (buf_len == 0)
            return 0;
  
  

•   /* Reserve one extra for possible appended newline. */
  

•   if (stream->append_newlines)
  

•           buf_len++;
  

•   /* Need space for null byte. */
  

•   len++;
  

•   buf_len++;
  
    frag_container = alloc_string_stream_fragment(stream->test,
  

•                                                 len,
  

•                                                 buf_len,
                                                  stream->gfp);
    if (IS_ERR(frag_container))
            return PTR_ERR(frag_container);
  
  

•   len = vsnprintf(frag_container->fragment, len, fmt, args);
  

•   if (stream->append_newlines) {
  

•           /* Don't include reserved newline byte in writeable length. */
  

•           result_len = vsnprintf(frag_container->fragment, buf_len - 1, fmt, args);
  

•           /* Append newline if necessary. */
  

•           if (frag_container->fragment[result_len - 1] != '\n')
  

•                   result_len = strlcat(frag_container->fragment, "\n", buf_len);
  

•   } else {
  

•           result_len = vsnprintf(frag_container->fragment, buf_len, fmt, args);
  

•   }
  

•   spin_lock(&stream->lock);
  

•   stream->length += len;
  

•   stream->length += result_len;
    list_add_tail(&frag_container->node, &stream->fragments);
    spin_unlock(&stream->lock);
  
  

diff --git a/lib/kunit/string-stream.h b/lib/kunit/string-stream.h index b669f9a75a94..048930bf97f0 100644 --- a/lib/kunit/string-stream.h +++ b/lib/kunit/string-stream.h @@ -25,6 +25,7 @@ struct string_stream { spinlock_t lock; struct kunit *test; gfp_t gfp;

•   bool append_newlines;
  

};

struct kunit; @@ -47,4 +48,10 @@ bool string_stream_is_empty(struct string_stream *stream);

void string_stream_destroy(struct string_stream *stream);

+static inline void string_stream_set_append_newlines(struct string_stream *stream,

•                                                bool append_newlines)
  

+{

•   stream->append_newlines = append_newlines;
  

+}

#endif /* _KUNIT_STRING_STREAM_H */

2.30.2

On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald rf@opensource.cirrus.com wrote:

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

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

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

Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com

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

Reviewed-by: David Gow davidgow@google.com

-- David

lib/kunit/string-stream-test.c | 51 ++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+)

diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c index efe13e3322b5..46c2ac162fe8 100644 --- a/lib/kunit/string-stream-test.c +++ b/lib/kunit/string-stream-test.c @@ -23,6 +23,7 @@ static void string_stream_init_test(struct kunit *test) KUNIT_EXPECT_TRUE(test, list_empty(&stream->fragments)); KUNIT_EXPECT_PTR_EQ(test, stream->test, test); KUNIT_EXPECT_EQ(test, stream->gfp, GFP_KERNEL);

•   KUNIT_EXPECT_FALSE(test, stream->append_newlines);
  
    KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
  

} @@ -226,12 +227,62 @@ static void string_stream_append_empty_string_test(struct kunit *test) KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream), "Add this line"); }

+/* Adding strings without automatic newline appending */ +static void string_stream_no_auto_newline_test(struct kunit *test) +{

•   struct string_stream *stream;
  

•   stream = alloc_string_stream(test, GFP_KERNEL);
  

•   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
  

•   /*
  

•    * Add some strings with and without newlines. All formatted
  

•    * newlines should be preserved. No extra newlines should be
  

•    * added.
  

•    */
  

•   string_stream_add(stream, "One");
  

•   string_stream_add(stream, "Two\n");
  

•   string_stream_add(stream, "%s\n", "Three");
  

•   string_stream_add(stream, "Four");
  

•   KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream),
  

•                      "OneTwo\nThree\nFour");
  

+}

+/* Adding strings with automatic newline appending */ +static void string_stream_auto_newline_test(struct kunit *test) +{

•   struct string_stream *stream;
  

•   stream = alloc_string_stream(test, GFP_KERNEL);
  

•   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
  

•   string_stream_set_append_newlines(stream, true);
  

•   KUNIT_EXPECT_TRUE(test, stream->append_newlines);
  

•   /*
  

•    * Add some strings with and without newlines. Newlines should
  

•    * be appended to lines that do not end with \n, but newlines
  

•    * resulting from the formatting should not be changed.
  

•    */
  

•   string_stream_add(stream, "One");
  

•   string_stream_add(stream, "Two\n");
  

•   string_stream_add(stream, "%s\n", "Three");
  

•   string_stream_add(stream, "%s", "Four\n");
  

•   string_stream_add(stream, "Five\n%s", "Six");
  

•   string_stream_add(stream, "Seven\n\n");
  

•   string_stream_add(stream, "Eight");
  

•   KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream),
  

•                      "One\nTwo\nThree\nFour\nFive\nSix\nSeven\n\nEight\n");
  

+}

static struct kunit_case string_stream_test_cases[] = { KUNIT_CASE(string_stream_init_test), KUNIT_CASE(string_stream_line_add_test), KUNIT_CASE(string_stream_variable_length_line_test), KUNIT_CASE(string_stream_append_test), KUNIT_CASE(string_stream_append_empty_string_test),

•   KUNIT_CASE(string_stream_no_auto_newline_test),
  

•   KUNIT_CASE(string_stream_auto_newline_test),
    {}
  

};

-- 2.30.2

On 15/8/23 10:16, David Gow wrote:

On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald rf@opensource.cirrus.com wrote:

...

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

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

Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com

Makes sense to me.

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

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

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

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

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

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

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

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

So -- to switch to some stream-of-consciousness thoughts about this -- basically there are three possible variants of string_stream_get_string():

• Current version: uses the stream's struct kunit. Useless if this is

NULL, but very convenient otherwise.

• This patch: manage the string using the struct kunit passed as an

argument. Still can't be used directly outside a test, but adds enough flexibility to get _append to work.

• Totally unmanaged: the generated string is allocated separately, and

must be freed (directly or in a deferred action) by the caller. Works well outside tests, but less convenient.

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

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

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

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

Reviewed-by: David Gow davidgow@google.com

Cheers, -- David

...

lib/kunit/string-stream-test.c | 26 +++++++++++++++----------- lib/kunit/string-stream.c | 8 ++++---- lib/kunit/string-stream.h | 3 ++- lib/kunit/test.c | 2 +- 4 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c index 46c2ac162fe8..8a30bb7d5fb7 100644 --- a/lib/kunit/string-stream-test.c +++ b/lib/kunit/string-stream-test.c @@ -57,7 +57,7 @@ static void string_stream_line_add_test(struct kunit *test) } num_lines = i;

•   concat_string = string_stream_get_string(stream);
  

•   concat_string = string_stream_get_string(test, stream, GFP_KERNEL);
     KUNIT_EXPECT_NOT_ERR_OR_NULL(test, concat_string);
     KUNIT_EXPECT_EQ(test, strlen(concat_string), total_len);
  
  

@@ -113,7 +113,7 @@ static void string_stream_variable_length_line_test(struct kunit *test) } num_lines = i;

•   concat_string = string_stream_get_string(stream);
  

•   concat_string = string_stream_get_string(test, stream, GFP_KERNEL);
     KUNIT_EXPECT_NOT_ERR_OR_NULL(test, concat_string);
     KUNIT_EXPECT_EQ(test, strlen(concat_string), total_len);
  
  

@@ -165,17 +165,18 @@ static void string_stream_append_test(struct kunit *test)

     /* Append content of empty stream to empty stream */
     string_stream_append(stream_1, stream_2);

•   KUNIT_EXPECT_EQ(test, strlen(string_stream_get_string(stream_1)), 0);
  

•   KUNIT_EXPECT_EQ(test, strlen(string_stream_get_string(test, stream_1, GFP_KERNEL)), 0);
  
     /* Add some data to stream_1 */
     for (i = 0; i < ARRAY_SIZE(strings_1); ++i)
             string_stream_add(stream_1, "%s\n", strings_1[i]);
  
  

•   original_content = string_stream_get_string(stream_1);
  

•   original_content = string_stream_get_string(test, stream_1, GFP_KERNEL);
  
     /* Append content of empty stream to non-empty stream */
     string_stream_append(stream_1, stream_2);
  

•   KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream_1), original_content);
  

•   KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream_1, GFP_KERNEL),
  

•                      original_content);
  
     /* Add some data to stream_2 */
     for (i = 0; i < ARRAY_SIZE(strings_2); ++i)
  

@@ -188,14 +189,15 @@ static void string_stream_append_test(struct kunit *test) * End result should be the original content of stream_1 plus * the content of stream_2. */

•   stream_2_content = string_stream_get_string(stream_2);
  

•   stream_2_content = string_stream_get_string(test, stream_2, GFP_KERNEL);
     combined_length = strlen(original_content) + strlen(stream_2_content);
     combined_length++; /* for terminating \0 */
     combined_content = kunit_kmalloc(test, combined_length, GFP_KERNEL);
     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, combined_content);
     snprintf(combined_content, combined_length, "%s%s", original_content, stream_2_content);
  
  

•   KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream_1), combined_content);
  

•   KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream_1, GFP_KERNEL),
  

•                      combined_content);
  
     /* Append content of non-empty stream to empty stream */
     string_stream_destroy(stream_1);
  

@@ -204,7 +206,8 @@ static void string_stream_append_test(struct kunit *test) KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);

     string_stream_append(stream_1, stream_2);

•   KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream_1), stream_2_content);
  

•   KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream_1, GFP_KERNEL),
  

•                      stream_2_content);
  

  }

  /* Adding an empty string should not create a fragment. */

@@ -224,7 +227,8 @@ static void string_stream_append_empty_string_test(struct kunit *test) string_stream_add(stream, "Add this line"); string_stream_add(stream, "%s", ""); KUNIT_EXPECT_EQ(test, list_count_nodes(&stream->fragments), 1);

•   KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream), "Add this line");
  

•   KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream, GFP_KERNEL),
  

•                      "Add this line");
  

  }

  /* Adding strings without automatic newline appending */

@@ -244,7 +248,7 @@ static void string_stream_no_auto_newline_test(struct kunit *test) string_stream_add(stream, "Two\n"); string_stream_add(stream, "%s\n", "Three"); string_stream_add(stream, "Four");

•   KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream),
  

•   KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream, GFP_KERNEL),
                        "OneTwo\nThree\nFour");
  

  }

@@ -271,7 +275,7 @@ static void string_stream_auto_newline_test(struct kunit *test) string_stream_add(stream, "Five\n%s", "Six"); string_stream_add(stream, "Seven\n\n"); string_stream_add(stream, "Eight");

•   KUNIT_EXPECT_STREQ(test, string_stream_get_string(stream),
  

•   KUNIT_EXPECT_STREQ(test, string_stream_get_string(test, stream, GFP_KERNEL),
                        "One\nTwo\nThree\nFour\nFive\nSix\nSeven\n\nEight\n");
  

  }

diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c index 1dcf6513b692..eb673d3ea3bd 100644 --- a/lib/kunit/string-stream.c +++ b/lib/kunit/string-stream.c @@ -117,13 +117,14 @@ static void string_stream_clear(struct string_stream *stream) spin_unlock(&stream->lock); }

-char *string_stream_get_string(struct string_stream *stream) +char *string_stream_get_string(struct kunit *test, struct string_stream *stream,

•                          gfp_t gfp)
  

  { struct string_stream_fragment *frag_container; size_t buf_len = stream->length + 1; /* +1 for null byte. */ char *buf;

•   buf = kunit_kzalloc(stream->test, buf_len, stream->gfp);
  

•   buf = kunit_kzalloc(test, buf_len, gfp);
     if (!buf)
             return NULL;
  
  

@@ -140,8 +141,7 @@ int string_stream_append(struct string_stream *stream, { const char *other_content;

•   other_content = string_stream_get_string(other);
  

•   other_content = string_stream_get_string(other->test, other, other->gfp);
     if (!other_content)
             return -ENOMEM;
  
  

diff --git a/lib/kunit/string-stream.h b/lib/kunit/string-stream.h index 048930bf97f0..6b4a747881c5 100644 --- a/lib/kunit/string-stream.h +++ b/lib/kunit/string-stream.h @@ -39,7 +39,8 @@ int __printf(2, 0) string_stream_vadd(struct string_stream *stream, const char *fmt, va_list args);

-char *string_stream_get_string(struct string_stream *stream); +char *string_stream_get_string(struct kunit *test, struct string_stream *stream,

•                          gfp_t gfp);
  
  

  int string_stream_append(struct string_stream *stream, struct string_stream *other);

diff --git a/lib/kunit/test.c b/lib/kunit/test.c index 49698a168437..520e15f49d0d 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -286,7 +286,7 @@ static void kunit_print_string_stream(struct kunit *test, if (string_stream_is_empty(stream)) return;

•   buf = string_stream_get_string(stream);
  

•   buf = string_stream_get_string(test, stream, GFP_KERNEL);
     if (!buf) {
             kunit_err(test,
                       "Could not allocate buffer, dumping stream:\n");
  

-- 2.30.2

On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald rf@opensource.cirrus.com wrote:

Re-work string_stream so that it is not tied to a struct kunit. This is to allow using it for the log of struct kunit_suite.

Instead of resource-managing individual allocations the whole string_stream object can be resource-managed as a single object:

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

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

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

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

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

Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com

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

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

Reviewed-by: David Gow davidgow@google.com

lib/kunit/string-stream-test.c | 2 +- lib/kunit/string-stream.c | 92 +++++++++++++++++++++++----------- lib/kunit/string-stream.h | 12 ++++- lib/kunit/test.c | 2 +- 4 files changed, 77 insertions(+), 31 deletions(-)

diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c index 8a30bb7d5fb7..437aa4b3179d 100644 --- a/lib/kunit/string-stream-test.c +++ b/lib/kunit/string-stream-test.c @@ -200,7 +200,7 @@ static void string_stream_append_test(struct kunit *test) combined_content);

    /* Append content of non-empty stream to empty stream */

•   string_stream_destroy(stream_1);
  

•   string_stream_clear(stream_1);
  
    stream_1 = alloc_string_stream(test, GFP_KERNEL);
    KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream_1);
  

diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c index eb673d3ea3bd..06104a729b45 100644 --- a/lib/kunit/string-stream.c +++ b/lib/kunit/string-stream.c @@ -13,30 +13,28 @@ #include "string-stream.h"

-static struct string_stream_fragment *alloc_string_stream_fragment(

•           struct kunit *test, int len, gfp_t gfp)
  

+static struct string_stream_fragment *alloc_string_stream_fragment(int len, gfp_t gfp) { struct string_stream_fragment *frag;

•   frag = kunit_kzalloc(test, sizeof(*frag), gfp);
  

•   frag = kzalloc(sizeof(*frag), gfp);
    if (!frag)
            return ERR_PTR(-ENOMEM);
  
  

•   frag->fragment = kunit_kmalloc(test, len, gfp);
  

•   frag->fragment = kmalloc(len, gfp);
    if (!frag->fragment) {
  

•           kunit_kfree(test, frag);
  

•           kfree(frag);
            return ERR_PTR(-ENOMEM);
    }
  
    return frag;
  

}

-static void string_stream_fragment_destroy(struct kunit *test,

•                                      struct string_stream_fragment *frag)
  

+static void string_stream_fragment_destroy(struct string_stream_fragment *frag) { list_del(&frag->node);

•   kunit_kfree(test, frag->fragment);
  

•   kunit_kfree(test, frag);
  

•   kfree(frag->fragment);
  

•   kfree(frag);
  

}

int string_stream_vadd(struct string_stream *stream, @@ -65,9 +63,7 @@ int string_stream_vadd(struct string_stream *stream, /* Need space for null byte. */ buf_len++;

•   frag_container = alloc_string_stream_fragment(stream->test,
  

•                                                 buf_len,
  

•                                                 stream->gfp);
  

•   frag_container = alloc_string_stream_fragment(buf_len, stream->gfp);
    if (IS_ERR(frag_container))
            return PTR_ERR(frag_container);
  
  

@@ -102,7 +98,7 @@ int string_stream_add(struct string_stream *stream, const char *fmt, ...) return result; }

-static void string_stream_clear(struct string_stream *stream) +void string_stream_clear(struct string_stream *stream) { struct string_stream_fragment *frag_container, *frag_container_safe;

@@ -111,16 +107,28 @@ static void string_stream_clear(struct string_stream *stream) frag_container_safe, &stream->fragments, node) {

•           string_stream_fragment_destroy(stream->test, frag_container);
  

•           string_stream_fragment_destroy(frag_container);
    }
    stream->length = 0;
    spin_unlock(&stream->lock);
  

}

+static void _string_stream_concatenate_to_buf(struct string_stream *stream,

•                                         char *buf, size_t buf_len)
  

+{

•   struct string_stream_fragment *frag_container;
  

•   buf[0] = '\0';
  

•   spin_lock(&stream->lock);
  

•   list_for_each_entry(frag_container, &stream->fragments, node)
  

•           strlcat(buf, frag_container->fragment, buf_len);
  

•   spin_unlock(&stream->lock);
  

+}

char *string_stream_get_string(struct kunit *test, struct string_stream *stream, gfp_t gfp) {

•   struct string_stream_fragment *frag_container;
    size_t buf_len = stream->length + 1; /* +1 for null byte. */
    char *buf;
  
  

@@ -128,10 +136,7 @@ char *string_stream_get_string(struct kunit *test, struct string_stream *stream, if (!buf) return NULL;

•   spin_lock(&stream->lock);
  

•   list_for_each_entry(frag_container, &stream->fragments, node)
  

•           strlcat(buf, frag_container->fragment, buf_len);
  

•   spin_unlock(&stream->lock);
  

•   _string_stream_concatenate_to_buf(stream, buf, buf_len);
  
    return buf;
  

} @@ -139,13 +144,20 @@ char *string_stream_get_string(struct kunit *test, struct string_stream *stream, int string_stream_append(struct string_stream *stream, struct string_stream *other) {

•   const char *other_content;
  

•   size_t other_buf_len = other->length + 1; /* +1 for null byte. */
  

•   char *other_buf;
  

•   int ret;
  
  

•   other_content = string_stream_get_string(other->test, other, other->gfp);
  

•   if (!other_content)
  

•   other_buf = kmalloc(other_buf_len, GFP_KERNEL);
  

•   if (!other_buf)
            return -ENOMEM;
  
  

•   return string_stream_add(stream, other_content);
  

•   _string_stream_concatenate_to_buf(other, other_buf, other_buf_len);
  

•   ret = string_stream_add(stream, other_buf);
  

•   kfree(other_buf);
  

•   return ret;
  

}

bool string_stream_is_empty(struct string_stream *stream) @@ -153,23 +165,47 @@ bool string_stream_is_empty(struct string_stream *stream) return list_empty(&stream->fragments); }

-struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp) +void raw_free_string_stream(struct string_stream *stream) +{

•   string_stream_clear(stream);
  

•   kfree(stream);
  

+}

+struct string_stream *raw_alloc_string_stream(gfp_t gfp) { struct string_stream *stream;

•   stream = kunit_kzalloc(test, sizeof(*stream), gfp);
  

•   stream = kzalloc(sizeof(*stream), gfp);
    if (!stream)
            return ERR_PTR(-ENOMEM);
  
    stream->gfp = gfp;
  

•   stream->test = test;
    INIT_LIST_HEAD(&stream->fragments);
    spin_lock_init(&stream->lock);
  
    return stream;
  

}

-void string_stream_destroy(struct string_stream *stream) +struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp) {

•   string_stream_clear(stream);
  

•   struct string_stream *stream;
  

•   stream = raw_alloc_string_stream(gfp);
  

•   if (IS_ERR(stream))
  

•           return stream;
  

•   stream->test = test;
  

•   if (kunit_add_action_or_reset(test, (kunit_action_t *)raw_free_string_stream, stream) != 0)
  

•           return ERR_PTR(-ENOMEM);
  

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

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

•   return stream;
  

+}

+void free_string_stream(struct kunit *test, struct string_stream *stream) +{

•   if (!stream)
  

•           return;
  

•   kunit_release_action(test, (kunit_action_t *)raw_free_string_stream, (void *)stream);
  

(As above.)

} diff --git a/lib/kunit/string-stream.h b/lib/kunit/string-stream.h index 6b4a747881c5..fbeda486382a 100644 --- a/lib/kunit/string-stream.h +++ b/lib/kunit/string-stream.h @@ -23,14 +23,24 @@ struct string_stream { struct list_head fragments; /* length and fragments are protected by this lock */ spinlock_t lock;

•   /*
  

•    * Pointer to kunit this stream is associated with, or NULL if
  

•    * not associated with a kunit.
  

•    */
    struct kunit *test;
  

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

    gfp_t gfp;
    bool append_newlines;

};

struct kunit;

+struct string_stream *raw_alloc_string_stream(gfp_t gfp); +void raw_free_string_stream(struct string_stream *stream);

struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp); +void free_string_stream(struct kunit *test, struct string_stream *stream);

int __printf(2, 3) string_stream_add(struct string_stream *stream, const char *fmt, ...); @@ -47,7 +57,7 @@ int string_stream_append(struct string_stream *stream,

bool string_stream_is_empty(struct string_stream *stream);

-void string_stream_destroy(struct string_stream *stream); +void string_stream_clear(struct string_stream *stream);

static inline void string_stream_set_append_newlines(struct string_stream *stream, bool append_newlines) diff --git a/lib/kunit/test.c b/lib/kunit/test.c index 520e15f49d0d..4b69d12dfc96 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -322,7 +322,7 @@ static void kunit_fail(struct kunit *test, const struct kunit_loc *loc,

    kunit_print_string_stream(test, stream);

•   string_stream_destroy(stream);
  

•   free_string_stream(test, stream);
  

}

void __noreturn __kunit_abort(struct kunit *test)

2.30.2

On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald rf@opensource.cirrus.com wrote:

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

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

Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com

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

Reviewed-by: David Gow davidgow@google.com

Cheers, -- David

lib/kunit/string-stream-test.c | 117 ++++++++++++++++++++++++++++++++- lib/kunit/string-stream.c | 3 + 2 files changed, 119 insertions(+), 1 deletion(-)

diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c index 437aa4b3179d..05bfade2bd8a 100644 --- a/lib/kunit/string-stream-test.c +++ b/lib/kunit/string-stream-test.c @@ -6,16 +6,27 @@

• Author: Brendan Higgins brendanhiggins@google.com

*/

+#include <kunit/static_stub.h> #include <kunit/test.h> #include <linux/slab.h>

#include "string-stream.h"

+struct string_stream_test_priv {

•   struct string_stream *raw_stream;
  

•   /* For testing resource-managed free */
  

•   struct string_stream *freed_stream;
  

•   bool stream_free_again;
  

+};

/* string_stream object is initialized correctly. */ static void string_stream_init_test(struct kunit *test) {

•   struct string_stream_test_priv *priv = test->priv;
    struct string_stream *stream;
  
  

•   /* Resource-managed initialization */
    stream = alloc_string_stream(test, GFP_KERNEL);
    KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
  
  

@@ -26,6 +37,86 @@ static void string_stream_init_test(struct kunit *test) KUNIT_EXPECT_FALSE(test, stream->append_newlines);

    KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));

•   free_string_stream(test, stream);
  

•   /*
  

•    * Raw initialization. This allocation is not resource-managed
  

•    * so store it in priv->raw_stream to be cleaned up by the
  

•    * exit function.
  

•    */
  

•   priv->raw_stream = raw_alloc_string_stream(GFP_KERNEL);
  

•   stream = priv->raw_stream;
  

•   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
  

•   KUNIT_EXPECT_EQ(test, stream->length, 0);
  

•   KUNIT_EXPECT_TRUE(test, list_empty(&stream->fragments));
  

•   KUNIT_EXPECT_PTR_EQ(test, stream->test, NULL);
  

•   KUNIT_EXPECT_EQ(test, stream->gfp, GFP_KERNEL);
  

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

•   KUNIT_EXPECT_FALSE(test, stream->append_newlines);
  

•   KUNIT_EXPECT_TRUE(test, string_stream_is_empty(stream));
  

+}

+static void string_stream_raw_free_string_stream_stub(struct string_stream *stream) +{

•   struct kunit *fake_test = kunit_get_current_test();
  

•   struct string_stream_test_priv *priv = fake_test->priv;
  

•   if (priv->freed_stream)
  

•           priv->stream_free_again = true;
  

•   priv->freed_stream = stream;
  

•   /*
  

•    * Avoid calling deactivate_static_stub() or changing
  

•    * current->kunit_test during cleanup. Leave the stream to
  

•    * be freed during the test exit.
  

•    */
  

+}

+/* string_stream object is freed when test is cleaned up. */ +static void string_stream_resource_free_test(struct kunit *test) +{

•   struct string_stream_test_priv *priv = test->priv;
  

•   struct kunit *fake_test;
  

•   struct string_stream *stream;
  

•   fake_test = kunit_kzalloc(test, sizeof(*fake_test), GFP_KERNEL);
  

•   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fake_test);
  

•   kunit_init_test(fake_test, "string_stream_fake_test", NULL);
  

•   fake_test->priv = priv;
  

•   /*
  

•    * Activate stub before creating string_stream so the
  

•    * string_stream will be cleaned up first.
  

•    */
  

•   priv->freed_stream = NULL;
  

•   priv->stream_free_again = false;
  

•   kunit_activate_static_stub(fake_test,
  

•                              raw_free_string_stream,
  

•                              string_stream_raw_free_string_stream_stub);
  

•   stream = alloc_string_stream(fake_test, GFP_KERNEL);
  

•   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
  

•   /*
  

•    * Ensure the stream is freed when this test terminates.
  

•    */
  

•   priv->raw_stream = stream;
  

•   /* Set current->kunit_test to fake_test so the static stub will be called. */
  

•   current->kunit_test = fake_test;
  

•   /* Cleanup test - the stub function should be called */
  

•   kunit_cleanup(fake_test);
  

•   /* Set current->kunit_test back to current test. */
  

•   current->kunit_test = test;
  

•   KUNIT_EXPECT_PTR_EQ(test, priv->freed_stream, stream);
  

•   KUNIT_EXPECT_FALSE(test, priv->stream_free_again);
  

}

/* @@ -279,8 +370,30 @@ static void string_stream_auto_newline_test(struct kunit *test) "One\nTwo\nThree\nFour\nFive\nSix\nSeven\n\nEight\n"); }

+static int string_stream_test_init(struct kunit *test) +{

•   struct string_stream_test_priv *priv;
  

•   priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
  

•   if (!priv)
  

•           return -ENOMEM;
  

•   test->priv = priv;
  

•   return 0;
  

+}

+static void string_stream_test_exit(struct kunit *test) +{

•   struct string_stream_test_priv *priv = test->priv;
  

•   if (priv && priv->raw_stream)
  

•           raw_free_string_stream(priv->raw_stream);
  

+}

static struct kunit_case string_stream_test_cases[] = { KUNIT_CASE(string_stream_init_test),

•   KUNIT_CASE(string_stream_resource_free_test),
    KUNIT_CASE(string_stream_line_add_test),
    KUNIT_CASE(string_stream_variable_length_line_test),
    KUNIT_CASE(string_stream_append_test),
  

@@ -292,6 +405,8 @@ static struct kunit_case string_stream_test_cases[] = {

static struct kunit_suite string_stream_test_suite = { .name = "string-stream-test",

•   .test_cases = string_stream_test_cases
  

•   .test_cases = string_stream_test_cases,
  

•   .init = string_stream_test_init,
  

•   .exit = string_stream_test_exit,
  

}; kunit_test_suites(&string_stream_test_suite); diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c index 06104a729b45..1b55ac1be2e5 100644 --- a/lib/kunit/string-stream.c +++ b/lib/kunit/string-stream.c @@ -6,6 +6,7 @@

• Author: Brendan Higgins brendanhiggins@google.com

*/

+#include <kunit/static_stub.h> #include <kunit/test.h> #include <linux/list.h> #include <linux/slab.h> @@ -167,6 +168,8 @@ bool string_stream_is_empty(struct string_stream *stream)

void raw_free_string_stream(struct string_stream *stream) {

•   KUNIT_STATIC_STUB_REDIRECT(raw_free_string_stream, stream);
  

•   string_stream_clear(stream);
    kfree(stream);
  

}

2.30.2

On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald rf@opensource.cirrus.com wrote:

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

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

Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com

Yay! This is much nicer.

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

Reviewed-by: David Gow davidgow@google.com

Cheers, -- David

include/kunit/test.h | 14 ++++---- lib/kunit/Makefile | 5 +-- lib/kunit/debugfs.c | 36 +++++++++++++-------- lib/kunit/kunit-test.c | 52 +----------------------------- lib/kunit/log-test.c | 72 ++++++++++++++++++++++++++++++++++++++++++ lib/kunit/test.c | 44 +++----------------------- 6 files changed, 110 insertions(+), 113 deletions(-) create mode 100644 lib/kunit/log-test.c

diff --git a/include/kunit/test.h b/include/kunit/test.h index d33114097d0d..b915a0fe93c0 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -32,9 +32,7 @@ DECLARE_STATIC_KEY_FALSE(kunit_running);

struct kunit;

-/* Size of log associated with test. */ -#define KUNIT_LOG_SIZE 2048 +struct string_stream;

/* Maximum size of parameter description string. */ #define KUNIT_PARAM_DESC_SIZE 128 @@ -132,7 +130,7 @@ struct kunit_case { /* private: internal use only. */ enum kunit_status status; char *module_name;

•   char *log;
  

•   struct string_stream *log;
  

};

static inline char *kunit_status_to_ok_not_ok(enum kunit_status status) @@ -252,7 +250,7 @@ struct kunit_suite { /* private: internal use only */ char status_comment[KUNIT_STATUS_COMMENT_SIZE]; struct dentry *debugfs;

•   char *log;
  

•   struct string_stream *log;
    int suite_init_err;
  

};

@@ -278,7 +276,7 @@ struct kunit {

    /* private: internal use only. */
    const char *name; /* Read only after initialization! */

•   char *log; /* Points at case log after initialization */
  

•   struct string_stream *log; /* Points at case log after initialization */
    struct kunit_try_catch try_catch;
    /* param_value is the current parameter value for a test case. */
    const void *param_value;
  

@@ -314,7 +312,7 @@ const char *kunit_filter_glob(void); char *kunit_filter(void); char *kunit_filter_action(void);

-void kunit_init_test(struct kunit *test, const char *name, char *log); +void kunit_init_test(struct kunit *test, const char *name, struct string_stream *log);

int kunit_run_tests(struct kunit_suite *suite);

@@ -472,7 +470,7 @@ static inline void *kunit_kcalloc(struct kunit *test, size_t n, size_t size, gfp

void kunit_cleanup(struct kunit *test);

-void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...); +void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt, ...);

/**

• kunit_mark_skipped() - Marks @test_or_suite as skipped

diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile index 46f75f23dfe4..65735c2e1d14 100644 --- a/lib/kunit/Makefile +++ b/lib/kunit/Makefile @@ -18,9 +18,10 @@ obj-y += hooks.o

obj-$(CONFIG_KUNIT_TEST) += kunit-test.o

-# string-stream-test compiles built-in only. +# string-stream-test and log-test compiles built-in only. ifeq ($(CONFIG_KUNIT_TEST),y) -obj-$(CONFIG_KUNIT_TEST) += string-stream-test.o +obj-$(CONFIG_KUNIT_TEST) += string-stream-test.o \

•                                   log-test.o
  

endif

obj-$(CONFIG_KUNIT_EXAMPLE_TEST) += kunit-example-test.o diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c index 22c5c496a68f..ab53a7e5c898 100644 --- a/lib/kunit/debugfs.c +++ b/lib/kunit/debugfs.c @@ -37,14 +37,21 @@ void kunit_debugfs_init(void) debugfs_rootdir = debugfs_create_dir(KUNIT_DEBUGFS_ROOT, NULL); }

-static void debugfs_print_result(struct seq_file *seq,

•                            struct kunit_suite *suite,
  

•                            struct kunit_case *test_case)
  

+static void debugfs_print_result(struct seq_file *seq, struct string_stream *log) {

•   if (!test_case || !test_case->log)
  

•   struct string_stream_fragment *frag_container;
  

•   if (!log)
            return;
  
  

•   seq_printf(seq, "%s", test_case->log);
  

•   /*
  

•    * Walk the fragments so we don't need to allocate a temporary
  

•    * buffer to hold the entire string.
  

•    */
  

•   spin_lock(&log->lock);
  

•   list_for_each_entry(frag_container, &log->fragments, node)
  

•           seq_printf(seq, "%s", frag_container->fragment);
  

•   spin_unlock(&log->lock);
  

}

/* @@ -69,10 +76,9 @@ static int debugfs_print_results(struct seq_file *seq, void *v) seq_printf(seq, KUNIT_SUBTEST_INDENT "1..%zd\n", kunit_suite_num_test_cases(suite));

    kunit_suite_for_each_test_case(suite, test_case)

•           debugfs_print_result(seq, suite, test_case);
  

•           debugfs_print_result(seq, test_case->log);
  
  

•   if (suite->log)
  

•           seq_printf(seq, "%s", suite->log);
  

•   debugfs_print_result(seq, suite->log);
  
    seq_printf(seq, "%s %d %s\n",
               kunit_status_to_ok_not_ok(success), 1, suite->name);
  

@@ -105,9 +111,13 @@ void kunit_debugfs_create_suite(struct kunit_suite *suite) struct kunit_case *test_case;

    /* Allocate logs before creating debugfs representation. */

•   suite->log = kzalloc(KUNIT_LOG_SIZE, GFP_KERNEL);
  

•   kunit_suite_for_each_test_case(suite, test_case)
  

•           test_case->log = kzalloc(KUNIT_LOG_SIZE, GFP_KERNEL);
  

•   suite->log = raw_alloc_string_stream(GFP_KERNEL);
  

•   string_stream_set_append_newlines(suite->log, true);
  

•   kunit_suite_for_each_test_case(suite, test_case) {
  

•           test_case->log = raw_alloc_string_stream(GFP_KERNEL);
  

•           string_stream_set_append_newlines(test_case->log, true);
  

•   }
  
    suite->debugfs = debugfs_create_dir(suite->name, debugfs_rootdir);
  
  

@@ -121,7 +131,7 @@ void kunit_debugfs_destroy_suite(struct kunit_suite *suite) struct kunit_case *test_case;

    debugfs_remove_recursive(suite->debugfs);

•   kfree(suite->log);
  

•   raw_free_string_stream(suite->log);
    kunit_suite_for_each_test_case(suite, test_case)
  

•           kfree(test_case->log);
  

•           raw_free_string_stream(test_case->log);
  

} diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c index 83d8e90ca7a2..ecc39d5f7411 100644 --- a/lib/kunit/kunit-test.c +++ b/lib/kunit/kunit-test.c @@ -530,55 +530,6 @@ static struct kunit_suite kunit_resource_test_suite = { .test_cases = kunit_resource_test_cases, };

-static void kunit_log_test(struct kunit *test) -{

•   struct kunit_suite suite;
  

•   suite.log = kunit_kzalloc(test, KUNIT_LOG_SIZE, GFP_KERNEL);
  

•   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, suite.log);
  

•   kunit_log(KERN_INFO, test, "put this in log.");
  

•   kunit_log(KERN_INFO, test, "this too.");
  

•   kunit_log(KERN_INFO, &suite, "add to suite log.");
  

•   kunit_log(KERN_INFO, &suite, "along with this.");
  

-#ifdef CONFIG_KUNIT_DEBUGFS

•   KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
  

•                                strstr(test->log, "put this in log."));
  

•   KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
  

•                                strstr(test->log, "this too."));
  

•   KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
  

•                                strstr(suite.log, "add to suite log."));
  

•   KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
  

•                                strstr(suite.log, "along with this."));
  

-#else

•   KUNIT_EXPECT_NULL(test, test->log);
  

-#endif -}

-static void kunit_log_newline_test(struct kunit *test) -{

•   kunit_info(test, "Add newline\n");
  

•   if (test->log) {
  

•           KUNIT_ASSERT_NOT_NULL_MSG(test, strstr(test->log, "Add newline\n"),
  

•                   "Missing log line, full log:\n%s", test->log);
  

•           KUNIT_EXPECT_NULL(test, strstr(test->log, "Add newline\n\n"));
  

•   } else {
  

•           kunit_skip(test, "only useful when debugfs is enabled");
  

•   }
  

-}

-static struct kunit_case kunit_log_test_cases[] = {

•   KUNIT_CASE(kunit_log_test),
  

•   KUNIT_CASE(kunit_log_newline_test),
  

•   {}
  

-};

-static struct kunit_suite kunit_log_test_suite = {

•   .name = "kunit-log-test",
  

•   .test_cases = kunit_log_test_cases,
  

-};

static void kunit_status_set_failure_test(struct kunit *test) { struct kunit fake; @@ -658,7 +609,6 @@ static struct kunit_suite kunit_current_test_suite = { };

kunit_test_suites(&kunit_try_catch_test_suite, &kunit_resource_test_suite,

•             &kunit_log_test_suite, &kunit_status_test_suite,
  

•             &kunit_current_test_suite);
  

•             &kunit_status_test_suite, &kunit_current_test_suite);
  
  

MODULE_LICENSE("GPL v2"); diff --git a/lib/kunit/log-test.c b/lib/kunit/log-test.c new file mode 100644 index 000000000000..a93d87112fea --- /dev/null +++ b/lib/kunit/log-test.c @@ -0,0 +1,72 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • KUnit test for logging.
• • Based on code:
• • Copyright (C) 2019, Google LLC.
• • Author: Brendan Higgins brendanhiggins@google.com
• */

+#include <kunit/test.h>

+#include "string-stream.h"

+static void kunit_log_test(struct kunit *test) +{

•   struct kunit_suite suite;
  

•   char *full_log;
  

•   suite.log = alloc_string_stream(test, GFP_KERNEL);
  

•   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, suite.log);
  

•   string_stream_set_append_newlines(suite.log, true);
  

•   kunit_log(KERN_INFO, test, "put this in log.");
  

•   kunit_log(KERN_INFO, test, "this too.");
  

•   kunit_log(KERN_INFO, &suite, "add to suite log.");
  

•   kunit_log(KERN_INFO, &suite, "along with this.");
  

+#ifdef CONFIG_KUNIT_DEBUGFS

•   KUNIT_EXPECT_TRUE(test, test->log->append_newlines);
  

•   full_log = string_stream_get_string(test, test->log, GFP_KERNEL);
  

•   KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
  

•                                strstr(full_log, "put this in log."));
  

•   KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
  

•                                strstr(full_log, "this too."));
  

•   full_log = string_stream_get_string(test, suite.log, GFP_KERNEL);
  

•   KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
  

•                                strstr(full_log, "add to suite log."));
  

•   KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
  

•                                strstr(full_log, "along with this."));
  

+#else

•   KUNIT_EXPECT_NULL(test, test->log);
  

+#endif +}

+static void kunit_log_newline_test(struct kunit *test) +{

•   char *full_log;
  

•   kunit_info(test, "Add newline\n");
  

•   if (test->log) {
  

•           full_log = string_stream_get_string(test, test->log, GFP_KERNEL);
  

•           KUNIT_ASSERT_NOT_NULL_MSG(test, strstr(full_log, "Add newline\n"),
  

•                   "Missing log line, full log:\n%s", full_log);
  

•           KUNIT_EXPECT_NULL(test, strstr(full_log, "Add newline\n\n"));
  

•   } else {
  

•           kunit_skip(test, "only useful when debugfs is enabled");
  

•   }
  

+}

+static struct kunit_case kunit_log_test_cases[] = {

•   KUNIT_CASE(kunit_log_test),
  

•   KUNIT_CASE(kunit_log_newline_test),
  

•   {}
  

+};

+static struct kunit_suite kunit_log_test_suite = {

•   .name = "kunit-log-test",
  

•   .test_cases = kunit_log_test_cases,
  

+};

+kunit_test_suites(&kunit_log_test_suite); diff --git a/lib/kunit/test.c b/lib/kunit/test.c index 4b69d12dfc96..14e889e80129 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -109,51 +109,17 @@ static void kunit_print_test_stats(struct kunit *test, stats.total); }

-/**

• • kunit_log_newline() - Add newline to the end of log if one is not
• • already present.
• • @log: The log to add the newline to.
• */

-static void kunit_log_newline(char *log) -{

•   int log_len, len_left;
  

•   log_len = strlen(log);
  

•   len_left = KUNIT_LOG_SIZE - log_len - 1;
  

•   if (log_len > 0 && log[log_len - 1] != '\n')
  

•           strncat(log, "\n", len_left);
  

-}

-/*

• • Append formatted message to log, size of which is limited to
• • KUNIT_LOG_SIZE bytes (including null terminating byte).
• */

-void kunit_log_append(char *log, const char *fmt, ...) +/* Append formatted message to log. */ +void kunit_log_append(struct string_stream *log, const char *fmt, ...) { va_list args;

•   int len, log_len, len_left;
  
    if (!log)
            return;
  
  

•   log_len = strlen(log);
  

•   len_left = KUNIT_LOG_SIZE - log_len - 1;
  

•   if (len_left <= 0)
  

•           return;
  

•   /* Evaluate length of line to add to log */
    va_start(args, fmt);
  

•   len = vsnprintf(NULL, 0, fmt, args) + 1;
  

•   string_stream_vadd(log, fmt, args);
    va_end(args);
  

•   /* Print formatted line to the log */
  

•   va_start(args, fmt);
  

•   vsnprintf(log + log_len, min(len, len_left), fmt, args);
  

•   va_end(args);
  

•   /* Add newline to end of log if not already present. */
  

•   kunit_log_newline(log);
  

} EXPORT_SYMBOL_GPL(kunit_log_append);

@@ -359,14 +325,14 @@ void __kunit_do_failed_assertion(struct kunit *test, } EXPORT_SYMBOL_GPL(__kunit_do_failed_assertion);

-void kunit_init_test(struct kunit *test, const char *name, char *log) +void kunit_init_test(struct kunit *test, const char *name, struct string_stream *log) { spin_lock_init(&test->lock); INIT_LIST_HEAD(&test->resources); test->name = name; test->log = log; if (test->log)

•           test->log[0] = '\0';
  

•           string_stream_clear(log);
    test->status = KUNIT_SUCCESS;
    test->status_comment[0] = '\0';
  

}

2.30.2

On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald rf@opensource.cirrus.com wrote:

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

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

This allows changes in the string_stream implementation to be compared.

Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com

Thanks, this is very useful.

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

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

Reviewed-by: David Gow davidgow@google.com

Cheers, -- David

lib/kunit/string-stream-test.c | 54 ++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+)

diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c index 05bfade2bd8a..b55cc14f43fb 100644 --- a/lib/kunit/string-stream-test.c +++ b/lib/kunit/string-stream-test.c @@ -8,7 +8,9 @@

#include <kunit/static_stub.h> #include <kunit/test.h> +#include <linux/ktime.h> #include <linux/slab.h> +#include <linux/timekeeping.h>

#include "string-stream.h"

@@ -370,6 +372,57 @@ static void string_stream_auto_newline_test(struct kunit *test) "One\nTwo\nThree\nFour\nFive\nSix\nSeven\n\nEight\n"); }

+/*

• • This doesn't actually "test" anything. It reports time taken
• • and memory used for logging a large number of lines.
• */

+static void string_stream_performance_test(struct kunit *test) +{

•   struct string_stream_fragment *frag_container;
  

•   struct string_stream *stream;
  

•   char test_line[101];
  

•   ktime_t start_time, end_time;
  

•   size_t len, bytes_requested, actual_bytes_used, total_string_length;
  

•   int offset, i;
  

•   stream = alloc_string_stream(test, GFP_KERNEL);
  

•   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stream);
  

•   memset(test_line, 'x', sizeof(test_line) - 1);
  

•   test_line[sizeof(test_line) - 1] = '\0';
  

•   start_time = ktime_get();
  

•   for (i = 0; i < 10000; i++) {
  

•           offset = i % (sizeof(test_line) - 1);
  

•           string_stream_add(stream, "%s: %d\n", &test_line[offset], i);
  

•   }
  

•   end_time = ktime_get();
  

•   /*
  

•    * Calculate memory used. This doesn't include invisible
  

•    * overhead due to kernel allocator fragment size rounding.
  

•    */
  

•   bytes_requested = sizeof(*stream);
  

•   actual_bytes_used = ksize(stream);
  

•   total_string_length = 0;
  

•   list_for_each_entry(frag_container, &stream->fragments, node) {
  

•           bytes_requested += sizeof(*frag_container);
  

•           actual_bytes_used += ksize(frag_container);
  

•           len = strlen(frag_container->fragment);
  

•           total_string_length += len;
  

•           bytes_requested += len + 1; /* +1 for '\0' */
  

•           actual_bytes_used += ksize(frag_container->fragment);
  

•   }
  

•   kunit_info(test, "Time elapsed:           %lld us\n",
  

•              ktime_us_delta(end_time, start_time));
  

•   kunit_info(test, "Total string length:    %zu\n", total_string_length);
  

•   kunit_info(test, "Bytes requested:        %zu\n", bytes_requested);
  

•   kunit_info(test, "Actual bytes allocated: %zu\n", actual_bytes_used);
  

+}

static int string_stream_test_init(struct kunit *test) { struct string_stream_test_priv *priv; @@ -400,6 +453,7 @@ static struct kunit_case string_stream_test_cases[] = { KUNIT_CASE(string_stream_append_empty_string_test), KUNIT_CASE(string_stream_no_auto_newline_test), KUNIT_CASE(string_stream_auto_newline_test),

•   KUNIT_CASE(string_stream_performance_test),
    {}
  

};

-- 2.30.2