| 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: <CABVgOSmViGQRfxEJ_WLjfByCo8tpg3X3rVFjeR7ez+rLnGFRtg@mail.gmail.com>
Date: Wed, 13 Aug 2025 17:17:53 +0800
From: David Gow <davidgow@...gle.com>
To: Marie Zhussupova <marievic@...gle.com>
Cc: rmoar@...gle.com, shuah@...nel.org, brendan.higgins@...ux.dev,
mark.rutland@....com, 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 v2 2/7] kunit: Introduce param_init/exit for parameterized
test context management
On Tue, 12 Aug 2025 at 06:17, Marie Zhussupova <marievic@...gle.com> wrote:
>
> Add (*param_init) and (*param_exit) function pointers to
> `struct kunit_case`. Users will be able to set them
> via the new KUNIT_CASE_PARAM_WITH_INIT() macro.
>
> param_init/exit will be invoked by kunit_run_tests() once before
> and once after the parameterized test, respectively.
> They will receive the `struct kunit` that holds the parameterized
> test context; facilitating init and exit for shared state.
>
> This patch also sets param_init/exit to None in
> rust/kernel/kunit.rs.
>
> Signed-off-by: Marie Zhussupova <marievic@...gle.com>
> ---
This mostly looks good to me, though there's one issue I've picked up:
we don't try to skip running the test if the param_init function
fails. I think we should do that, ideally in this series, but I'd live
with it in the try-catch support follow-up if you'd strongly prefer
it.
Otherwise, I think there's an argument that param_init/param_exit
should run even before/after non-parameterised tests. The
suite_init/suite_exit functions would run even in an empty suite, for
example. Though given it's useless to actually implement this, I'm
happy with it the way it is.
Further note on the need to skip the tests below.
Cheers,
-- David
>
> Changes in v2:
>
> - param init/exit were set to None
> in rust/kernel/kunit.rs to fix the Rust breakage.
> - The name of __kunit_init_parent_test was
> changed to kunit_init_parent_param_test and
> its call was changed to happen only if the
> test is parameterized.
> - The param_exit call was also moved inside
> the check for if the test is parameterized.
> - KUNIT_CASE_PARAM_WITH_INIT() macro logic was changed
> to not automatically set generate_params() to KUnit's
> built-in generator function. Instead, the test user
> will be asked to provide it themselves.
> - The comments and the commit message were changed to
> reflect the parameterized testing terminology. See
> the patch series cover letter change log for the
> definitions.
>
> ---
> include/kunit/test.h | 25 +++++++++++++++++++++++++
> lib/kunit/test.c | 20 ++++++++++++++++++++
> rust/kernel/kunit.rs | 4 ++++
> 3 files changed, 49 insertions(+)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index b47b9a3102f3..d2e1b986b161 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -92,6 +92,8 @@ struct kunit_attributes {
> * @name: the name of the test case.
> * @generate_params: the generator function for parameterized tests.
> * @attr: the attributes associated with the test
> + * @param_init: The init function to run before a parameterized test.
> + * @param_exit: The exit function to run after a parameterized test.
> *
> * A test case is a function with the signature,
> * ``void (*)(struct kunit *)``
> @@ -128,6 +130,8 @@ struct kunit_case {
> const char *name;
> const void* (*generate_params)(const void *prev, char *desc);
> struct kunit_attributes attr;
> + int (*param_init)(struct kunit *test);
> + void (*param_exit)(struct kunit *test);
>
> /* private: internal use only. */
> enum kunit_status status;
> @@ -218,6 +222,27 @@ static inline char *kunit_status_to_ok_not_ok(enum kunit_status status)
> .generate_params = gen_params, \
> .attr = attributes, .module_name = KBUILD_MODNAME}
>
> +/**
> + * KUNIT_CASE_PARAM_WITH_INIT - Define a parameterized KUnit test case with custom
> + * param_init() and param_exit() functions.
> + * @test_name: The function implementing the test case.
> + * @gen_params: The function to generate parameters for the test case.
> + * @init: A reference to the param_init() function to run before a parameterized test.
> + * @exit: A reference to the param_exit() function to run after a parameterized test.
> + *
> + * Provides the option to register param_init() and param_exit() functions.
> + * param_init/exit will be passed the parameterized test context and run once
> + * before and once after the parameterized test. The init function can be used
> + * to add resources to share between parameter runs, and any other setup logic.
> + * The exit function can be used to clean up resources that were not managed by
> + * the parameterized test, and any other teardown logic.
> + */
> +#define KUNIT_CASE_PARAM_WITH_INIT(test_name, gen_params, init, exit) \
> + { .run_case = test_name, .name = #test_name, \
> + .generate_params = gen_params, \
> + .param_init = init, .param_exit = exit, \
> + .module_name = KBUILD_MODNAME}
> +
> /**
> * struct kunit_suite - describes a related collection of &struct kunit_case
> *
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 14a8bd846939..49a5e6c30c86 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -641,6 +641,19 @@ static void kunit_accumulate_stats(struct kunit_result_stats *total,
> total->total += add.total;
> }
>
> +static void kunit_init_parent_param_test(struct kunit_case *test_case, struct kunit *test)
> +{
> + if (test_case->param_init) {
> + int err = test_case->param_init(test);
> +
> + if (err) {
> + kunit_err(test_case, KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT
> + "# failed to initialize parent parameter test.");
> + test_case->status = KUNIT_FAILURE;
> + }
> + }
> +}
> +
> int kunit_run_tests(struct kunit_suite *suite)
> {
> char param_desc[KUNIT_PARAM_DESC_SIZE];
> @@ -678,6 +691,7 @@ int kunit_run_tests(struct kunit_suite *suite)
> kunit_run_case_catch_errors(suite, test_case, &test);
> kunit_update_stats(¶m_stats, test.status);
> } else {
> + kunit_init_parent_param_test(test_case, &test);
Should we skip the rest of the test if this fails? I think ultimately
we should -- that's the behaviour we have for, e.g.,
suite_init/suite_exit, though we don't skip the rest of the test if an
individual parameter fails.
Regardless, it seems likely that generate_params will crash if the
init failed for any test making serious use of it. But then again,
this is already a bit fragile due to the lack of try/catch around the
generate_params, etc.
My preference here would be to skip the rest of the test (and the
param_exit function) if the parent test is marked FAILED immediately
after this line. I could live with pushing that back to the follow-up
which adds the try-catch support, though, if you'd really prefer it.
> /* Get initial param. */
> param_desc[0] = '\0';
> /* TODO: Make generate_params try-catch */
> @@ -714,6 +728,12 @@ int kunit_run_tests(struct kunit_suite *suite)
> param_desc[0] = '\0';
> curr_param = test_case->generate_params(curr_param, param_desc);
> }
> + /*
> + * TODO: Put into a try catch. Since we don't need suite->exit
> + * for it we can't reuse kunit_try_run_cleanup for this yet.
> + */
> + if (test_case->param_exit)
> + test_case->param_exit(&test);
> /* TODO: Put this kunit_cleanup into a try-catch. */
> kunit_cleanup(&test);
> }
> diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
> index 4b8cdcb21e77..cda64574b44d 100644
> --- a/rust/kernel/kunit.rs
> +++ b/rust/kernel/kunit.rs
> @@ -207,6 +207,8 @@ pub const fn kunit_case(
> status: kernel::bindings::kunit_status_KUNIT_SUCCESS,
> module_name: core::ptr::null_mut(),
> log: core::ptr::null_mut(),
> + param_init: None,
> + param_exit: None,
> }
> }
>
> @@ -226,6 +228,8 @@ pub const fn kunit_case_null() -> kernel::bindings::kunit_case {
> status: kernel::bindings::kunit_status_KUNIT_SUCCESS,
> module_name: core::ptr::null_mut(),
> log: core::ptr::null_mut(),
> + param_init: None,
> + param_exit: None,
> }
> }
>
> --
> 2.51.0.rc0.205.g4a044479a3-goog
>
Download attachment "smime.p7s" of type "application/pkcs7-signature" (5281 bytes)
Powered by blists - more mailing lists