On Fri, Mar 1, 2024 at 2:26 AM Kees Cook <keesc...@chromium.org> wrote:
>
> Convert test-string_helpers.c to KUnit so it can be easily run with
> everything else.
...
> -#include <linux/array_size.h>
> #include <linux/init.h>
> +#include <kunit/test.h>
I know the order is broken here, but don't make it worse, please. And
stick with one schema where to put kunit/test.h always before
everything else and somewhere else (like always after linux/*).
> +#include <linux/array_size.h>
> #include <linux/kernel.h>
> #include <linux/slab.h>
> #include <linux/module.h>
...
> +static void test_string_check_buf(struct kunit *test,
> + const char *name, unsigned int flags,
> + char *in, size_t p,
> + char *out_real, size_t q_real,
> + char *out_test, size_t q_test)
> {
> - if (q_real == q_test && !memcmp(out_test, out_real, q_test))
> - return true;
> + int result;
> +
> + KUNIT_EXPECT_EQ(test, q_real, q_test);
This needs a message.
> + result = memcmp(out_test, out_real, q_test);
> + KUNIT_EXPECT_EQ(test, 0, result);
Why do we need this assertion? We have a dump below to show what's wrong.
> + if (q_real == q_test && result == 0)
> + return;
I'm not sure this is an equivalent change. IIRC KUnit assertions do
not continue on failure. (Long time last time I run KUnit test)
>
> pr_warn("Test '%s' failed: flags = %#x\n", name, flags);
>
> @@ -28,8 +37,6 @@ static __init bool test_string_check_buf(const char *name,
> unsigned int flags,
> out_test, q_test, true);
> print_hex_dump(KERN_WARNING, "Got: ", DUMP_PREFIX_NONE, 16, 1,
> out_real, q_real, true);
> -
> - return false;
> }
...
> +static void
> +test_string_escape_overflow(struct kunit *test,
> + const char *in, int p, unsigned int flags, const
> char *esc,
> int q_test, const char *name)
> {
> int q_real;
>
> q_real = string_escape_mem(in, p, NULL, 0, flags, esc);
> - if (q_real != q_test)
> - pr_warn("Test '%s' failed: flags = %#x, osz = 0, expected %d,
> got %d\n",
> - name, flags, q_test, q_real);
> + KUNIT_EXPECT_EQ(test, q_real, q_test);
You killed the message, not good.
> }
...
> +#define test_string_get_size_one(size, blk_size, exp_result10, exp_result2)
> \
> + do {
> \
> + BUILD_BUG_ON(sizeof(exp_result10) >= string_get_size_maxbuf);
> \
> + BUILD_BUG_ON(sizeof(exp_result2) >= string_get_size_maxbuf);
> \
No analogous assertions in KUnit?
> + __test_string_get_size(test, (size), (blk_size),
> (exp_result10), \
> + (exp_result2));
> \
> } while (0)
...
> {
> - if (!memcmp(res, exp, strlen(exp) + 1))
> + int result = memcmp(res, exp, strlen(exp) + 1);
> + KUNIT_EXPECT_EQ(test, 0, result);
As per above, what's the added value of this assertion?
> + if (!result)
> return;
...
> @@ -590,65 +604,68 @@ static void __init test_string_upper_lower(void)
> for (i = 0; i < ARRAY_SIZE(strings_upper); i++) {
> const char *s = strings_upper[i].in;
> int len = strlen(strings_upper[i].in) + 1;
> + int result;
>
> dst = kmalloc(len, GFP_KERNEL);
> - if (!dst)
> - return;
> + KUNIT_ASSERT_NOT_NULL(test, dst);
>
> string_upper(dst, s);
> - if (memcmp(dst, strings_upper[i].out, len)) {
> + result = memcmp(dst, strings_upper[i].out, len);
> + KUNIT_EXPECT_EQ(test, 0, result);
Ditto.
> + if (result)
> pr_warn("Test 'string_upper' failed : expected %s,
> got %s!\n",
> strings_upper[i].out, dst);
> - kfree(dst);
> - return;
> - }
> kfree(dst);
> }
>
> for (i = 0; i < ARRAY_SIZE(strings_lower); i++) {
> const char *s = strings_lower[i].in;
> int len = strlen(strings_lower[i].in) + 1;
> + int result;
>
> dst = kmalloc(len, GFP_KERNEL);
> - if (!dst)
> - return;
> + KUNIT_ASSERT_NOT_NULL(test, dst);
>
> string_lower(dst, s);
> - if (memcmp(dst, strings_lower[i].out, len)) {
> + result = memcmp(dst, strings_lower[i].out, len);
> + KUNIT_EXPECT_EQ(test, 0, result);
Ditto.
> + if (result)
> pr_warn("Test 'string_lower failed : : expected %s,
> got %s!\n",
> strings_lower[i].out, dst);
> - kfree(dst);
> - return;
> - }
> kfree(dst);
> }
> }
--
With Best Regards,
Andy Shevchenko
