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

On Thu, 24 Aug 2023 at 22:33, 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.

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@opensource.cirrus.com

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@google.com

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