| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CABVgOSkaqvqU7r51tyyNc+NXD9D72zA_3QeYoYYxO6qszkOHCw@mail.gmail.com>
Date: Sat, 2 Aug 2025 17:44:34 +0800
From: David Gow <davidgow@...gle.com>
To: Marie Zhussupova <marievic@...gle.com>
Cc: rmoar@...gle.com, shuah@...nel.org, brendan.higgins@...ux.dev,
elver@...gle.com, dvyukov@...gle.com, lucas.demarchi@...el.com,
thomas.hellstrom@...ux.intel.com, rodrigo.vivi@...el.com,
linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com,
kasan-dev@...glegroups.com, intel-xe@...ts.freedesktop.org,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/9] kunit: Add parent kunit for parameterized test context
On Wed, 30 Jul 2025 at 03:37, Marie Zhussupova <marievic@...gle.com> wrote:
>
> Currently, KUnit parameterized tests lack a mechanism
> to share resources across individual test invocations
> because the same `struct kunit` instance is reused for
> each test.
>
> This patch refactors kunit_run_tests() to provide each
> parameterized test with its own `struct kunit` instance.
> A new parent pointer is added to `struct kunit`, allowing
> individual parameterized tests to reference a shared
> parent kunit instance. Resources added to this parent
> will then be accessible to all individual parameter
> test executions.
>
> Signed-off-by: Marie Zhussupova <marievic@...gle.com>
> ---
I'm definitely a fan of this, though it's not without its downsides.
For most tests, I don't think this will be a noticeable difference,
since the "shared" struct kunit is reset after each parameter anyway,
so this is a pretty safe change.
Having a 'parent' struct kunit, and hence potentially a hierarchy,
does give us a good way of implementing more complicated parameterised
tests which might need some more persistent state. (Ultimately, we
could have another level for suites, and then allow
suite_init/suite_exit to setup something persistent, too.)
Anyway, this looks good to me. I've left some small notes below, but
nothing I think is actionable.
Reviewed-by: David Gow <davidgow@...gle.com>
Cheers,
-- David
> include/kunit/test.h | 12 ++++++++++--
> lib/kunit/test.c | 32 +++++++++++++++++++-------------
> 2 files changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 39c768f87dc9..a42d0c8cb985 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -268,14 +268,22 @@ struct kunit_suite_set {
> *
> * @priv: for user to store arbitrary data. Commonly used to pass data
> * created in the init function (see &struct kunit_suite).
> + * @parent: for user to store data that they want to shared across
> + * parameterized tests.
A part of me would prefer this not to explicitly call out
parameterized tests here, as the obvious extensions to this (having
suites have a struct kunit, or other forms of test hierarchy) would
still make use of this in non-parameterised use-cases.
But since parameterised tests are the only current use-case for it, I
can live with it if you'd prefer.
> *
> * Used to store information about the current context under which the test
> * is running. Most of this data is private and should only be accessed
> - * indirectly via public functions; the one exception is @priv which can be
> - * used by the test writer to store arbitrary data.
> + * indirectly via public functions; the two exceptions are @priv and @parent
> + * which can be used by the test writer to store arbitrary data or data that is
> + * available to all parameter test executions, respectively.
> */
> struct kunit {
> void *priv;
> + /*
> + * Reference to the parent struct kunit for storing shared resources
> + * during parameterized testing.
> + */
> + struct kunit *parent;
>
> /* private: internal use only. */
> const char *name; /* Read only after initialization! */
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index f3c6b11f12b8..4d6a39eb2c80 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -647,6 +647,7 @@ int kunit_run_tests(struct kunit_suite *suite)
> struct kunit_case *test_case;
> struct kunit_result_stats suite_stats = { 0 };
> struct kunit_result_stats total_stats = { 0 };
> + const void *curr_param;
>
> /* Taint the kernel so we know we've run tests. */
> add_taint(TAINT_TEST, LOCKDEP_STILL_OK);
> @@ -679,36 +680,39 @@ int kunit_run_tests(struct kunit_suite *suite)
> } else {
> /* Get initial param. */
> param_desc[0] = '\0';
> - test.param_value = test_case->generate_params(NULL, param_desc);
> + /* TODO: Make generate_params try-catch */
Thanks for adding the TODO here: this isn't a regression, but it's
good to note that we should get around to fixing it.
> + curr_param = test_case->generate_params(NULL, param_desc);
> test_case->status = KUNIT_SKIPPED;
> kunit_log(KERN_INFO, &test, KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT
> "KTAP version 1\n");
> kunit_log(KERN_INFO, &test, KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT
> "# Subtest: %s", test_case->name);
>
> - while (test.param_value) {
> - kunit_run_case_catch_errors(suite, test_case, &test);
> + while (curr_param) {
> + struct kunit param_test = {
> + .param_value = curr_param,
> + .param_index = ++test.param_index,
> + .parent = &test,
> + };
> + kunit_init_test(¶m_test, test_case->name, test_case->log);
> + kunit_run_case_catch_errors(suite, test_case, ¶m_test);
>
> if (param_desc[0] == '\0') {
> snprintf(param_desc, sizeof(param_desc),
> "param-%d", test.param_index);
> }
>
> - kunit_print_ok_not_ok(&test, KUNIT_LEVEL_CASE_PARAM,
> - test.status,
> - test.param_index + 1,
> + kunit_print_ok_not_ok(¶m_test, KUNIT_LEVEL_CASE_PARAM,
> + param_test.status,
> + param_test.param_index,
> param_desc,
> - test.status_comment);
> + param_test.status_comment);
>
> - kunit_update_stats(¶m_stats, test.status);
> + kunit_update_stats(¶m_stats, param_test.status);
>
> /* Get next param. */
> param_desc[0] = '\0';
> - test.param_value = test_case->generate_params(test.param_value, param_desc);
> - test.param_index++;
> - test.status = KUNIT_SUCCESS;
> - test.status_comment[0] = '\0';
> - test.priv = NULL;
> + curr_param = test_case->generate_params(curr_param, param_desc);
> }
> }
>
> @@ -723,6 +727,8 @@ int kunit_run_tests(struct kunit_suite *suite)
>
> kunit_update_stats(&suite_stats, test_case->status);
> kunit_accumulate_stats(&total_stats, param_stats);
> + /* TODO: Put this kunit_cleanup into a try-catch. */
> + kunit_cleanup(&test);
Hmm... it is a shame we can't easily include this in the existing
try-catch mechanism. Definitely worth fixing in a follow-up.
> }
>
> if (suite->suite_exit)
> --
> 2.50.1.552.g942d659e1b-goog
>
Download attachment "smime.p7s" of type "application/pkcs7-signature" (5281 bytes)
Powered by blists - more mailing lists