Message-ID: <1f8eff74-005b-4fa9-9446-47f4cdbf3e8d@sirena.org.uk>
Date: Fri, 18 Oct 2024 14:04:04 +0100
From: Mark Brown <broonie@...nel.org>
To: Jeff Xu <jeffxu@...omium.org>
Cc: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
	Muhammad Usama Anjum <usama.anjum@...labora.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

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.

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.

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.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists