[v3,1/7] kunit: Replace fixed-size log with dynamically-extending buffer
Commit Message
Re-implement the log buffer as a list of buffer fragments that can
be extended as the size of the log info grows.
When using parameterization the test case can run many times and create
a large amount of log. It's not really practical to keep increasing the
size of the fixed buffer every time a test needs more space. And a big
fixed buffer wastes memory.
The original char *log pointer is replaced by a pointer to a list of
struct kunit_log_frag, each containing a fixed-size buffer.
kunit_log_append() now attempts to append to the last kunit_log_frag in
the list. If there isn't enough space it will append a new kunit_log_frag
to the list. This simple implementation does not attempt to completely
fill the buffer in every kunit_log_frag.
The 'log' member of kunit_suite, kunit_test_case and kunit_suite must be a
pointer because the API of kunit_log() requires that is the same type in
all three structs. As kunit.log is a pointer to the 'log' of the current
kunit_case, it must be a pointer in the other two structs.
The existing kunit-test.c log tests have been updated to build against the
new fragmented log implementation. If the test fails the new function
get_concatenated_log() constructs a single contiguous string from the
log fragments so that the whole log can be emitted in the failure
message.
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
include/kunit/test.h | 25 +++++++++++-----
lib/kunit/debugfs.c | 65 ++++++++++++++++++++++++++++++++++--------
lib/kunit/kunit-test.c | 48 +++++++++++++++++++++++++------
lib/kunit/test.c | 63 ++++++++++++++++++++++++++++------------
4 files changed, 155 insertions(+), 46 deletions(-)
Comments
On Wed, Aug 9, 2023 at 11:54 AM Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> Re-implement the log buffer as a list of buffer fragments that can
> be extended as the size of the log info grows.
>
> When using parameterization the test case can run many times and create
> a large amount of log. It's not really practical to keep increasing the
> size of the fixed buffer every time a test needs more space. And a big
> fixed buffer wastes memory.
>
> The original char *log pointer is replaced by a pointer to a list of
> struct kunit_log_frag, each containing a fixed-size buffer.
>
> kunit_log_append() now attempts to append to the last kunit_log_frag in
> the list. If there isn't enough space it will append a new kunit_log_frag
> to the list. This simple implementation does not attempt to completely
> fill the buffer in every kunit_log_frag.
>
> The 'log' member of kunit_suite, kunit_test_case and kunit_suite must be a
> pointer because the API of kunit_log() requires that is the same type in
> all three structs. As kunit.log is a pointer to the 'log' of the current
> kunit_case, it must be a pointer in the other two structs.
>
> The existing kunit-test.c log tests have been updated to build against the
> new fragmented log implementation. If the test fails the new function
> get_concatenated_log() constructs a single contiguous string from the
> log fragments so that the whole log can be emitted in the failure
> message.
Hello!
All the tests now pass for me and this patch now looks good to me. I
have tested it and it seems to be working well.
I just have a few nits below. But I am overall happy with this patch.
Reviewed-by: Rae Moar <rmoar@google.com>
-Rae
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
> include/kunit/test.h | 25 +++++++++++-----
> lib/kunit/debugfs.c | 65 ++++++++++++++++++++++++++++++++++--------
> lib/kunit/kunit-test.c | 48 +++++++++++++++++++++++++------
> lib/kunit/test.c | 63 ++++++++++++++++++++++++++++------------
> 4 files changed, 155 insertions(+), 46 deletions(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index d33114097d0d..cb5082efc91c 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -33,8 +33,8 @@ DECLARE_STATIC_KEY_FALSE(kunit_running);
>
> struct kunit;
>
> -/* Size of log associated with test. */
> -#define KUNIT_LOG_SIZE 2048
> +/* Size of log buffer fragments. */
> +#define KUNIT_LOG_FRAGMENT_SIZE (256 - sizeof(struct list_head))
>
> /* Maximum size of parameter description string. */
> #define KUNIT_PARAM_DESC_SIZE 128
> @@ -85,6 +85,17 @@ struct kunit_attributes {
> enum kunit_speed speed;
> };
>
> +struct kunit_log_frag {
> + struct list_head list;
> + char buf[KUNIT_LOG_FRAGMENT_SIZE];
> +};
> +
> +static inline void kunit_init_log_frag(struct kunit_log_frag *frag)
> +{
> + INIT_LIST_HEAD(&frag->list);
> + frag->buf[0] = '\0';
> +}
> +
> /**
> * struct kunit_case - represents an individual test case.
> *
> @@ -132,7 +143,7 @@ struct kunit_case {
> /* private: internal use only. */
> enum kunit_status status;
> char *module_name;
> - char *log;
> + struct list_head *log;
> };
>
> static inline char *kunit_status_to_ok_not_ok(enum kunit_status status)
> @@ -252,7 +263,7 @@ struct kunit_suite {
> /* private: internal use only */
> char status_comment[KUNIT_STATUS_COMMENT_SIZE];
> struct dentry *debugfs;
> - char *log;
> + struct list_head *log;
> int suite_init_err;
> };
>
> @@ -278,7 +289,7 @@ struct kunit {
>
> /* private: internal use only. */
> const char *name; /* Read only after initialization! */
> - char *log; /* Points at case log after initialization */
> + struct list_head *log; /* Points at case log after initialization */
To respond to some of the comments from the previous version, I am
fine with the list_head being a pointer considering there can only be
one list_head in this struct definition.
> struct kunit_try_catch try_catch;
> /* param_value is the current parameter value for a test case. */
> const void *param_value;
> @@ -314,7 +325,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 list_head *log);
>
> int kunit_run_tests(struct kunit_suite *suite);
>
> @@ -472,7 +483,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 list_head *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..a26b6d31bd2f 100644
> --- a/lib/kunit/debugfs.c
> +++ b/lib/kunit/debugfs.c
> @@ -5,6 +5,7 @@
> */
>
> #include <linux/debugfs.h>
> +#include <linux/list.h>
> #include <linux/module.h>
>
> #include <kunit/test.h>
> @@ -37,14 +38,15 @@ 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_log(struct seq_file *seq, const struct list_head *log)
> {
> - if (!test_case || !test_case->log)
> + struct kunit_log_frag *frag;
> +
> + if (!log)
> return;
>
> - seq_printf(seq, "%s", test_case->log);
> + list_for_each_entry(frag, log, list)
> + seq_puts(seq, frag->buf);
> }
>
> /*
> @@ -69,10 +71,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_log(seq, test_case->log);
>
> - if (suite->log)
> - seq_printf(seq, "%s", suite->log);
> + debugfs_print_log(seq, suite->log);
>
> seq_printf(seq, "%s %d %s\n",
> kunit_status_to_ok_not_ok(success), 1, suite->name);
> @@ -100,14 +101,53 @@ static const struct file_operations debugfs_results_fops = {
> .release = debugfs_release,
> };
>
> +static struct list_head *kunit_debugfs_alloc_log(void)
> +{
> + struct list_head *log;
> + struct kunit_log_frag *frag;
> +
> + log = kzalloc(sizeof(*log), GFP_KERNEL);
> + if (!log)
> + return NULL;
> +
> + INIT_LIST_HEAD(log);
> +
> + frag = kmalloc(sizeof(*frag), GFP_KERNEL);
> + if (!frag) {
> + kfree(log);
> + return NULL;
> + }
> +
> + kunit_init_log_frag(frag);
> + list_add_tail(&frag->list, log);
> +
> + return log;
> +}
> +
> +static void kunit_debugfs_free_log(struct list_head *log)
> +{
> + struct kunit_log_frag *frag, *n;
> +
> + if (!log)
> + return;
> +
> + list_for_each_entry_safe(frag, n, log, list) {
> + list_del(&frag->list);
> + kfree(frag);
> + }
> +
> + kfree(log);
> +}
> +
> 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);
> + suite->log = kunit_debugfs_alloc_log();
> +
> kunit_suite_for_each_test_case(suite, test_case)
> - test_case->log = kzalloc(KUNIT_LOG_SIZE, GFP_KERNEL);
> + test_case->log = kunit_debugfs_alloc_log();
>
> suite->debugfs = debugfs_create_dir(suite->name, debugfs_rootdir);
>
> @@ -121,7 +161,8 @@ void kunit_debugfs_destroy_suite(struct kunit_suite *suite)
> struct kunit_case *test_case;
>
> debugfs_remove_recursive(suite->debugfs);
> - kfree(suite->log);
> + kunit_debugfs_free_log(suite->log);
> +
> kunit_suite_for_each_test_case(suite, test_case)
> - kfree(test_case->log);
> + kunit_debugfs_free_log(test_case->log);
> }
> diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
> index 83d8e90ca7a2..a199f83bac67 100644
> --- a/lib/kunit/kunit-test.c
> +++ b/lib/kunit/kunit-test.c
> @@ -530,12 +530,37 @@ static struct kunit_suite kunit_resource_test_suite = {
> .test_cases = kunit_resource_test_cases,
> };
>
> +static char *get_concatenated_log(struct kunit *test, const struct list_head *log)
> +{
> + struct kunit_log_frag *frag;
> + size_t len = 0;
> + char *p;
I wonder if we could change the name of p to be a bit more
descriptive. Maybe concat_log?
> +
> + list_for_each_entry(frag, log, list)
> + len += strlen(frag->buf);
> +
> + len++; /* for terminating '\0' */
> + p = kunit_kzalloc(test, len, GFP_KERNEL);
> +
> + list_for_each_entry(frag, log, list)
> + strlcat(p, frag->buf, len);
> +
> + return p;
> +}
> +
> static void kunit_log_test(struct kunit *test)
> {
> struct kunit_suite suite;
> + struct kunit_log_frag *frag;
>
> - suite.log = kunit_kzalloc(test, KUNIT_LOG_SIZE, GFP_KERNEL);
> + suite.log = kunit_kzalloc(test, sizeof(*suite.log), GFP_KERNEL);
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, suite.log);
> + INIT_LIST_HEAD(suite.log);
This section of the test is pretty dense. I would love to see at least
one comment in this section. Maybe it could be here and say something
like:
"/* Allocate, initialize, and then add the first fragment of log */"
> + frag = kunit_kmalloc(test, sizeof(*frag), GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, frag);
> + kunit_init_log_frag(frag);
> + KUNIT_EXPECT_EQ(test, frag->buf[0], '\0');
> + list_add_tail(&frag->list, suite.log);
>
> kunit_log(KERN_INFO, test, "put this in log.");
> kunit_log(KERN_INFO, test, "this too.");
> @@ -543,14 +568,17 @@ static void kunit_log_test(struct kunit *test)
> kunit_log(KERN_INFO, &suite, "along with this.");
>
> #ifdef CONFIG_KUNIT_DEBUGFS
> + frag = list_first_entry(test->log, struct kunit_log_frag, list);
> KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
> - strstr(test->log, "put this in log."));
> + strstr(frag->buf, "put this in log."));
> KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
> - strstr(test->log, "this too."));
> + strstr(frag->buf, "this too."));
> +
> + frag = list_first_entry(suite.log, struct kunit_log_frag, list);
> KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
> - strstr(suite.log, "add to suite log."));
> + strstr(frag->buf, "add to suite log."));
> KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
> - strstr(suite.log, "along with this."));
> + strstr(frag->buf, "along with this."));
> #else
> KUNIT_EXPECT_NULL(test, test->log);
This test passes when CONFIG_KUNIT_DEBUGFS=n while most of the other
tests are skipped. Should this test be skipped instead?
I'm assuming since the assert/expect statements at the beginning are
run even if CONFIG_KUNIT_DEBUGFS=n, this test should not be skipped
but I just wanted to check.
> #endif
> @@ -558,11 +586,15 @@ static void kunit_log_test(struct kunit *test)
>
> static void kunit_log_newline_test(struct kunit *test)
> {
> + struct kunit_log_frag *frag;
> +
> kunit_info(test, "Add newline\n");
> if (test->log) {
This is a small nit but I would prefer that the if statements to
decide whether CONFIG_KUNIT_DEBUGFS is enabled were uniform. So I
would prefer that we choose between if (test->log) and #ifdef
CONFIG_KUNIT_DEBUGFS. I think we originally chose to use if
(test->log) here to avoid the compile-time #ifdef.
> - 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"));
> + frag = list_first_entry(test->log, struct kunit_log_frag, list);
> + KUNIT_ASSERT_NOT_NULL_MSG(test, strstr(frag->buf, "Add newline\n"),
> + "Missing log line, full log:\n%s",
> + get_concatenated_log(test, test->log));
> + KUNIT_EXPECT_NULL(test, strstr(frag->buf, "Add newline\n\n"));
> } else {
> kunit_skip(test, "only useful when debugfs is enabled");
> }
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 49698a168437..dfe51bc2b387 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -11,6 +11,7 @@
> #include <kunit/test-bug.h>
> #include <kunit/attributes.h>
> #include <linux/kernel.h>
> +#include <linux/list.h>
> #include <linux/module.h>
> #include <linux/moduleparam.h>
> #include <linux/panic.h>
> @@ -114,46 +115,66 @@ static void kunit_print_test_stats(struct kunit *test,
> * already present.
> * @log: The log to add the newline to.
> */
> -static void kunit_log_newline(char *log)
> +static void kunit_log_newline(struct kunit_log_frag *frag)
> {
> int log_len, len_left;
>
> - log_len = strlen(log);
> - len_left = KUNIT_LOG_SIZE - log_len - 1;
> + log_len = strlen(frag->buf);
> + len_left = sizeof(frag->buf) - log_len - 1;
>
> - if (log_len > 0 && log[log_len - 1] != '\n')
> - strncat(log, "\n", len_left);
> + if (log_len > 0 && frag->buf[log_len - 1] != '\n')
> + strncat(frag->buf, "\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, ...)
> +static struct kunit_log_frag *kunit_log_extend(struct list_head *log)
I would love to see just a short comment describing kunit_log_extend
to prevent confusion with kunit_log_append.
> +{
> + struct kunit_log_frag *frag;
> +
> + frag = kmalloc(sizeof(*frag), GFP_KERNEL);
> + if (!frag)
> + return NULL;
> +
> + kunit_init_log_frag(frag);
> + list_add_tail(&frag->list, log);
> +
> + return frag;
> +}
> +
> +/* Append formatted message to log, extending the log buffer if necessary. */
> +void kunit_log_append(struct list_head *log, const char *fmt, ...)
> {
> va_list args;
> + struct kunit_log_frag *frag;
> 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;
> + frag = list_last_entry(log, struct kunit_log_frag, list);
> + log_len = strlen(frag->buf);
> + len_left = sizeof(frag->buf) - log_len - 1;
>
> /* Evaluate length of line to add to log */
> va_start(args, fmt);
> len = vsnprintf(NULL, 0, fmt, args) + 1;
> va_end(args);
>
> + if (len > len_left) {
> + frag = kunit_log_extend(log);
> + if (!frag)
> + return;
> +
> + len_left = sizeof(frag->buf) - 1;
> + log_len = 0;
> + }
> +
> /* Print formatted line to the log */
> va_start(args, fmt);
> - vsnprintf(log + log_len, min(len, len_left), fmt, args);
> + vsnprintf(frag->buf + 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);
> + kunit_log_newline(frag);
> }
> EXPORT_SYMBOL_GPL(kunit_log_append);
>
> @@ -359,14 +380,18 @@ 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 list_head *log)
> {
> spin_lock_init(&test->lock);
> INIT_LIST_HEAD(&test->resources);
> test->name = name;
> test->log = log;
> - if (test->log)
> - test->log[0] = '\0';
> + if (test->log) {
> + struct kunit_log_frag *frag = list_first_entry(test->log,
> + struct kunit_log_frag,
> + list);
> + frag->buf[0] = '\0';
> + }
> test->status = KUNIT_SUCCESS;
> test->status_comment[0] = '\0';
> }
> --
> 2.30.2
>
On 9/8/23 22:10, Rae Moar wrote:
> On Wed, Aug 9, 2023 at 11:54 AM Richard Fitzgerald
> <rf@opensource.cirrus.com> wrote:
>>
>> Re-implement the log buffer as a list of buffer fragments that can
>> be extended as the size of the log info grows.
>>
>> When using parameterization the test case can run many times and create
>> a large amount of log. It's not really practical to keep increasing the
>> size of the fixed buffer every time a test needs more space. And a big
>> fixed buffer wastes memory.
>>
>> The original char *log pointer is replaced by a pointer to a list of
>> struct kunit_log_frag, each containing a fixed-size buffer.
>>
>> kunit_log_append() now attempts to append to the last kunit_log_frag in
>> the list. If there isn't enough space it will append a new kunit_log_frag
>> to the list. This simple implementation does not attempt to completely
>> fill the buffer in every kunit_log_frag.
>>
>> The 'log' member of kunit_suite, kunit_test_case and kunit_suite must be a
>> pointer because the API of kunit_log() requires that is the same type in
>> all three structs. As kunit.log is a pointer to the 'log' of the current
>> kunit_case, it must be a pointer in the other two structs.
>>
>> The existing kunit-test.c log tests have been updated to build against the
>> new fragmented log implementation. If the test fails the new function
>> get_concatenated_log() constructs a single contiguous string from the
>> log fragments so that the whole log can be emitted in the failure
>> message.
>
> Hello!
>
> All the tests now pass for me and this patch now looks good to me. I
> have tested it and it seems to be working well.
>
> I just have a few nits below. But I am overall happy with this patch.
>
> Reviewed-by: Rae Moar <rmoar@google.com>
>
> -Rae
>
...
>> +static char *get_concatenated_log(struct kunit *test, const struct list_head *log)
>> +{
>> + struct kunit_log_frag *frag;
>> + size_t len = 0;
>> + char *p;
>
> I wonder if we could change the name of p to be a bit more
> descriptive. Maybe concat_log?
I'll do that.
>> +
>> + list_for_each_entry(frag, log, list)
>> + len += strlen(frag->buf);
>> +
>> + len++; /* for terminating '\0' */
>> + p = kunit_kzalloc(test, len, GFP_KERNEL);
>> +
>> + list_for_each_entry(frag, log, list)
>> + strlcat(p, frag->buf, len);
>> +
>> + return p;
>> +}
>> +
>> static void kunit_log_test(struct kunit *test)
>> {
>> struct kunit_suite suite;
>> + struct kunit_log_frag *frag;
>>
>> - suite.log = kunit_kzalloc(test, KUNIT_LOG_SIZE, GFP_KERNEL);
>> + suite.log = kunit_kzalloc(test, sizeof(*suite.log), GFP_KERNEL);
>> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, suite.log);
>> + INIT_LIST_HEAD(suite.log);
>
> This section of the test is pretty dense. I would love to see at least
> one comment in this section. Maybe it could be here and say something
> like:
> "/* Allocate, initialize, and then add the first fragment of log */"
Good. You like comments. So do I.
But many people hate them so when I'm starting on a new subsystem I go
with however the existing code looks. And the bits of kunit I've looked
at are light on comments.
...
>> @@ -543,14 +568,17 @@ static void kunit_log_test(struct kunit *test)
>> kunit_log(KERN_INFO, &suite, "along with this.");
>>
>> #ifdef CONFIG_KUNIT_DEBUGFS
>> + frag = list_first_entry(test->log, struct kunit_log_frag, list);
>> KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
>> - strstr(test->log, "put this in log."));
>> + strstr(frag->buf, "put this in log."));
>> KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
>> - strstr(test->log, "this too."));
>> + strstr(frag->buf, "this too."));
>> +
>> + frag = list_first_entry(suite.log, struct kunit_log_frag, list);
>> KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
>> - strstr(suite.log, "add to suite log."));
>> + strstr(frag->buf, "add to suite log."));
>> KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
>> - strstr(suite.log, "along with this."));
>> + strstr(frag->buf, "along with this."));
>> #else
>> KUNIT_EXPECT_NULL(test, test->log);
>
> This test passes when CONFIG_KUNIT_DEBUGFS=n while most of the other
> tests are skipped. Should this test be skipped instead?
>
> I'm assuming since the assert/expect statements at the beginning are
> run even if CONFIG_KUNIT_DEBUGFS=n, this test should not be skipped
> but I just wanted to check.
That the existing code, which I didn't change.
I think the best thing here would be to move the fragment testing into a
new test case.
>> #endif
>> @@ -558,11 +586,15 @@ static void kunit_log_test(struct kunit *test)
>>
>> static void kunit_log_newline_test(struct kunit *test)
>> {
>> + struct kunit_log_frag *frag;
>> +
>> kunit_info(test, "Add newline\n");
>> if (test->log) {
>
> This is a small nit but I would prefer that the if statements to
> decide whether CONFIG_KUNIT_DEBUGFS is enabled were uniform. So I
> would prefer that we choose between if (test->log) and #ifdef
> CONFIG_KUNIT_DEBUGFS. I think we originally chose to use if
> (test->log) here to avoid the compile-time #ifdef.
Actually the existing code did it both ways. The newline test used
if (test->log) - but this makes sense because it actually does testing
on test->log.
The original kunit_log_test() used #ifdef CONFIG_KUNIT_DEBUGFS. I based
my new functions on kunit_log_test(). But I can change them. Would you
prefer
if (!test->log)
skip
or
if (!IS_ENABLED(CONFIG_KUNIT_DEBUGFS))
skip
@@ -33,8 +33,8 @@ DECLARE_STATIC_KEY_FALSE(kunit_running);
struct kunit;
-/* Size of log associated with test. */
-#define KUNIT_LOG_SIZE 2048
+/* Size of log buffer fragments. */
+#define KUNIT_LOG_FRAGMENT_SIZE (256 - sizeof(struct list_head))
/* Maximum size of parameter description string. */
#define KUNIT_PARAM_DESC_SIZE 128
@@ -85,6 +85,17 @@ struct kunit_attributes {
enum kunit_speed speed;
};
+struct kunit_log_frag {
+ struct list_head list;
+ char buf[KUNIT_LOG_FRAGMENT_SIZE];
+};
+
+static inline void kunit_init_log_frag(struct kunit_log_frag *frag)
+{
+ INIT_LIST_HEAD(&frag->list);
+ frag->buf[0] = '\0';
+}
+
/**
* struct kunit_case - represents an individual test case.
*
@@ -132,7 +143,7 @@ struct kunit_case {
/* private: internal use only. */
enum kunit_status status;
char *module_name;
- char *log;
+ struct list_head *log;
};
static inline char *kunit_status_to_ok_not_ok(enum kunit_status status)
@@ -252,7 +263,7 @@ struct kunit_suite {
/* private: internal use only */
char status_comment[KUNIT_STATUS_COMMENT_SIZE];
struct dentry *debugfs;
- char *log;
+ struct list_head *log;
int suite_init_err;
};
@@ -278,7 +289,7 @@ struct kunit {
/* private: internal use only. */
const char *name; /* Read only after initialization! */
- char *log; /* Points at case log after initialization */
+ struct list_head *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 +325,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 list_head *log);
int kunit_run_tests(struct kunit_suite *suite);
@@ -472,7 +483,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 list_head *log, const char *fmt, ...);
/**
* kunit_mark_skipped() - Marks @test_or_suite as skipped
@@ -5,6 +5,7 @@
*/
#include <linux/debugfs.h>
+#include <linux/list.h>
#include <linux/module.h>
#include <kunit/test.h>
@@ -37,14 +38,15 @@ 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_log(struct seq_file *seq, const struct list_head *log)
{
- if (!test_case || !test_case->log)
+ struct kunit_log_frag *frag;
+
+ if (!log)
return;
- seq_printf(seq, "%s", test_case->log);
+ list_for_each_entry(frag, log, list)
+ seq_puts(seq, frag->buf);
}
/*
@@ -69,10 +71,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_log(seq, test_case->log);
- if (suite->log)
- seq_printf(seq, "%s", suite->log);
+ debugfs_print_log(seq, suite->log);
seq_printf(seq, "%s %d %s\n",
kunit_status_to_ok_not_ok(success), 1, suite->name);
@@ -100,14 +101,53 @@ static const struct file_operations debugfs_results_fops = {
.release = debugfs_release,
};
+static struct list_head *kunit_debugfs_alloc_log(void)
+{
+ struct list_head *log;
+ struct kunit_log_frag *frag;
+
+ log = kzalloc(sizeof(*log), GFP_KERNEL);
+ if (!log)
+ return NULL;
+
+ INIT_LIST_HEAD(log);
+
+ frag = kmalloc(sizeof(*frag), GFP_KERNEL);
+ if (!frag) {
+ kfree(log);
+ return NULL;
+ }
+
+ kunit_init_log_frag(frag);
+ list_add_tail(&frag->list, log);
+
+ return log;
+}
+
+static void kunit_debugfs_free_log(struct list_head *log)
+{
+ struct kunit_log_frag *frag, *n;
+
+ if (!log)
+ return;
+
+ list_for_each_entry_safe(frag, n, log, list) {
+ list_del(&frag->list);
+ kfree(frag);
+ }
+
+ kfree(log);
+}
+
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);
+ suite->log = kunit_debugfs_alloc_log();
+
kunit_suite_for_each_test_case(suite, test_case)
- test_case->log = kzalloc(KUNIT_LOG_SIZE, GFP_KERNEL);
+ test_case->log = kunit_debugfs_alloc_log();
suite->debugfs = debugfs_create_dir(suite->name, debugfs_rootdir);
@@ -121,7 +161,8 @@ void kunit_debugfs_destroy_suite(struct kunit_suite *suite)
struct kunit_case *test_case;
debugfs_remove_recursive(suite->debugfs);
- kfree(suite->log);
+ kunit_debugfs_free_log(suite->log);
+
kunit_suite_for_each_test_case(suite, test_case)
- kfree(test_case->log);
+ kunit_debugfs_free_log(test_case->log);
}
@@ -530,12 +530,37 @@ static struct kunit_suite kunit_resource_test_suite = {
.test_cases = kunit_resource_test_cases,
};
+static char *get_concatenated_log(struct kunit *test, const struct list_head *log)
+{
+ struct kunit_log_frag *frag;
+ size_t len = 0;
+ char *p;
+
+ list_for_each_entry(frag, log, list)
+ len += strlen(frag->buf);
+
+ len++; /* for terminating '\0' */
+ p = kunit_kzalloc(test, len, GFP_KERNEL);
+
+ list_for_each_entry(frag, log, list)
+ strlcat(p, frag->buf, len);
+
+ return p;
+}
+
static void kunit_log_test(struct kunit *test)
{
struct kunit_suite suite;
+ struct kunit_log_frag *frag;
- suite.log = kunit_kzalloc(test, KUNIT_LOG_SIZE, GFP_KERNEL);
+ suite.log = kunit_kzalloc(test, sizeof(*suite.log), GFP_KERNEL);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, suite.log);
+ INIT_LIST_HEAD(suite.log);
+ frag = kunit_kmalloc(test, sizeof(*frag), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, frag);
+ kunit_init_log_frag(frag);
+ KUNIT_EXPECT_EQ(test, frag->buf[0], '\0');
+ list_add_tail(&frag->list, suite.log);
kunit_log(KERN_INFO, test, "put this in log.");
kunit_log(KERN_INFO, test, "this too.");
@@ -543,14 +568,17 @@ static void kunit_log_test(struct kunit *test)
kunit_log(KERN_INFO, &suite, "along with this.");
#ifdef CONFIG_KUNIT_DEBUGFS
+ frag = list_first_entry(test->log, struct kunit_log_frag, list);
KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
- strstr(test->log, "put this in log."));
+ strstr(frag->buf, "put this in log."));
KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
- strstr(test->log, "this too."));
+ strstr(frag->buf, "this too."));
+
+ frag = list_first_entry(suite.log, struct kunit_log_frag, list);
KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
- strstr(suite.log, "add to suite log."));
+ strstr(frag->buf, "add to suite log."));
KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
- strstr(suite.log, "along with this."));
+ strstr(frag->buf, "along with this."));
#else
KUNIT_EXPECT_NULL(test, test->log);
#endif
@@ -558,11 +586,15 @@ static void kunit_log_test(struct kunit *test)
static void kunit_log_newline_test(struct kunit *test)
{
+ struct kunit_log_frag *frag;
+
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"));
+ frag = list_first_entry(test->log, struct kunit_log_frag, list);
+ KUNIT_ASSERT_NOT_NULL_MSG(test, strstr(frag->buf, "Add newline\n"),
+ "Missing log line, full log:\n%s",
+ get_concatenated_log(test, test->log));
+ KUNIT_EXPECT_NULL(test, strstr(frag->buf, "Add newline\n\n"));
} else {
kunit_skip(test, "only useful when debugfs is enabled");
}
@@ -11,6 +11,7 @@
#include <kunit/test-bug.h>
#include <kunit/attributes.h>
#include <linux/kernel.h>
+#include <linux/list.h>
#include <linux/module.h>
#include <linux/moduleparam.h>
#include <linux/panic.h>
@@ -114,46 +115,66 @@ static void kunit_print_test_stats(struct kunit *test,
* already present.
* @log: The log to add the newline to.
*/
-static void kunit_log_newline(char *log)
+static void kunit_log_newline(struct kunit_log_frag *frag)
{
int log_len, len_left;
- log_len = strlen(log);
- len_left = KUNIT_LOG_SIZE - log_len - 1;
+ log_len = strlen(frag->buf);
+ len_left = sizeof(frag->buf) - log_len - 1;
- if (log_len > 0 && log[log_len - 1] != '\n')
- strncat(log, "\n", len_left);
+ if (log_len > 0 && frag->buf[log_len - 1] != '\n')
+ strncat(frag->buf, "\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, ...)
+static struct kunit_log_frag *kunit_log_extend(struct list_head *log)
+{
+ struct kunit_log_frag *frag;
+
+ frag = kmalloc(sizeof(*frag), GFP_KERNEL);
+ if (!frag)
+ return NULL;
+
+ kunit_init_log_frag(frag);
+ list_add_tail(&frag->list, log);
+
+ return frag;
+}
+
+/* Append formatted message to log, extending the log buffer if necessary. */
+void kunit_log_append(struct list_head *log, const char *fmt, ...)
{
va_list args;
+ struct kunit_log_frag *frag;
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;
+ frag = list_last_entry(log, struct kunit_log_frag, list);
+ log_len = strlen(frag->buf);
+ len_left = sizeof(frag->buf) - log_len - 1;
/* Evaluate length of line to add to log */
va_start(args, fmt);
len = vsnprintf(NULL, 0, fmt, args) + 1;
va_end(args);
+ if (len > len_left) {
+ frag = kunit_log_extend(log);
+ if (!frag)
+ return;
+
+ len_left = sizeof(frag->buf) - 1;
+ log_len = 0;
+ }
+
/* Print formatted line to the log */
va_start(args, fmt);
- vsnprintf(log + log_len, min(len, len_left), fmt, args);
+ vsnprintf(frag->buf + 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);
+ kunit_log_newline(frag);
}
EXPORT_SYMBOL_GPL(kunit_log_append);
@@ -359,14 +380,18 @@ 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 list_head *log)
{
spin_lock_init(&test->lock);
INIT_LIST_HEAD(&test->resources);
test->name = name;
test->log = log;
- if (test->log)
- test->log[0] = '\0';
+ if (test->log) {
+ struct kunit_log_frag *frag = list_first_entry(test->log,
+ struct kunit_log_frag,
+ list);
+ frag->buf[0] = '\0';
+ }
test->status = KUNIT_SUCCESS;
test->status_comment[0] = '\0';
}