Re: [PATCH 3/4] kunit: Allow function redirection outside of the KUnit thread

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 21.08.2024 23:38, Lucas De Marchi wrote:
> On Wed, Aug 21, 2024 at 04:43:04PM GMT, Michal Wajdeczko wrote:
>> 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
>> 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@intel.com>
>> ---
>> Cc: David Gow <davidgow@google.com>
>> Cc: Daniel Latypov <dlatypov@google.com>
>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>> 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).
> 
> not sure about the name FIXED vs STATIC. What about

well, I must admit that I also wasn't sure about the name..

> 
> KUNIT_FIXED_STUB_REDIRECT()
> KUNIT_FIXED_STUB_REDIRECT_ALL_CONTEXTS()

since we have

	KUNIT_STATIC_STUB_REDIRECT()

then maybe

	KUNIT_STATIC_STUB_REDIRECT_ALL_CONTEXTS()
	KUNIT_STATIC_STUB_REDIRECT_GLOBAL()

> 
> ?
> 
>> + *
>> + * Example:
>> + *
>> + * .. code-block:: c
>> + *
>> + *    static int (*func_stub)(int n);
>> + *
>> + *    int real_func(int n)
>> + *    {
>> + *        KUNIT_FIXED_STUB_REDIRECT(func_stub, n);
> 
> what I don't like here is that KUNIT_STATIC_STUB_REDIRECT()
> uses the name of **this** function, whiel KUNIT_FIXED_STUB_REDIRECT()
> uses the function pointer. I don't have a better suggestion on how to
> handle it, but this looks error prone.

the KUNIT_STATIC_STUB_REDIRECT() is in a better position as it can do
lookup a resource (which is a stub function pointer) based on the "this
function" as it runs in a kunit context, while FIXED variant must access
stub pointer directly

or do you mean that wrong stub name could provided?
but note that you may also make the same mistake today:

int foo(int n)
{
	KUNIT_STATIC_STUB_REDIRECT(bar, n);
}

hmm, maybe we can try to do this:

KUNIT_STATIC_STUB_REDIRECT_GLOBAL(stubs, func, args...)
@stubs - place where function pointers are kept
@func - we can use it to check types and as member name

	replacement = stubs.func;
	return replacement(args);

kunit_activate_static_stub_global(test, stubs, func, replacement)
@stubs - place where function pointers are kept
@func - we can use it to check types and as member name

	stubs.func = replacement

> 
>> + *        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);
>> + *    }
>> + */
>> +#define KUNIT_FIXED_STUB_REDIRECT(stub, args...) do
>> {                    \
>> +    typeof(stub) replacement = (stub);                        \
>> +    if (kunit_is_running()) {                            \
>> +        if (unlikely(replacement)) {                        \
> 
> probably better as `if (unlikely(kunit_is_running() && replacement))`?
> Because we are likely to use one specific replacement in just 1 or few
> tests from an entire testsuite.

kunit_is_running() is already 'unlikely'

> 
> Lucas De Marchi
> 
>> +            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);
>> +    *stub = replacement_func;
>> +    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
>>