Message-ID: <CABi2SkV38U-ZCAq9W091zYkOM1m5e-C27YmVXdTCi-t+p_W_fQ@mail.gmail.com>
Date: Fri, 18 Oct 2024 11:06:20 -0700
From: Jeff Xu <jeffxu@...omium.org>
To: Mark Brown <broonie@...nel.org>, Muhammad Usama Anjum <usama.anjum@...labora.com>
Cc: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, akpm@...ux-foundation.org, 
	linux-kselftest@...r.kernel.org, linux-mm@...ck.org, 
	linux-hardening@...r.kernel.org, linux-kernel@...r.kernel.org, 
	pedro.falcato@...il.com, willy@...radead.org, vbabka@...e.cz, 
	Liam.Howlett@...cle.com, rientjes@...gle.com, keescook@...omium.org
Subject: Re: [PATCH v3 4/5] selftests/mseal: add more tests for mmap

Hi Mark and Muhammad

On Fri, Oct 18, 2024 at 6:04 AM Mark Brown <broonie@...nel.org> wrote:
>
> On Thu, Oct 17, 2024 at 12:49:40PM -0700, Jeff Xu wrote:
>
> > So it is not a problem with the MACRO, but where is it used ?
>
> >         ret = sys_mseal(ptr, size);
> >         FAIL_TEST_IF_FALSE(!ret);
>
> > Take this example, it would be
> > assert(!ret)
>
> The problem is that the macro name is confusing and not terribly
> consistent with how the rest of the selftests work.  The standard
> kselftest result reporting is
>
>         ksft_test_result(bool result, char *name_format, ...);
>
> so the result of the test is a boolean flag which is passed in.  This
> macro on the other hand sounds like a double negative so you have to
> stop and think what the logic is, and it's not seen anywhere else so
> nobody is going to be familiar with it.  The main thing this is doing is
> burying a return statement in there, that's a bit surprising too.
>
Thanks for explaining the problem, naming is hard. Do you have a
suggestion on a better naming?

> I'll also note that these macros are resulting in broken kselftest
> output, the name for a test has to be stable for automated systems to be
> able to associate test results between runs but these print
>
>                         ksft_test_result_fail("%s: line:%d\n",          \
>                                                 __func__, __LINE__);    \
>                         return;                                         \
>
> which includes the line number of the test in the name which is an
> obvious problem, automated systems won't be able to tell that any two
> failures are related to each other never mind the passing test.  We
> should report why things failed but it's better to do that with a
> ksft_print_msg(), ideally one that's directly readable rather than
> requiring someone to go into the source code and look it up.
>
I don't know what  the issue you described is ? Are you saying that we
are missing line numbers ? it is not. here is the sample of output:
Failure in the second test case from last:

ok 105 test_munmap_free_multiple_ranges
not ok 106 test_munmap_free_multiple_ranges_with_split: line:2573
ok 107 test_munmap_free_multiple_ranges_with_split
# Planned tests != run tests (106 != 107)

> A more standard way to write what you've got here would be to have the
> tests return a bool then have a runner loop which iterates over the
> tests:
>
>         struct {
>                 char *name;
>                 bool (*func)(void);
>         } tests[];
>
>         ...
>
>         for (i = 0; i < ARRAY_SIZE(tests); i++)
>                 ksft_test_result(tests[i].test(), tests[i].name);
>
> then the tests can just have explicit return statements and don't need
> to worry about logging anything other than diagnostics.
>
> Depending on how much you need to share between tests you might also be
> able to use kselftest_harness.h which fork()s each test into a separate
> child and allows you to just fault to fail if that's easier.
>
> > > We are writing unit tests in a test framework, let's use very well
> > > established industry practices please.
>
> Plus also the fact that we have a framework here...
>
> > > Also note that you don't even need to reinvent the wheel, there is a
> > > fully-featured test harness available in
> > > tools/testing/selftests/kselftest_harness.h with both ASSERT_xxx() and
> > > EXPECT_xxx() helpers.
>
> > The EXPECT_xxx() doesn't take care of reporting though,  or maybe it
>
> I rather think people would've noticed if the test harness was so broken
> that it was unable to report failures.  If it is that broken we should
> fix it rather than open coding something else.

In general, I agree with those comments, but I would like to rely on
domain experts in test infra to recommend what to use, or is
acceptable.

In this case, I hope Muhammad, who reviewed this code in the first
place, can make recommendations on a replacement of this macro.

I would image the needs of something similar to FAIL_TEST_IF_FALSE is
common in selftest writing:

1> lightweight enough so dev can pick up quickly and adapt to existing
tests, instead of rewriting everything from scratch.
2> assert like syntax
3> fail the current test case, but continue running the next test case
4> take care of reporting test failures.

Thanks
-Jeff

Powered by blists - more mailing lists