On Wed, Aug 21, 2024 at 10:43 AM Michal Wajdeczko
<michal.wajdeczko@xxxxxxxxx> wrote:
>
Hello!
This is looking good and seems to be working well. I just had some
questions below.
Thanks!
-Rae
> Currently, the 'static stub' API only allows function redirection
> for calls made from the kthread of the current test, which prevents
> the use of this functionalty when the tested code is also used by
A slight typo here: functionality
> other threads, outside of the KUnit test, like from the workqueue.
>
> Add another set of macros to allow redirection to the replacement
> functions, which, unlike the KUNIT_STATIC_STUB_REDIRECT, will
> affect all calls done during the test execution.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> ---
> Cc: David Gow <davidgow@xxxxxxxxxx>
> Cc: Daniel Latypov <dlatypov@xxxxxxxxxx>
> Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> ---
> include/kunit/static_stub.h | 80 +++++++++++++++++++++++++++++++++++++
> lib/kunit/static_stub.c | 21 ++++++++++
> 2 files changed, 101 insertions(+)
>
> diff --git a/include/kunit/static_stub.h b/include/kunit/static_stub.h
> index bf940322dfc0..3dd98c8f3f1f 100644
> --- a/include/kunit/static_stub.h
> +++ b/include/kunit/static_stub.h
> @@ -12,6 +12,7 @@
>
> /* If CONFIG_KUNIT is not enabled, these stubs quietly disappear. */
> #define KUNIT_STATIC_STUB_REDIRECT(real_fn_name, args...) do {} while (0)
> +#define KUNIT_FIXED_STUB_REDIRECT(stub, args...) do {} while (0)
>
> #else
>
> @@ -109,5 +110,84 @@ void __kunit_activate_static_stub(struct kunit *test,
> */
> void kunit_deactivate_static_stub(struct kunit *test, void *real_fn_addr);
>
> +/**
> + * KUNIT_FIXED_STUB_REDIRECT() - Call a fixed function stub if activated.
> + * @stub: The location of the function stub pointer
> + * @args: All of the arguments passed to this stub
> + *
> + * This is a function prologue which is used to allow calls to the current
> + * function to be redirected if a KUnit is running. If the stub is NULL or
> + * the KUnit is not running the function will continue execution as normal.
> + *
> + * The function stub pointer must be stored in a place that is accessible both
> + * from the test code that will activate this stub and from the function where
> + * we will do the redirection.
> + *
> + * Unlike the KUNIT_STATIC_STUB_REDIRECT(), this redirection will work
> + * even if the caller is not in a KUnit context (like a worker thread).
> + *
> + * Example:
> + *
> + * .. code-block:: c
> + *
> + * static int (*func_stub)(int n);
> + *
> + * int real_func(int n)
> + * {
> + * KUNIT_FIXED_STUB_REDIRECT(func_stub, n);
> + * return n + 1;
> + * }
> + *
> + * int replacement_func(int n)
> + * {
> + * return n + 100;
> + * }
> + *
> + * void example_test(struct kunit *test)
> + * {
> + * KUNIT_EXPECT_EQ(test, real_func(1), 2);
> + * func_stub = replacement_func;
> + * KUNIT_EXPECT_EQ(test, real_func(1), 101);
I think we should model activating the stub here in the example to
make it a bit clearer.
> + * }
> + */
> +#define KUNIT_FIXED_STUB_REDIRECT(stub, args...) do { \
> + typeof(stub) replacement = (stub); \
> + if (kunit_is_running()) { \
> + if (unlikely(replacement)) { \
> + pr_info(KUNIT_SUBTEST_INDENT "# %s: calling stub %ps\n", \
> + __func__, replacement); \
> + return replacement(args); \
> + } \
> + } \
> +} while (0)
> +
> +void __kunit_activate_fixed_stub(struct kunit *test, void *stub_ptr, void *replacement_func);
> +
> +/**
> + * kunit_activate_fixed_stub() - Setup a fixed function stub.
> + * @test: Test case that wants to activate a fixed function stub
> + * @stub: The location of the function stub pointer
> + * @replacement: The replacement function
> + *
> + * This helper setups a function stub with the replacement function.
> + * It will also automatically restore stub to NULL at the test end.
> + */
> +#define kunit_activate_fixed_stub(test, stub, replacement) do { \
> + typecheck_pointer(stub); \
> + typecheck_fn(typeof(stub), replacement); \
> + typeof(stub) *stub_ptr = &(stub); \
> + __kunit_activate_fixed_stub((test), stub_ptr, (replacement)); \
> +} while (0)
> +
> +/**
> + * kunit_deactivate_fixed_stub() - Disable a fixed function stub.
> + * @test: Test case that wants to deactivate a fixed function stub (unused for now)
> + * @stub: The location of the function stub pointer
> + */
> +#define kunit_deactivate_fixed_stub(test, stub) do { \
> + typecheck(struct kunit *, (test)); \
> + (stub) = NULL; \
> +} while (0)
> +
> #endif
> #endif
> diff --git a/lib/kunit/static_stub.c b/lib/kunit/static_stub.c
> index 92b2cccd5e76..1b50cf457e89 100644
> --- a/lib/kunit/static_stub.c
> +++ b/lib/kunit/static_stub.c
> @@ -121,3 +121,24 @@ void __kunit_activate_static_stub(struct kunit *test,
> }
> }
> EXPORT_SYMBOL_GPL(__kunit_activate_static_stub);
> +
> +static void nullify_stub_ptr(void *data)
> +{
> + void **ptr = data;
> +
> + *ptr = NULL;
> +}
> +
> +/*
> + * Helper function for kunit_activate_static_stub(). The macro does
> + * typechecking, so use it instead.
> + */
> +void __kunit_activate_fixed_stub(struct kunit *test, void *stub_ptr, void *replacement_func)
> +{
> + void **stub = stub_ptr;
> +
> + KUNIT_ASSERT_NOT_NULL(test, stub_ptr);
Should we check here if the replacement_func is null and if so
deactivate the stub similar to the static stubbing?
> + *stub = replacement_func;
I do really appreciate this simplicity. But I wonder if David has any
thoughts on the security of direct replacement rather than using the
resource API?
> + KUNIT_ASSERT_EQ(test, 0, kunit_add_action_or_reset(test, nullify_stub_ptr, stub_ptr));
> +}
> +EXPORT_SYMBOL_GPL(__kunit_activate_fixed_stub);
> --
> 2.43.0
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@xxxxxxxxxxxxxxxx.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20240821144305.1958-4-michal.wajdeczko%40intel.com.
