Re: [PATCH] seq_buf: Introduce KUnit tests

[David Gow <davidgow@google.com>]

[-- Attachment #1: Type: text/plain, Size: 11175 bytes --]

On Thu, 17 Jul 2025 at 16:52, Kees Cook <kees@kernel.org> wrote:
>
> Add KUnit tests for the seq_buf API to ensure its correctness and
> prevent future regressions.
>
> The tests cover the following functions:
> - seq_buf_init()
> - DECLARE_SEQ_BUF()
> - seq_buf_clear()
> - seq_buf_puts()
> - seq_buf_putc()
> - seq_buf_printf()
> - seq_buf_get_buf()
> - seq_buf_commit()
>
> $ tools/testing/kunit/kunit.py run seq_buf
> =================== seq_buf (9 subtests) ===================
> [PASSED] seq_buf_init_test
> [PASSED] seq_buf_declare_test
> [PASSED] seq_buf_clear_test
> [PASSED] seq_buf_puts_test
> [PASSED] seq_buf_puts_overflow_test
> [PASSED] seq_buf_putc_test
> [PASSED] seq_buf_printf_test
> [PASSED] seq_buf_printf_overflow_test
> [PASSED] seq_buf_get_buf_commit_test
> ===================== [PASSED] seq_buf =====================
>
> Signed-off-by: Kees Cook <kees@kernel.org>
> ---
> I used an LLM to produce this; it did pretty well, but I had to help it
> get the Kconfig and make targets in the right places, and I tweaked some
> of the edge cases and added a bit more (perhaps redundant) state checking.
>

This patch looks good to me. A couple of very minor notes below, but
nothing I think is a big issue.

I don't have much to say on the LLM use -- it definitely looks better
than the last time I tried it. If something like Sasha's suggestion
for how to tag LLM contributions[1] goes through, I do think noting
what was used could be a useful thing to have.

Reviewed-by: David Gow <davidgow@google.com>

-- David

[1] https://lore.kernel.org/all/20250725175358.1989323-1-sashal@kernel.org/

> Cc: David Gow <davidgow@google.com>
> Cc: Rae Moar <rmoar@google.com>
> Cc: Tamir Duberstein <tamird@gmail.com>
> Cc: Eric Biggers <ebiggers@kernel.org>
> Cc: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Petr Mladek <pmladek@suse.com>
> Cc: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> ---
>  lib/Kconfig.debug         |   9 ++
>  lib/tests/Makefile        |   1 +
>  lib/tests/seq_buf_kunit.c | 205 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 215 insertions(+)
>  create mode 100644 lib/tests/seq_buf_kunit.c
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index cf05bf1df983..048efc3183d5 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2470,6 +2470,15 @@ config SCANF_KUNIT_TEST
>
>           If unsure, say N.
>
> +config SEQ_BUF_KUNIT_TEST
> +       tristate "KUnit test for seq_buf" if !KUNIT_ALL_TESTS
> +       depends on KUNIT
> +       default KUNIT_ALL_TESTS
> +       help
> +         This builds unit tests for the seq_buf library.
> +
> +         If unsure, say N.
> +

FYI: Checkpatch complains a bit that this is too short a description.
It doesn't bother me much, though:
WARNING: please write a help paragraph that fully describes the config
symbol with at least 4 lines

>  config STRING_KUNIT_TEST
>         tristate "KUnit test string functions at runtime" if !KUNIT_ALL_TESTS
>         depends on KUNIT
> diff --git a/lib/tests/Makefile b/lib/tests/Makefile
> index 84b15c986b8c..fa6d728a8b5b 100644
> --- a/lib/tests/Makefile
> +++ b/lib/tests/Makefile
> @@ -36,6 +36,7 @@ obj-$(CONFIG_OVERFLOW_KUNIT_TEST) += overflow_kunit.o
>  obj-$(CONFIG_PRINTF_KUNIT_TEST) += printf_kunit.o
>  obj-$(CONFIG_RANDSTRUCT_KUNIT_TEST) += randstruct_kunit.o
>  obj-$(CONFIG_SCANF_KUNIT_TEST) += scanf_kunit.o
> +obj-$(CONFIG_SEQ_BUF_KUNIT_TEST) += seq_buf_kunit.o
>  obj-$(CONFIG_SIPHASH_KUNIT_TEST) += siphash_kunit.o
>  obj-$(CONFIG_SLUB_KUNIT_TEST) += slub_kunit.o
>  obj-$(CONFIG_TEST_SORT) += test_sort.o
> diff --git a/lib/tests/seq_buf_kunit.c b/lib/tests/seq_buf_kunit.c
> new file mode 100644
> index 000000000000..74648dbda13f
> --- /dev/null
> +++ b/lib/tests/seq_buf_kunit.c
> @@ -0,0 +1,205 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit tests for the seq_buf API
> + *
> + * Copyright (C) 2025, Google LLC.
> + */
> +
> +#include <kunit/test.h>
> +#include <linux/seq_buf.h>
> +
> +static void seq_buf_init_test(struct kunit *test)
> +{
> +       char buf[32];
> +       struct seq_buf s;
> +
> +       seq_buf_init(&s, buf, sizeof(buf));
> +
> +       KUNIT_EXPECT_EQ(test, s.size, 32);
> +       KUNIT_EXPECT_EQ(test, s.len, 0);
> +       KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(&s));
> +       KUNIT_EXPECT_EQ(test, seq_buf_buffer_left(&s), 32);
> +       KUNIT_EXPECT_EQ(test, seq_buf_used(&s), 0);
> +       KUNIT_EXPECT_STREQ(test, seq_buf_str(&s), "");
> +}
> +
> +static void seq_buf_declare_test(struct kunit *test)
> +{
> +       DECLARE_SEQ_BUF(s, 24);
> +
> +       KUNIT_EXPECT_EQ(test, s.size, 24);
> +       KUNIT_EXPECT_EQ(test, s.len, 0);
> +       KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(&s));
> +       KUNIT_EXPECT_EQ(test, seq_buf_buffer_left(&s), 24);
> +       KUNIT_EXPECT_EQ(test, seq_buf_used(&s), 0);
> +       KUNIT_EXPECT_STREQ(test, seq_buf_str(&s), "");
> +}
> +
> +static void seq_buf_clear_test(struct kunit *test)
> +{
> +       DECLARE_SEQ_BUF(s, 128);
> +
> +       seq_buf_puts(&s, "hello");
> +       KUNIT_EXPECT_EQ(test, s.len, 5);
> +       KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(&s));

My gut feeling seeing this was that we should test that
seq_buf_clear() also clears the overflow flag, though the separate
overflow tests below all do seem to test that, so it's probably
redundant.

> +       KUNIT_EXPECT_STREQ(test, seq_buf_str(&s), "hello");
> +
> +       seq_buf_clear(&s);
> +
> +       KUNIT_EXPECT_EQ(test, s.len, 0);
> +       KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(&s));
> +       KUNIT_EXPECT_STREQ(test, seq_buf_str(&s), "");
> +}
> +
> +static void seq_buf_puts_test(struct kunit *test)
> +{
> +       DECLARE_SEQ_BUF(s, 16);
> +
> +       seq_buf_puts(&s, "hello");
> +       KUNIT_EXPECT_EQ(test, seq_buf_used(&s), 5);
> +       KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(&s));
> +       KUNIT_EXPECT_STREQ(test, seq_buf_str(&s), "hello");
> +
> +       seq_buf_puts(&s, " world");
> +       KUNIT_EXPECT_EQ(test, seq_buf_used(&s), 11);
> +       KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(&s));
> +       KUNIT_EXPECT_STREQ(test, seq_buf_str(&s), "hello world");
> +}
> +
> +static void seq_buf_puts_overflow_test(struct kunit *test)
> +{
> +       DECLARE_SEQ_BUF(s, 10);
> +
> +       seq_buf_puts(&s, "123456789");
> +       KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(&s));
> +       KUNIT_EXPECT_EQ(test, seq_buf_used(&s), 9);
> +
> +       seq_buf_puts(&s, "0");
> +       KUNIT_EXPECT_TRUE(test, seq_buf_has_overflowed(&s));
> +       KUNIT_EXPECT_EQ(test, seq_buf_used(&s), 10);
> +       KUNIT_EXPECT_STREQ(test, seq_buf_str(&s), "123456789");
> +
> +       seq_buf_clear(&s);
> +       KUNIT_EXPECT_EQ(test, s.len, 0);
> +       KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(&s));
> +       KUNIT_EXPECT_STREQ(test, seq_buf_str(&s), "");
> +}
> +
> +static void seq_buf_putc_test(struct kunit *test)
> +{
> +       DECLARE_SEQ_BUF(s, 4);
> +
> +       seq_buf_putc(&s, 'a');
> +       seq_buf_putc(&s, 'b');
> +       seq_buf_putc(&s, 'c');
> +
> +       KUNIT_EXPECT_EQ(test, seq_buf_used(&s), 3);
> +       KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(&s));
> +       KUNIT_EXPECT_STREQ(test, seq_buf_str(&s), "abc");
> +
> +       seq_buf_putc(&s, 'd');
> +       KUNIT_EXPECT_EQ(test, seq_buf_used(&s), 4);
> +       KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(&s));
> +       KUNIT_EXPECT_STREQ(test, seq_buf_str(&s), "abc");
> +
> +       seq_buf_putc(&s, 'e');
> +       KUNIT_EXPECT_EQ(test, seq_buf_used(&s), 4);
> +       KUNIT_EXPECT_TRUE(test, seq_buf_has_overflowed(&s));
> +       KUNIT_EXPECT_STREQ(test, seq_buf_str(&s), "abc");
> +
> +       seq_buf_clear(&s);
> +       KUNIT_EXPECT_EQ(test, s.len, 0);
> +       KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(&s));
> +       KUNIT_EXPECT_STREQ(test, seq_buf_str(&s), "");
> +}
> +
> +static void seq_buf_printf_test(struct kunit *test)
> +{
> +       DECLARE_SEQ_BUF(s, 32);
> +
> +       seq_buf_printf(&s, "hello %s", "world");
> +       KUNIT_EXPECT_EQ(test, seq_buf_used(&s), 11);
> +       KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(&s));
> +       KUNIT_EXPECT_STREQ(test, seq_buf_str(&s), "hello world");
> +
> +       seq_buf_printf(&s, " %d", 123);
> +       KUNIT_EXPECT_EQ(test, seq_buf_used(&s), 15);
> +       KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(&s));
> +       KUNIT_EXPECT_STREQ(test, seq_buf_str(&s), "hello world 123");
> +}
> +
> +static void seq_buf_printf_overflow_test(struct kunit *test)
> +{
> +       DECLARE_SEQ_BUF(s, 16);
> +
> +       seq_buf_printf(&s, "%lu", 1234567890UL);
> +       KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(&s));
> +       KUNIT_EXPECT_EQ(test, seq_buf_used(&s), 10);
> +       KUNIT_EXPECT_STREQ(test, seq_buf_str(&s), "1234567890");
> +
> +       seq_buf_printf(&s, "%s", "abcdefghij");
> +       KUNIT_EXPECT_TRUE(test, seq_buf_has_overflowed(&s));
> +       KUNIT_EXPECT_EQ(test, seq_buf_used(&s), 16);
> +       KUNIT_EXPECT_STREQ(test, seq_buf_str(&s), "1234567890abcde");
> +
> +       seq_buf_clear(&s);
> +       KUNIT_EXPECT_EQ(test, s.len, 0);
> +       KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(&s));
> +       KUNIT_EXPECT_STREQ(test, seq_buf_str(&s), "");
> +}
> +
> +static void seq_buf_get_buf_commit_test(struct kunit *test)
> +{
> +       DECLARE_SEQ_BUF(s, 16);
> +       char *buf;
> +       size_t len;
> +
> +       len = seq_buf_get_buf(&s, &buf);
> +       KUNIT_EXPECT_EQ(test, len, 16);
> +       KUNIT_EXPECT_PTR_NE(test, buf, NULL);
> +
> +       memcpy(buf, "hello", 5);
> +       seq_buf_commit(&s, 5);
> +
> +       KUNIT_EXPECT_EQ(test, seq_buf_used(&s), 5);
> +       KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(&s));
> +       KUNIT_EXPECT_STREQ(test, seq_buf_str(&s), "hello");
> +
> +       len = seq_buf_get_buf(&s, &buf);
> +       KUNIT_EXPECT_EQ(test, len, 11);
> +       KUNIT_EXPECT_PTR_NE(test, buf, NULL);
> +
> +       memcpy(buf, " worlds!", 8);
> +       seq_buf_commit(&s, 6);
> +
> +       KUNIT_EXPECT_EQ(test, seq_buf_used(&s), 11);
> +       KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(&s));
> +       KUNIT_EXPECT_STREQ(test, seq_buf_str(&s), "hello world");
> +
> +       len = seq_buf_get_buf(&s, &buf);
> +       KUNIT_EXPECT_EQ(test, len, 5);
> +       KUNIT_EXPECT_PTR_NE(test, buf, NULL);
> +
> +       seq_buf_commit(&s, -1);
> +       KUNIT_EXPECT_TRUE(test, seq_buf_has_overflowed(&s));
> +}
> +
> +static struct kunit_case seq_buf_test_cases[] = {
> +       KUNIT_CASE(seq_buf_init_test),
> +       KUNIT_CASE(seq_buf_declare_test),
> +       KUNIT_CASE(seq_buf_clear_test),
> +       KUNIT_CASE(seq_buf_puts_test),
> +       KUNIT_CASE(seq_buf_puts_overflow_test),
> +       KUNIT_CASE(seq_buf_putc_test),
> +       KUNIT_CASE(seq_buf_printf_test),
> +       KUNIT_CASE(seq_buf_printf_overflow_test),
> +       KUNIT_CASE(seq_buf_get_buf_commit_test),
> +       {}
> +};
> +
> +static struct kunit_suite seq_buf_test_suite = {
> +       .name = "seq_buf",
> +       .test_cases = seq_buf_test_cases,
> +};
> +
> +kunit_test_suite(seq_buf_test_suite);
> --
> 2.34.1
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5281 bytes --]