On 9/8/23 22:10, Rae Moar wrote:
On Wed, Aug 9, 2023 at 11:54 AM Richard Fitzgerald
<rf@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
-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
