Received-SPF: pass (google.com: domain of linux-kernel+bounces-89089-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 1 Mar 2024 12:29:26 -0800
From: Kees Cook <keescook@chromium.org>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Andy Shevchenko <andy@kernel.org>,
	Kent Overstreet <kent.overstreet@linux.dev>,
	linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] string: Convert helpers selftest to KUnit
Message-ID: <202403011227.6C63925@keescook>
References: <20240301002416.it.092-kees@kernel.org>
 <20240301002650.1544829-2-keescook@chromium.org>
 <CAHp75Vf+zzVyVZ=fcXgdCMsp1MZdHmJ1aBTKFE4jHai2pWi27g@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAHp75Vf+zzVyVZ=fcXgdCMsp1MZdHmJ1aBTKFE4jHai2pWi27g@mail.gmail.com>

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@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/*).

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