Re: [PATCH v2 2/2] string: Convert helpers selftest to KUnit

[Kees Cook]

On Fri, Mar 01, 2024 at 01:20:41PM +0200, Andy Shevchenko wrote:
> On Fri, Mar 1, 2024 at 2:26 AM Kees Cook <keescook@xxxxxxxxxxxx> 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/*).

Fixed.

> 
> > +#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.

I've improved this all around with ...STREQ and ...MEMEQ calls, and
added _MSG versions where needed to retain the prior reported test
context.

> ...
> 
> > +#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?

It's designed for runtime testing. This is fine as-is for compile-time
asserts.

Thanks for the reviews!

-Kees

-- 
Kees Cook