Re: [PATCH v3 2/7] kunit: kunit-test: Add test cases for extending log buffer

On Wed, Aug 9, 2023 at 11:54 AM Richard Fitzgerald rf@opensource.cirrus.com wrote:

Add test cases for the dynamically-extending log buffer.

kunit_log_init_frag_test() tests that kunit_init_log_frag() correctly initializes new struct kunit_log_frag.

kunit_log_extend_test_1() logs a series of numbered lines then tests that the resulting log contains all the lines.

kunit_log_extend_test_2() logs a large number of lines of varying length to create many fragments, then tests that all lines are present.

kunit_log_newline_test() has a new test to append a line that is exactly the length of the available space in the current fragment and check that the resulting log has a trailing '\n'.

Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com

Hello!

These tests now pass for me. Thanks!

I do have a few comments below mostly regarding comments and a few clarifying questions.

-Rae

---

lib/kunit/kunit-test.c | 182 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 174 insertions(+), 8 deletions(-)

diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c index a199f83bac67..c0ee33a8031e 100644 --- a/lib/kunit/kunit-test.c +++ b/lib/kunit/kunit-test.c @@ -7,6 +7,7 @@ */ #include <kunit/test.h> #include <kunit/test-bug.h> +#include <linux/prandom.h>

#include "try-catch-impl.h"

@@ -530,10 +531,12 @@ 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) +static char *get_concatenated_log(struct kunit *test, const struct list_head *log,

•                             int *num_frags)
  

{ struct kunit_log_frag *frag; size_t len = 0;

•   int frag_count = 0;
    char *p;
  
    list_for_each_entry(frag, log, list)
  

@@ -542,24 +545,42 @@ static char *get_concatenated_log(struct kunit *test, const struct list_head *lo len++; /* for terminating '\0' */ p = kunit_kzalloc(test, len, GFP_KERNEL);

•   list_for_each_entry(frag, log, list)
  

•   list_for_each_entry(frag, log, list) {
            strlcat(p, frag->buf, len);
  

•           ++frag_count;
  

•   }
  

•   if (num_frags)
  

•           *num_frags = frag_count;
  
    return p;
  

}

-static void kunit_log_test(struct kunit *test) +static void kunit_log_init_frag_test(struct kunit *test) {

•   struct kunit_suite suite;
    struct kunit_log_frag *frag;
  
  

•   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);
  

•   memset(frag, 0x5a, sizeof(*frag));
  

Why is the fragment getting filled here with memset? Should this be tested? Feel free to let me know, I'm just uncertain.

    kunit_init_log_frag(frag);
    KUNIT_EXPECT_EQ(test, frag->buf[0], '\0');

•   KUNIT_EXPECT_TRUE(test, list_is_first(&frag->list, &frag->list));
  

•   KUNIT_EXPECT_TRUE(test, list_is_last(&frag->list, &frag->list));
  

+}

+static void kunit_log_test(struct kunit *test) +{

•   struct kunit_suite suite;
  

•   struct kunit_log_frag *frag;
  

•   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_kzalloc(test, sizeof(*frag), GFP_KERNEL);
  

•   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, frag);
  

•   kunit_init_log_frag(frag);
    list_add_tail(&frag->list, suite.log);
  
    kunit_log(KERN_INFO, test, "put this in log.");
  

@@ -586,23 +607,168 @@ static void kunit_log_test(struct kunit *test)

static void kunit_log_newline_test(struct kunit *test) {

•   struct kunit_suite suite;
    struct kunit_log_frag *frag;
  

•   char *p;
  

Similar to last email, could we change p to be a more descriptive name such as concat_log?

    kunit_info(test, "Add newline\n");
    if (test->log) {
            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));
  

•                   get_concatenated_log(test, test->log, NULL));
            KUNIT_EXPECT_NULL(test, strstr(frag->buf, "Add newline\n\n"));
  

Should this section of kunit_log_newline_test be separated into a new test? This test seems a bit long and seems to have two distinct sections?

•           suite.log = kunit_kzalloc(test, sizeof(*suite.log), GFP_KERNEL);
  

•           KUNIT_ASSERT_NOT_ERR_OR_NULL(test, suite.log);
  

•           INIT_LIST_HEAD(suite.log);
  

I would love to see a comment here to explain and break up this section similar to the comment from the previous email.

•           frag = kunit_kzalloc(test, sizeof(*frag), GFP_KERNEL);
  

•           KUNIT_ASSERT_NOT_ERR_OR_NULL(test, frag);
  

•           kunit_init_log_frag(frag);
  

•           list_add_tail(&frag->list, suite.log);
  

•           /* String that exactly fills fragment leaving no room for \n */
  

•           memset(frag->buf, 0, sizeof(frag->buf));
  

•           memset(frag->buf, 'x', sizeof(frag->buf) - 9);
  

•           kunit_log_append(suite.log, "12345678");
  

•           p = get_concatenated_log(test, suite.log, NULL);
  

•           KUNIT_ASSERT_NOT_ERR_OR_NULL(test, p);
  

•           KUNIT_EXPECT_NOT_NULL_MSG(test, strstr(p, "x12345678\n"),
  

•                   "Newline not appended when fragment is full. Log is:\n'%s'", p);
    } else {
            kunit_skip(test, "only useful when debugfs is enabled");
    }
  

}

+static void kunit_log_extend_test_1(struct kunit *test)

In general, I would really like to see more comments in the next two tests describing the test behavior. I would prefer a comment for each of the while/do-while loops below. I just found the behavior to be slightly confusing to understand without comments (although I do appreciate the comments that are in kunit_log_extend_test_2).

Also, I really appreciate how detailed these tests are.

Another potential idea is to rename these two tests to be kunit_log_extend_test() and kunit_log_rand_extend_test() instead to be more descriptive?

+{ +#ifdef CONFIG_KUNIT_DEBUGFS

•   struct kunit_suite suite;
  

•   struct kunit_log_frag *frag;
  

•   char line[60];
  

•   char *p, *pn;
  

Similar to before, could we change p and pn to be slightly more descriptive names? Maybe concat_log and newline_ptr or newline_log or newline_char?

•   size_t len, n;
  

•   int num_lines, num_frags, i;
  

•   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_kzalloc(test, sizeof(*frag), GFP_KERNEL);
  

•   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, frag);
  

•   kunit_init_log_frag(frag);
  

•   list_add_tail(&frag->list, suite.log);
  

•   i = 0;
  

•   len = 0;
  

•   do {
  

•           n = snprintf(line, sizeof(line),
  

•                        "The quick brown fox jumps over the lazy penguin %d\n", i);
  

•           KUNIT_ASSERT_LT(test, n, sizeof(line));
  

•           kunit_log_append(suite.log, line);
  

•           ++i;
  

•           len += n;
  

•   }  while (len < (sizeof(frag->buf) * 30));
  

Are we trying to restrict the num_frags to less than 30? And then we could check that with a KUNIT_EXPECT? Currently, the num_frags are just above 30. That is ok too. I just was wondering if this was intentional? (Same as kunit_log_extend_test_2)

•   num_lines = i;
  

•   p = get_concatenated_log(test, suite.log, &num_frags);
  

•   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, p);
  

•   KUNIT_EXPECT_GT(test, num_frags, 1);
  

•   kunit_info(test, "num lines:%d num_frags:%d total len:%zu\n",
  

•              num_lines, num_frags, strlen(p));
  

•   i = 0;
  

•   while ((pn = strchr(p, '\n')) != NULL) {
  

•           *pn = '\0';
  

•           snprintf(line, sizeof(line),
  

•                    "The quick brown fox jumps over the lazy penguin %d", i);
  

•           KUNIT_EXPECT_STREQ(test, p, line);
  

•           p = pn + 1;
  

•           ++i;
  

•   }
  

•   KUNIT_EXPECT_EQ(test, i, num_lines);
  

+#else

•   kunit_skip(test, "only useful when debugfs is enabled");
  

+#endif +}

+static void kunit_log_extend_test_2(struct kunit *test) +{ +#ifdef CONFIG_KUNIT_DEBUGFS

•   struct kunit_suite suite;
  

•   struct kunit_log_frag *frag;
  

•   struct rnd_state rnd;
  

•   char line[101];
  

•   char *p, *pn;
  

Similar to above, could p and pn be renamed to be more descriptive?

•   size_t len;
  

•   int num_lines, num_frags, n, i;
  

•   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_kzalloc(test, sizeof(*frag), GFP_KERNEL);
  

•   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, frag);
  

•   kunit_init_log_frag(frag);
  

•   list_add_tail(&frag->list, suite.log);
  

•   /* Build log line of varying content */
  

•   line[0] = '\0';
  

•   i = 0;
  

•   do {
  

•           char tmp[9];
  

•           snprintf(tmp, sizeof(tmp), "%x", i++);
  

•           len = strlcat(line, tmp, sizeof(line));
  

•   } while (len < sizeof(line) - 1);
  

Could there be an expectation statement here to check the line has been properly filled. Maybe checking the length?

•   /*
  

•    * Log lines of different lengths until we have created
  

•    * many fragments.
  

•    * The "randomness" must be repeatable.
  

•    */
  

•   prandom_seed_state(&rnd, 3141592653589793238ULL);
  

•   i = 0;
  

•   len = 0;
  

•   num_lines = 0;
  

•   do {
  

•           kunit_log_append(suite.log, "%s\n", &line[i]);
  

•           len += sizeof(line) - i;
  

•           num_lines++;
  

•           i = prandom_u32_state(&rnd) % (sizeof(line) - 1);
  

•   } while (len < (sizeof(frag->buf) * 30));
  

•   /* There must be more than one buffer fragment now */
  

•   KUNIT_EXPECT_FALSE(test, list_is_singular(suite.log));
  

•   p = get_concatenated_log(test, suite.log, &num_frags);
  

•   KUNIT_ASSERT_NOT_ERR_OR_NULL(test, p);
  

•   KUNIT_EXPECT_GT(test, num_frags, 1);
  

•   kunit_info(test, "num lines:%d num_frags:%d total len:%zu\n",
  

•              num_lines, num_frags, strlen(p));
  

•   prandom_seed_state(&rnd, 3141592653589793238ULL);
  

•   i = 0;
  

•   n = 0;
  

•   while ((pn = strchr(p, '\n')) != NULL) {
  

•           *pn = '\0';
  

•           KUNIT_EXPECT_STREQ(test, p, &line[i]);
  

•           p = pn + 1;
  

•           n++;
  

•           i = prandom_u32_state(&rnd) % (sizeof(line) - 1);
  

•   }
  

•   KUNIT_EXPECT_EQ_MSG(test, n, num_lines, "Not enough lines.");
  

Is it possible for this to be too many lines instead? Should this comment instead be "Unexpected number of lines". Also could we have a similar message for the test above for this expectation regarding the number of lines.

+#else

•   kunit_skip(test, "only useful when debugfs is enabled");
  

+#endif +}

static struct kunit_case kunit_log_test_cases[] = {

•   KUNIT_CASE(kunit_log_init_frag_test),
    KUNIT_CASE(kunit_log_test),
    KUNIT_CASE(kunit_log_newline_test),
  

•   KUNIT_CASE(kunit_log_extend_test_1),
  

•   KUNIT_CASE(kunit_log_extend_test_2),
    {}
  

};

-- 2.30.2