On 21.08.2024 23:32, Rae Moar wrote:
> 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.
oops, this DOC was taken almost as-is from my Xe series mentioned in the
cover letter, where I didn't have any "activate_fixed_stub" code yet
will fix
>
>> + * }
>> + */
>> +#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?
I had KUNIT_ASSERT_NOT_NULL(test, replacement_func) but decided to drop
that to also allow using 'activate_stub(NULL)' to deactivate the stub -
but that was before I introduced a separate 'deactivate_stub()'
will bring that back
>
>> + *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?
note that at redirection point we will not be able to use resource API
since that could be run in a non-kunit context
>
>> + 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.
