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

[David Gow]

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

Looks pretty good to me, and works fine here.

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

Otherwise,
Reviewed-by: David Gow <davidgow@xxxxxxxxxx>

Cheers,
-- David

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

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature