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."));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