[PATCH 0/4] kunit: Add macros to help write more complex tests

[Michal Wajdeczko]

It was suggested to promote some of the ideas introduced by [1] to be
a part of the core KUnit instead of keeping them locally.

[1] https://patchwork.freedesktop.org/series/137095/

Cc: Rae Moar <rm...@google.com>
Cc: David Gow <davi...@google.com>
Cc: Lucas De Marchi <lucas.d...@intel.com>

Michal Wajdeczko (4):
kunit: Introduce kunit_is_running()
kunit: Add macro to conditionally expose declarations to tests
kunit: Allow function redirection outside of the KUnit thread
kunit: Add example with alternate function redirection method

include/kunit/static_stub.h | 80 ++++++++++++++++++++++++++++++++++
include/kunit/test-bug.h | 12 ++++-
include/kunit/visibility.h | 8 ++++
lib/kunit/kunit-example-test.c | 63 ++++++++++++++++++++++++++
lib/kunit/static_stub.c | 21 +++++++++
5 files changed, 182 insertions(+), 2 deletions(-)

--
2.43.0

[Michal Wajdeczko]

Wrap uses of the static key 'kunit_running' into a helper macro
to allow future checks to be placed in the code residing outside
of the CONFIG_KUNIT. We will start using this in upcoming patch.

Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>
---
Cc: Daniel Latypov <dlat...@google.com>

Cc: David Gow <davi...@google.com>
Cc: Lucas De Marchi <lucas.d...@intel.com>

---
include/kunit/test-bug.h | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h
index 47aa8f21ccce..e8ea3bab7250 100644
--- a/include/kunit/test-bug.h
+++ b/include/kunit/test-bug.h
@@ -25,6 +25,13 @@ extern struct kunit_hooks_table {
void *(*get_static_stub_address)(struct kunit *test, void *real_fn_addr);
} kunit_hooks;

+/**
+ * kunit_is_running() - True, if KUnit test is currently running.
+ *
+ * If CONFIG_KUNIT is not enabled, it will compile down to a false.
+ */
+#define kunit_is_running() static_branch_unlikely(&kunit_running)
+
/**
* kunit_get_current_test() - Return a pointer to the currently running
* KUnit test.
@@ -40,7 +47,7 @@ extern struct kunit_hooks_table {
*/
static inline struct kunit *kunit_get_current_test(void)
{
- if (!static_branch_unlikely(&kunit_running))
+ if (!kunit_is_running())
return NULL;

return current->kunit_test;
@@ -53,7 +60,7 @@ static inline struct kunit *kunit_get_current_test(void)
* If a KUnit test is running in the current task, mark that test as failed.
*/
#define kunit_fail_current_test(fmt, ...) do { \
- if (static_branch_unlikely(&kunit_running)) { \
+ if (kunit_is_running()) { \
/* Guaranteed to be non-NULL when kunit_running true*/ \
kunit_hooks.fail_current_test(__FILE__, __LINE__, \
fmt, ##__VA_ARGS__); \
@@ -64,6 +71,7 @@ static inline struct kunit *kunit_get_current_test(void)

static inline struct kunit *kunit_get_current_test(void) { return NULL; }

+#define kunit_is_running() false
#define kunit_fail_current_test(fmt, ...) do {} while (0)

#endif
--
2.43.0

[Michal Wajdeczko]

The DECLARE_IF_KUNIT macro will introduces identifiers only if
CONFIG_KUNIT is enabled. Otherwise if CONFIG_KUNIT is not enabled
no identifiers from the param list will be defined.

Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>
---

Cc: Rae Moar <rm...@google.com>
Cc: David Gow <davi...@google.com>
Cc: Lucas De Marchi <lucas.d...@intel.com>

---
include/kunit/visibility.h | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/include/kunit/visibility.h b/include/kunit/visibility.h
index 0dfe35feeec6..1c23773f826c 100644
--- a/include/kunit/visibility.h
+++ b/include/kunit/visibility.h
@@ -11,6 +11,13 @@
#define _KUNIT_VISIBILITY_H

#if IS_ENABLED(CONFIG_KUNIT)
+ /**
+ * DECLARE_IF_KUNIT - A macro that introduces identifiers only if
+ * CONFIG_KUNIT is enabled. Otherwise if CONFIG_KUNIT is not enabled
+ * no identifiers will be defined.
+ * @body: identifiers to be introduced conditionally
+ */
+ #define DECLARE_IF_KUNIT(body...) body
/**
* VISIBLE_IF_KUNIT - A macro that sets symbols to be static if
* CONFIG_KUNIT is not enabled. Otherwise if CONFIG_KUNIT is enabled
@@ -26,6 +33,7 @@
#define EXPORT_SYMBOL_IF_KUNIT(symbol) EXPORT_SYMBOL_NS(symbol, \
EXPORTED_FOR_KUNIT_TESTING)
#else
+ #define DECLARE_IF_KUNIT(body...)
#define VISIBLE_IF_KUNIT static
#define EXPORT_SYMBOL_IF_KUNIT(symbol)
#endif
--
2.43.0

[Michal Wajdeczko]

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.w...@intel.com>
---

Cc: David Gow <davi...@google.com>
Cc: Daniel Latypov <dlat...@google.com>

Cc: Lucas De Marchi <lucas.d...@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).
+ *
+ * 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);
+ * }
+ */
+#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);
+ *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

[Michal Wajdeczko]

Add example how to use KUNIT_FIXED_STUB_REDIRECT and compare its
usage with the KUNIT_STATIC_STUB_REDIRECT. Also show how the
DECLARE_IF_KUNIT macro could be helpful in declaring test data in
the non-test data structures.

Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>
---

Cc: David Gow <davi...@google.com>
Cc: Daniel Latypov <dlat...@google.com>

Cc: Lucas De Marchi <lucas.d...@intel.com>

---
lib/kunit/kunit-example-test.c | 63 ++++++++++++++++++++++++++++++++++
1 file changed, 63 insertions(+)

diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
index 3056d6bc705d..120e08d8899b 100644
--- a/lib/kunit/kunit-example-test.c
+++ b/lib/kunit/kunit-example-test.c
@@ -6,8 +6,10 @@
* Author: Brendan Higgins <brendan...@google.com>
*/

+#include <linux/workqueue.h>
#include <kunit/test.h>
#include <kunit/static_stub.h>
+#include <kunit/visibility.h>

/*
* This is the most fundamental element of KUnit, the test case. A test case
@@ -221,6 +223,66 @@ static void example_static_stub_using_fn_ptr_test(struct kunit *test)
KUNIT_EXPECT_EQ(test, add_one(1), 2);
}

+/* This could be a location of various fixed stub functions. */
+static struct {
+ DECLARE_IF_KUNIT(int (*add_two)(int i));
+} stubs;
+
+/* This is a function we'll replace with stubs. */
+static int add_two(int i)
+{
+ /* This will trigger the stub if active. */
+ KUNIT_STATIC_STUB_REDIRECT(add_two, i);
+ KUNIT_FIXED_STUB_REDIRECT(stubs.add_two, i);
+
+ return i + 2;
+}
+
+struct add_two_async_work {
+ struct work_struct work;
+ int param;
+ int result;
+};
+
+static void add_two_async_func(struct work_struct *work)
+{
+ struct add_two_async_work *w = container_of(work, typeof(*w), work);
+
+ w->result = add_two(w->param);
+}
+
+static int add_two_async(int i)
+{
+ struct add_two_async_work w = { .param = i };
+
+ INIT_WORK_ONSTACK(&w.work, add_two_async_func);
+ schedule_work(&w.work);
+ flush_work(&w.work);
+ destroy_work_on_stack(&w.work);
+
+ return w.result;
+}
+
+/*
+ */
+static void example_fixed_stub_test(struct kunit *test)
+{
+ /* static stub redirection works only for KUnit thread */
+ kunit_activate_static_stub(test, add_two, subtract_one);
+ KUNIT_EXPECT_EQ(test, add_two(1), subtract_one(1));
+ KUNIT_EXPECT_NE_MSG(test, add_two_async(1), subtract_one(1),
+ "stub shouldn't be active outside KUnit thread!");
+ kunit_deactivate_static_stub(test, add_two);
+ KUNIT_EXPECT_EQ(test, add_two(1), add_two(1));
+
+ /* fixed stub redirection works for KUnit and other threads */
+ kunit_activate_fixed_stub(test, stubs.add_two, subtract_one);
+ KUNIT_EXPECT_EQ(test, add_two(1), subtract_one(1));
+ KUNIT_EXPECT_EQ(test, add_two_async(1), subtract_one(1));
+ kunit_deactivate_fixed_stub(test, stubs.add_two);
+ KUNIT_EXPECT_EQ(test, add_two(1), add_two(1));
+}
+
static const struct example_param {
int value;
} example_params_array[] = {
@@ -294,6 +356,7 @@ static struct kunit_case example_test_cases[] = {
KUNIT_CASE(example_all_expect_macros_test),
KUNIT_CASE(example_static_stub_test),
KUNIT_CASE(example_static_stub_using_fn_ptr_test),
+ KUNIT_CASE(example_fixed_stub_test),
KUNIT_CASE(example_priv_test),
KUNIT_CASE_PARAM(example_params_test, example_gen_params),
KUNIT_CASE_SLOW(example_slow_test),
--
2.43.0

[Rae Moar]

On Wed, Aug 21, 2024 at 10:43 AM Michal Wajdeczko
<michal.w...@intel.com> wrote:
>
> Wrap uses of the static key 'kunit_running' into a helper macro
> to allow future checks to be placed in the code residing outside
> of the CONFIG_KUNIT. We will start using this in upcoming patch.
>
> Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>

Hello!

I am good with this. It is definitely a prettier way to access kunit_running.

Reviewed-by: Rae Moar <rm...@google.com>

Thanks!

-Rae

> --
> 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+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20240821144305.1958-2-michal.wajdeczko%40intel.com.

[Rae Moar]

On Wed, Aug 21, 2024 at 10:43 AM Michal Wajdeczko
<michal.w...@intel.com> wrote:
>

> The DECLARE_IF_KUNIT macro will introduces identifiers only if
> CONFIG_KUNIT is enabled. Otherwise if CONFIG_KUNIT is not enabled
> no identifiers from the param list will be defined.
>
> Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>

Hello!

I like this macro. I think it could definitely be useful in declaring
static functions for KUnit testing in the header files. So I am happy
to add it.

We should also add this to the documentation at some point. I've been
wanting to revamp the visibility.h macros documentation anyways.

Reviewed-by: Rae Moar <rm...@google.com>

Thanks!

-Rae

[Rae Moar]

On Wed, Aug 21, 2024 at 10:43 AM Michal Wajdeczko
<michal.w...@intel.com> wrote:
>

> Add example how to use KUNIT_FIXED_STUB_REDIRECT and compare its
> usage with the KUNIT_STATIC_STUB_REDIRECT. Also show how the
> DECLARE_IF_KUNIT macro could be helpful in declaring test data in
> the non-test data structures.
>
> Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>

Hello!

I really like this test. It provides a great overview of this patch
series. I just have a couple comments below.

Otherwise,

Reviewed-by: Rae Moar <rm...@google.com>

Thanks!
-Rae

Is the use of DECLARE_IF_KUNIT useful here? KUnit should always be
enabled if this file is being compiled/run. Is the idea to show an
example here that could be used outside of kunit test files?

Additionally, would it make sense to call this add_two_stub instead to
make it clear that this is not a definition of the add_two function?
Or is it helpful for people to see this as an example of how to handle
multiple stubs: struct of stubs with exact names? Let me know what you
think.

It looks like the method description is missing here.

> --
> 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+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20240821144305.1958-5-michal.wajdeczko%40intel.com.

[Lucas De Marchi]

On Wed, Aug 21, 2024 at 04:43:02PM GMT, Michal Wajdeczko wrote:
>Wrap uses of the static key 'kunit_running' into a helper macro
>to allow future checks to be placed in the code residing outside
>of the CONFIG_KUNIT. We will start using this in upcoming patch.
>
>Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>

Reviewed-by: Lucas De Marchi <lucas.d...@intel.com>

Lucas De Marchi

[Rae Moar]

On Wed, Aug 21, 2024 at 10:43 AM Michal Wajdeczko
<michal.w...@intel.com> 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

I think we should model activating the stub here in the example to
make it a bit clearer.

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+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20240821144305.1958-4-michal.wajdeczko%40intel.com.

[Lucas De Marchi]

not sure about the name FIXED vs STATIC. What about

KUNIT_FIXED_STUB_REDIRECT()
KUNIT_FIXED_STUB_REDIRECT_ALL_CONTEXTS()

?

>+ *
>+ * 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.

>+ * 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.

Lucas De Marchi

[Michal Wajdeczko]

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

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

[Michal Wajdeczko]

yes, the idea was to show that 'stubs' declarations could be placed
anywhere, without any cost if compiled without KUNIT (I was trying to
mention that in commit message)

>
> Additionally, would it make sense to call this add_two_stub instead to
> make it clear that this is not a definition of the add_two function?
> Or is it helpful for people to see this as an example of how to handle
> multiple stubs: struct of stubs with exact names? Let me know what you
> think.

the 'add_two' above is just a member name, and IMO we shouldn't repeat
that this is about 'stub' since the whole struct is for 'stubs'

and yes, the idea was also to show that if applicable, other function
stubs declarations could be either placed together

ha, I missed to copy commit message here

[Michal Wajdeczko]

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'

[David Gow]

On Wed, 21 Aug 2024 at 22:43, Michal Wajdeczko
<michal.w...@intel.com> wrote:
>
> Wrap uses of the static key 'kunit_running' into a helper macro
> to allow future checks to be placed in the code residing outside
> of the CONFIG_KUNIT. We will start using this in upcoming patch.
>
> Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>
> ---
> Cc: Daniel Latypov <dlat...@google.com>
> Cc: David Gow <davi...@google.com>
> Cc: Lucas De Marchi <lucas.d...@intel.com>
> ---

This is a big improvement, thanks!

Reviewed-by: David Gow <davi...@google.com>

Cheers,
-- David

[David Gow]

On Wed, 21 Aug 2024 at 22:43, Michal Wajdeczko
<michal.w...@intel.com> wrote:
>

> The DECLARE_IF_KUNIT macro will introduces identifiers only if
> CONFIG_KUNIT is enabled. Otherwise if CONFIG_KUNIT is not enabled
> no identifiers from the param list will be defined.
>
> Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>
> ---
> Cc: Rae Moar <rm...@google.com>
> Cc: David Gow <davi...@google.com>
> Cc: Lucas De Marchi <lucas.d...@intel.com>
> ---

I like this, thanks!

Reviewed-by: David Gow <davi...@google.com>

[David Gow]

On Wed, 21 Aug 2024 at 22:43, Michal Wajdeczko
<michal.w...@intel.com> 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.w...@intel.com>
> ---
> Cc: David Gow <davi...@google.com>
> Cc: Daniel Latypov <dlat...@google.com>
> Cc: Lucas De Marchi <lucas.d...@intel.com>
> ---

Thanks for sending this in: we really do need a way to use stubs from
other threads.

In general, I think there are two possible ways of implementing that:
- Make a version of the stubs which don't need a KUnit context (i.e.,
this patch), or
- Have a way of extending the KUnit context to other threads.

Personally, I'd prefer the latter if it were feasible, as it is both
cleaner from a user point of view (we don't need two variants of the
same thing), and extends more easily to features beyond stubs.
However, there are a few downsides:
- We'd need to find a way of handling the case where the test function
returns while there's still a background thread happening.
- In general, KUnit would need to be audited for thread safety for
those macros (I don't think it's too bad, but worth checking),
- We'd need a way of passing the KUnit context to the new thread,
which might be difficult to make pleasant.
- We'd need to handle cases where a thread is only partly running test
code, or otherwise doesn't create its threads directly (e.g.,
workqueues, rcu, etc)
- What should happen if an assertion fails on another thread — does
the failing thread quit, does the original thread quit, both?

In short, it's a complicated enough situation that I doubt we'd solve
all of those problems cleanly or quickly, so this patch is probably
the better option. Regardless, we need a story for what the thread
safety of this is -- can you activate/deactivate stubs while the
stubbed function is being called. (My gut feeling is "this should be
possible, but is probably ill-advised" is what we should aim for,
which probably requires making sure the stub update is done
atomically.

More rambling below, but I think this is probably good once the
atomic/thread-safety stuff is either sorted out or at least
documented. As for the name, how about KUNIT_GLOBAL_STUB_REDIRECT()?
I'm okay with FIXED_STUB if you prefer it, though.

Cheers,
-- David

I suspect we want to at least make the logging here optional,
particularly since it doesn't go into the test log.

> + __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 { \

Do we want to actually hook this into the struct kunit here? I suspect
we do, but it does mean that this has to either be called from the
main thread, or the struct kunit* needs to be passed around.

As below, does this need to be atomic?

> +}
> +
> +/*
> + * 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;

Do we need to do something here to make this atomic? At the very
least, I think we want to READ_ONCE()/WRITE_ONCE() these, but even
then we could end up with torn writes, I think.

In general, though, what's the rule around activating a stub from a
different thread to it being called? I thinki we definitely want to
allow it, but _maybe_ we can get away with saying that the stub can't
be activated/deactivated concurrently with being used?

> + KUNIT_ASSERT_EQ(test, 0, kunit_add_action_or_reset(test, nullify_stub_ptr, stub_ptr));

Again, we probably need to emphasise that any work done on another
thread needs to be complete before the test ends on the main thread.
This isn't specific to this feature, though.

> +}
> +EXPORT_SYMBOL_GPL(__kunit_activate_fixed_stub);

> --
> 2.43.0
>

[David Gow]

On Wed, 21 Aug 2024 at 22:43, Michal Wajdeczko
<michal.w...@intel.com> wrote:
>

> Add example how to use KUNIT_FIXED_STUB_REDIRECT and compare its
> usage with the KUNIT_STATIC_STUB_REDIRECT. Also show how the
> DECLARE_IF_KUNIT macro could be helpful in declaring test data in
> the non-test data structures.
>
> Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>
> ---
> Cc: David Gow <davi...@google.com>
> Cc: Daniel Latypov <dlat...@google.com>
> Cc: Lucas De Marchi <lucas.d...@intel.com>
> ---

This looks good to me, thanks!

Reviewed-by: David Gow <davi...@google.com>

Thanks,
-- David

[Rae Moar]

Happy with this as is then! Thanks for your response.

-Rae

[Michal Wajdeczko]

v1: https://groups.google.com/g/kunit-dev/c/f4LIMLyofj8
v2: make it more complex and attempt to be thread safe
s/FIXED_STUB/GLOBAL_STUB (David, Lucas)
make it little more thread safe (Rae, David)
wait until stub call finishes before test end (David)
wait until stub call finishes before changing stub (David)
allow stub deactivation (Rae)
prefer kunit log (David)
add simple selftest (Michal)
also introduce ONLY_IF_KUNIT macro (Michal)

Sample output from the tests:

$ tools/testing/kunit/kunit.py run *example*.*global* \
--kunitconfig lib/kunit/.kunitconfig --raw_output

KTAP version 1
1..1
# example: initializing suite
KTAP version 1
# Subtest: example
# module: kunit_example_test
1..1
# example_global_stub_test: initializing
# example_global_stub_test: add_two: redirecting to subtract_one
# example_global_stub_test: add_two: redirecting to subtract_one
# example_global_stub_test: cleaning up
ok 1 example_global_stub_test
# example: exiting suite
ok 1 example

$ tools/testing/kunit/kunit.py run *global*.*global* \
--kunitconfig lib/kunit/.kunitconfig --raw_output

KTAP version 1
1..1
KTAP version 1
# Subtest: kunit_global_stub
# module: kunit_test
1..4
# kunit_global_stub_test_activate: real_void_func: redirecting to replacement_void_func
# kunit_global_stub_test_activate: real_func: redirecting to replacement_func
# kunit_global_stub_test_activate: real_func: redirecting to replacement_func
# kunit_global_stub_test_activate: real_func: redirecting to other_replacement_func
# kunit_global_stub_test_activate: real_func: redirecting to other_replacement_func
# kunit_global_stub_test_activate: real_func: redirecting to super_replacement_func
# kunit_global_stub_test_activate: real_func: redirecting to super_replacement_func
ok 1 kunit_global_stub_test_activate
ok 2 kunit_global_stub_test_deactivate
# kunit_global_stub_test_slow_deactivate: real_func: redirecting to slow_replacement_func
# kunit_global_stub_test_slow_deactivate: real_func: redirecting to slow_replacement_func
# kunit_global_stub_test_slow_deactivate: waiting for slow_replacement_func
# kunit_global_stub_test_slow_deactivate.speed: slow
ok 3 kunit_global_stub_test_slow_deactivate
# kunit_global_stub_test_slow_replace: real_func: redirecting to slow_replacement_func
# kunit_global_stub_test_slow_replace: real_func: redirecting to slow_replacement_func
# kunit_global_stub_test_slow_replace: waiting for slow_replacement_func
# kunit_global_stub_test_slow_replace: real_func: redirecting to other_replacement_func
# kunit_global_stub_test_slow_replace.speed: slow
ok 4 kunit_global_stub_test_slow_replace
# kunit_global_stub: pass:4 fail:0 skip:0 total:4
# Totals: pass:4 fail:0 skip:0 total:4
ok 1 kunit_global_stub

Cc: Rae Moar <rm...@google.com>
Cc: David Gow <davi...@google.com>

Cc: Lucas De Marchi <lucas.d...@intel.com>

Michal Wajdeczko (6):

kunit: Introduce kunit_is_running()
kunit: Add macro to conditionally expose declarations to tests

kunit: Add macro to conditionally expose expressions to tests

kunit: Allow function redirection outside of the KUnit thread
kunit: Add example with alternate function redirection method

kunit: Add some selftests for global stub redirection macros

include/kunit/static_stub.h | 158 ++++++++++++++++++++
include/kunit/test-bug.h | 12 +-
include/kunit/visibility.h | 16 +++
lib/kunit/kunit-example-test.c | 67 +++++++++
lib/kunit/kunit-test.c | 254 ++++++++++++++++++++++++++++++++-
lib/kunit/static_stub.c | 49 +++++++
6 files changed, 553 insertions(+), 3 deletions(-)

--
2.43.0

[Michal Wajdeczko]

Wrap uses of the static key 'kunit_running' into a helper macro
to allow future checks to be placed in the code residing outside
of the CONFIG_KUNIT. We will start using this in upcoming patch.

Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>

Reviewed-by: Rae Moar <rm...@google.com>

Reviewed-by: Lucas De Marchi <lucas.d...@intel.com>

Reviewed-by: David Gow <davi...@google.com>
---

[Michal Wajdeczko]

The DECLARE_IF_KUNIT macro will introduces identifiers only if
CONFIG_KUNIT is enabled. Otherwise if CONFIG_KUNIT is not enabled
no identifiers from the param list will be defined.

Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>
Reviewed-by: Rae Moar <rm...@google.com>

Reviewed-by: David Gow <davi...@google.com>
---

Cc: Lucas De Marchi <lucas.d...@intel.com>

---

[Michal Wajdeczko]

The ONLY_IF_KUNIT macro will add expression statement only if the

CONFIG_KUNIT is enabled. Otherwise if CONFIG_KUNIT is not enabled

it will evaluate always to 0.

Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>
---

Cc: Rae Moar <rm...@google.com>
Cc: David Gow <davi...@google.com>
Cc: Lucas De Marchi <lucas.d...@intel.com>

---
include/kunit/visibility.h | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/include/kunit/visibility.h b/include/kunit/visibility.h

index 1c23773f826c..69c71eacf368 100644
--- a/include/kunit/visibility.h
+++ b/include/kunit/visibility.h
@@ -18,6 +18,13 @@

* @body: identifiers to be introduced conditionally

*/
#define DECLARE_IF_KUNIT(body...) body
+ /**
+ * ONLY_IF_KUNIT - A macro that adds expression statement only if

+ * CONFIG_KUNIT is enabled. Otherwise if CONFIG_KUNIT is not enabled

+ * it will evaluate always to 0.
+ * @expr: expression to be introduced conditionally
+ */
+ #define ONLY_IF_KUNIT(expr...) expr

/**
* VISIBLE_IF_KUNIT - A macro that sets symbols to be static if
* CONFIG_KUNIT is not enabled. Otherwise if CONFIG_KUNIT is enabled

@@ -34,6 +41,7 @@
EXPORTED_FOR_KUNIT_TESTING)
#else
#define DECLARE_IF_KUNIT(body...)
+ #define ONLY_IF_KUNIT(expr...) 0

[Michal Wajdeczko]

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 functionality 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.

These new stubs, named 'global', must be declared using dedicated
KUNIT_DECLARE_GLOBAL_STUB() macro and then can be placed either as
global static variables or as part of the other structures.

To properly maintain stubs lifecycle, they can be activated only
from the main KUnit context. Some precaution is taken to avoid
changing or deactivating replacement functions if one is still
used by other thread.

Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>
---

Cc: Rae Moar <rm...@google.com>
Cc: David Gow <davi...@google.com>

Cc: Daniel Latypov <dlat...@google.com>

Cc: Lucas De Marchi <lucas.d...@intel.com>

---
v2: s/FIXED_STUB/GLOBAL_STUB (David, Lucas)

make it little more thread safe (Rae, David)
wait until stub call finishes before test end (David)
wait until stub call finishes before changing stub (David)
allow stub deactivation (Rae)
prefer kunit log (David)

---
include/kunit/static_stub.h | 158 ++++++++++++++++++++++++++++++++++++
lib/kunit/static_stub.c | 49 +++++++++++
2 files changed, 207 insertions(+)

diff --git a/include/kunit/static_stub.h b/include/kunit/static_stub.h
index bf940322dfc0..42a70dcefb56 100644
--- a/include/kunit/static_stub.h
+++ b/include/kunit/static_stub.h
@@ -12,12 +12,15 @@

/* If CONFIG_KUNIT is not enabled, these stubs quietly disappear. */
#define KUNIT_STATIC_STUB_REDIRECT(real_fn_name, args...) do {} while (0)

+#define KUNIT_GLOBAL_STUB_REDIRECT(stub_name, args...) do {} while (0)
+#define KUNIT_DECLARE_GLOBAL_STUB(stub_name, stub_type)

#else

#include <kunit/test.h>
#include <kunit/test-bug.h>

+#include <linux/cleanup.h> /* for CLASS */
#include <linux/compiler.h> /* for {un,}likely() */
#include <linux/sched.h> /* for task_struct */

@@ -109,5 +112,160 @@ void __kunit_activate_static_stub(struct kunit *test,

*/
void kunit_deactivate_static_stub(struct kunit *test, void *real_fn_addr);

+/**

+ * struct kunit_global_stub - Represents a context of global function stub.
+ * @replacement: The address of replacement function.
+ * @owner: The KUnit test that owns the stub, valid only when @busy > 0.
+ * @busy: The stub busyness counter incremented on entry to the replacement
+ * function, decremented on exit, used to signal if the stub is idle.
+ * @idle: The completion state to indicate when the stub is idle again.
+ *
+ * This structure is for KUnit internal use only.
+ * See KUNIT_DECLARE_GLOBAL_STUB().
+ */
+struct kunit_global_stub {
+ void *replacement;
+ struct kunit *owner;
+ atomic_t busy;
+ struct completion idle;
+};
+
+/**
+ * KUNIT_DECLARE_GLOBAL_STUB() - Declare a global function stub.
+ * @stub_name: The name of the stub, must be a valid identifier
+ * @stub_type: The type of the function that this stub will replace
+ *
+ * This macro will declare new identifier of an anonymous type that will
+ * represent global stub function that could be used by KUnit. It can be stored
+ * outside of the KUnit code. If the CONFIG_KUNIT is not enabled this will
+ * be evaluated to an empty statement.
+ *
+ * The anonymous type introduced by this macro is mostly a wrapper to generic
+ * struct kunit_global_stub but with additional dummy member, that is never
+ * used directly, but is needed to maintain the type of the stub function.
+ */
+#define KUNIT_DECLARE_GLOBAL_STUB(stub_name, stub_type) \
+union { \
+ struct kunit_global_stub base; \
+ typeof(stub_type) dummy; \
+} stub_name
+
+/* Internal struct to define guard class */
+struct kunit_global_stub_guard {
+ struct kunit_global_stub *stub;
+ void *active_replacement;
+};
+
+/* Internal class used to guard stub calls */
+DEFINE_CLASS(kunit_global_stub_guard,
+ struct kunit_global_stub_guard,
+ ({
+ struct kunit_global_stub *stub = _T.stub;
+ bool active = !!_T.active_replacement;
+
+ if (active && !atomic_dec_return(&stub->busy))
+ complete_all(&stub->idle);
+ }),
+ ({
+ class_kunit_global_stub_guard_t guard;
+ bool active = !!atomic_inc_not_zero(&stub->busy);
+
+ guard.stub = stub;
+ guard.active_replacement = active ? READ_ONCE(stub->replacement) : NULL;
+
+ guard;
+ }),
+ struct kunit_global_stub *stub)
+
+/**
+ * KUNIT_GLOBAL_STUB_REDIRECT() - Call a fixed function stub if activated.
+ * @stub: The function stub declared using KUNIT_DECLARE_GLOBAL_STUB()

+ * @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 KUnit is not
+ * running or stub is not yet activated the function will continue execution
+ * as normal.
+ *
+ * The function stub must be declared with KUNIT_DECLARE_GLOBAL_STUB() that is
+ * stored in a place that is accessible from both the test code, which will
+ * activate this stub using kunit_activate_global_stub(), and from the function,
+ * where we will do this redirection using KUNIT_GLOBAL_STUB_REDIRECT().

+ *
+ * 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
+ *

+ * KUNIT_DECLARE_GLOBAL_STUB(func_stub, int (*)(int n));

+ *
+ * int real_func(int n)
+ * {

+ * KUNIT_GLOBAL_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);

+ * kunit_activate_global_stub(test, func_stub, replacement_func);

+ * KUNIT_EXPECT_EQ(test, real_func(1), 101);
+ * }
+ */

+#define KUNIT_GLOBAL_STUB_REDIRECT(stub, args...) do { \
+ if (kunit_is_running()) { \
+ typeof(stub) *__stub = &(stub); \
+ CLASS(kunit_global_stub_guard, guard)(&__stub->base); \
+ typeof(__stub->dummy) replacement = guard.active_replacement; \
+ if (unlikely(replacement)) { \
+ kunit_info(__stub->base.owner, "%s: redirecting to %ps\n", \

+ __func__, replacement); \
+ return replacement(args); \
+ } \
+ } \
+} while (0)
+

+void __kunit_activate_global_stub(struct kunit *test, struct kunit_global_stub *stub,
+ void *replacement_addr);
+
+/**
+ * kunit_activate_global_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 deactivate the stub at the test end.
+ *
+ * The redirection can be disabled with kunit_deactivate_global_stub().
+ * The stub must be declared using KUNIT_DECLARE_GLOBAL_STUB().
+ */
+#define kunit_activate_global_stub(test, stub, replacement) do { \
+ typeof(stub) *__stub = &(stub); \
+ typecheck_fn(typeof(__stub->dummy), (replacement)); \
+ __kunit_activate_global_stub((test), &__stub->base, (replacement)); \
+} while (0)
+
+void __kunit_deactivate_global_stub(struct kunit *test, struct kunit_global_stub *stub);
+
+/**
+ * kunit_deactivate_global_stub() - Disable a fixed function stub.

+ * @test: Test case that wants to deactivate a fixed function stub

+ * @stub: The location of the function stub pointer
+ *

+ * The stub must be declared using KUNIT_DECLARE_GLOBAL_STUB().
+ */
+#define kunit_deactivate_global_stub(test, stub) do { \
+ typeof(stub) *__stub = &(stub); \
+ __kunit_deactivate_global_stub((test), &__stub->base); \

+} while (0)
+
#endif
#endif
diff --git a/lib/kunit/static_stub.c b/lib/kunit/static_stub.c

index 92b2cccd5e76..799a7271dc5b 100644
--- a/lib/kunit/static_stub.c
+++ b/lib/kunit/static_stub.c
@@ -121,3 +121,52 @@ void __kunit_activate_static_stub(struct kunit *test,
}
}
EXPORT_SYMBOL_GPL(__kunit_activate_static_stub);
+
+static void sanitize_global_stub(void *data)
+{
+ struct kunit *test = kunit_get_current_test();
+ struct kunit_global_stub *stub = data;
+
+ KUNIT_EXPECT_NE(test, 0, atomic_read(&stub->busy));
+ KUNIT_EXPECT_PTR_EQ(test, test, READ_ONCE(stub->owner));
+
+ reinit_completion(&stub->idle);
+ if (!atomic_dec_and_test(&stub->busy)) {
+ kunit_info(test, "waiting for %ps\n", stub->replacement);
+ KUNIT_EXPECT_EQ(test, 0, wait_for_completion_interruptible(&stub->idle));
+ }
+
+ WRITE_ONCE(stub->owner, NULL);
+ WRITE_ONCE(stub->replacement, NULL);
+}
+
+/*
+ * Helper function for kunit_activate_global_stub(). The macro does

+ * typechecking, so use it instead.
+ */

+void __kunit_activate_global_stub(struct kunit *test,
+ struct kunit_global_stub *stub,
+ void *replacement_addr)
+{
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stub);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, replacement_addr);
+ if (atomic_read(&stub->busy))
+ kunit_release_action(test, sanitize_global_stub, stub);
+ else
+ init_completion(&stub->idle);
+ WRITE_ONCE(stub->owner, test);
+ WRITE_ONCE(stub->replacement, replacement_addr);
+ KUNIT_ASSERT_EQ(test, 1, atomic_inc_return(&stub->busy));
+ KUNIT_ASSERT_EQ(test, 0, kunit_add_action_or_reset(test, sanitize_global_stub, stub));
+}
+EXPORT_SYMBOL_GPL(__kunit_activate_global_stub);
+
+/*
+ * Helper function for kunit_deactivate_global_stub(). Use it instead.
+ */
+void __kunit_deactivate_global_stub(struct kunit *test, struct kunit_global_stub *stub)
+{
+ if (atomic_read(&stub->busy))
+ kunit_release_action(test, sanitize_global_stub, stub);
+}
+EXPORT_SYMBOL_GPL(__kunit_deactivate_global_stub);
--
2.43.0

[Michal Wajdeczko]

Add example how to use KUNIT_FIXED_STUB_REDIRECT and compare its
usage with the KUNIT_STATIC_STUB_REDIRECT. Also show how the
DECLARE_IF_KUNIT macro could be helpful in declaring test data in
the non-test data structures.

Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>
Reviewed-by: Rae Moar <rm...@google.com> #v1
Reviewed-by: David Gow <davi...@google.com> #v1

---
Cc: Daniel Latypov <dlat...@google.com>

Cc: Lucas De Marchi <lucas.d...@intel.com>

---
v2: add missing testcase description (Rae) and rebase
---
lib/kunit/kunit-example-test.c | 67 ++++++++++++++++++++++++++++++++++
1 file changed, 67 insertions(+)

diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
index 3056d6bc705d..146935a16883 100644

--- a/lib/kunit/kunit-example-test.c
+++ b/lib/kunit/kunit-example-test.c
@@ -6,8 +6,10 @@
* Author: Brendan Higgins <brendan...@google.com>
*/

+#include <linux/workqueue.h>
#include <kunit/test.h>
#include <kunit/static_stub.h>
+#include <kunit/visibility.h>

/*
* This is the most fundamental element of KUnit, the test case. A test case

@@ -221,6 +223,70 @@ static void example_static_stub_using_fn_ptr_test(struct kunit *test)
KUNIT_EXPECT_EQ(test, add_one(1), 2);
}

+/* This could be a location of various global stub functions. */
+static struct {
+ KUNIT_DECLARE_GLOBAL_STUB(add_two, int (*)(int i));

+} stubs;
+
+/* This is a function we'll replace with stubs. */
+static int add_two(int i)
+{
+ /* This will trigger the stub if active. */
+ KUNIT_STATIC_STUB_REDIRECT(add_two, i);

+ KUNIT_GLOBAL_STUB_REDIRECT(stubs.add_two, i);

+}
+
+/*
+ * This test shows how to use KUNIT_GLOBAL_STUB_REDIRECT and compares its
+ * usage with the KUNIT_STATIC_STUB_REDIRECT.
+ */
+static void example_global_stub_test(struct kunit *test)

+{
+ /* static stub redirection works only for KUnit thread */
+ kunit_activate_static_stub(test, add_two, subtract_one);
+ KUNIT_EXPECT_EQ(test, add_two(1), subtract_one(1));
+ KUNIT_EXPECT_NE_MSG(test, add_two_async(1), subtract_one(1),
+ "stub shouldn't be active outside KUnit thread!");
+
+ kunit_deactivate_static_stub(test, add_two);
+ KUNIT_EXPECT_EQ(test, add_two(1), add_two(1));
+
+ /* fixed stub redirection works for KUnit and other threads */

+ kunit_activate_global_stub(test, stubs.add_two, subtract_one);

+ KUNIT_EXPECT_EQ(test, add_two(1), subtract_one(1));
+ KUNIT_EXPECT_EQ(test, add_two_async(1), subtract_one(1));
+

+ kunit_deactivate_global_stub(test, stubs.add_two);

+ KUNIT_EXPECT_EQ(test, add_two(1), add_two(1));
+}
+
static const struct example_param {
int value;
} example_params_array[] = {

@@ -294,6 +360,7 @@ static struct kunit_case example_test_cases[] = {
KUNIT_CASE(example_all_expect_macros_test),
KUNIT_CASE(example_static_stub_test),
KUNIT_CASE(example_static_stub_using_fn_ptr_test),
+ KUNIT_CASE(example_global_stub_test),

[Michal Wajdeczko]

While we already have few ASSERT() within the implementation,
it's always better to have dedicated test cases. Add tests for:
- automatic deactivation of the stubs at the test end
- blocked deactivation until all active stub calls finish
- blocked stub change until all active stub calls finish
- safe abuse (deactivation without activation)

Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>
---

Cc: Rae Moar <rm...@google.com>
Cc: David Gow <davi...@google.com>
Cc: Lucas De Marchi <lucas.d...@intel.com>

---
lib/kunit/kunit-test.c | 254 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 253 insertions(+), 1 deletion(-)

diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
index 37e02be1e710..eb1bb312ad71 100644
--- a/lib/kunit/kunit-test.c
+++ b/lib/kunit/kunit-test.c

@@ -6,8 +6,10 @@
* Author: Brendan Higgins <brendan...@google.com>
*/

#include "linux/gfp_types.h"
+#include <kunit/static_stub.h>
#include <kunit/test.h>
#include <kunit/test-bug.h>
+#include <kunit/visibility.h>

#include <linux/device.h>
#include <kunit/device.h>
@@ -866,10 +868,260 @@ static struct kunit_suite kunit_current_test_suite = {
.test_cases = kunit_current_test_cases,
};

+static struct {
+ /* this stub matches the real function */
+ KUNIT_DECLARE_GLOBAL_STUB(first_stub, int (*)(int i));
+ /* this stub matches only return type of the real function */
+ KUNIT_DECLARE_GLOBAL_STUB(second_stub, int (*)(int bit, int data));
+ /* this is an example stub that returns void */
+ KUNIT_DECLARE_GLOBAL_STUB(void_stub, void (*)(void));
+ /* this is an example how to store additional data for use by stubs */
+ DECLARE_IF_KUNIT(int data);
+ DECLARE_IF_KUNIT(int counter);
+} stubs = {
+ DECLARE_IF_KUNIT(.data = 3),
+};
+
+static int real_func(int i)
+{
+ KUNIT_GLOBAL_STUB_REDIRECT(stubs.first_stub, i);
+ KUNIT_GLOBAL_STUB_REDIRECT(stubs.second_stub, BIT(i), stubs.data);

+
+ return i;
+}

+
+struct real_work {

+ struct work_struct work;
+ int param;
+ int result;
+};
+

+static void real_work_func(struct work_struct *work)
+{
+ struct real_work *w = container_of(work, typeof(*w), work);
+
+ w->result = real_func(w->param);
+}
+
+static int real_func_async(int i)
+{
+ struct real_work w = { .param = i, .result = -EINPROGRESS };
+
+ INIT_WORK_ONSTACK(&w.work, real_work_func);

+ schedule_work(&w.work);
+ flush_work(&w.work);
+ destroy_work_on_stack(&w.work);
+
+ return w.result;
+}
+

+static int replacement_func(int i)
+{
+ return i + 1;
+}
+
+static int other_replacement_func(int i)
+{
+ return i + 10;
+}
+
+static int super_replacement_func(int bit, int data)
+{
+ return bit * data;
+}
+
+static int slow_replacement_func(int i)
+{
+ schedule_timeout_interruptible(HZ / 20);
+ return replacement_func(i);
+}
+
+static void real_void_func(void)
+{
+ KUNIT_GLOBAL_STUB_REDIRECT(stubs.void_stub);
+ DECLARE_IF_KUNIT(stubs.counter++);
+}
+
+static void replacement_void_func(void)
+{
+ stubs.counter--;
+}
+
+static void expect_deactivated(void *data)

+{
+ struct kunit *test = kunit_get_current_test();
+

+ KUNIT_EXPECT_NULL(test, stubs.first_stub.base.owner);
+ KUNIT_EXPECT_NULL(test, stubs.first_stub.base.replacement);
+ KUNIT_EXPECT_NULL(test, stubs.second_stub.base.owner);
+ KUNIT_EXPECT_NULL(test, stubs.second_stub.base.replacement);
+ KUNIT_EXPECT_NULL(test, stubs.void_stub.base.owner);
+ KUNIT_EXPECT_NULL(test, stubs.void_stub.base.replacement);
+}
+
+static void kunit_global_stub_test_deactivate(struct kunit *test)
+{
+ /* make sure everything will be deactivated */
+ KUNIT_ASSERT_EQ(test, 0, kunit_add_action_or_reset(test, expect_deactivated, test));
+
+ /* deactivate without activate */
+ kunit_deactivate_global_stub(test, stubs.first_stub);
+
+ /* deactivate twice */
+ kunit_deactivate_global_stub(test, stubs.first_stub);
+
+ /* allow to skip deactivation (will be tested by expect_deactivated action) */
+ kunit_activate_global_stub(test, stubs.first_stub, replacement_func);
+}
+
+static void kunit_global_stub_test_activate(struct kunit *test)
+{
+ int real, replacement, other, super, i = 2;
+
+ /* prerequisites */
+ real_void_func();
+ KUNIT_ASSERT_EQ(test, stubs.counter, 1);
+ replacement_void_func();
+ KUNIT_ASSERT_EQ(test, stubs.counter, 0);
+
+ /* prerequisites cont'd */
+ KUNIT_ASSERT_EQ(test, real_func(i), real = real_func_async(i));
+ KUNIT_ASSERT_NE(test, real_func(i), replacement = replacement_func(i));
+ KUNIT_ASSERT_NE(test, real_func(i), other = other_replacement_func(i));
+ KUNIT_ASSERT_NE(test, real_func(i), super = super_replacement_func(BIT(i), stubs.data));
+
+ /* make sure everything will be deactivated */
+ KUNIT_ASSERT_EQ(test, 0, kunit_add_action_or_reset(test, expect_deactivated, test));
+
+ /* allow to activate replacement */
+ kunit_activate_global_stub(test, stubs.void_stub, replacement_void_func);
+ real_void_func();
+ KUNIT_ASSERT_EQ(test, stubs.counter, -1);
+
+ /* allow to activate replacement */
+ kunit_activate_global_stub(test, stubs.first_stub, replacement_func);
+ KUNIT_EXPECT_EQ(test, real_func(i), replacement);
+ KUNIT_EXPECT_EQ(test, real_func_async(i), replacement);
+
+ /* allow to change replacement */
+ kunit_activate_global_stub(test, stubs.first_stub, other_replacement_func);
+ KUNIT_EXPECT_EQ(test, real_func(i), other);
+ KUNIT_EXPECT_EQ(test, real_func_async(i), other);
+
+ /* allow to deactivate replacement */
+ kunit_deactivate_global_stub(test, stubs.first_stub);
+ KUNIT_EXPECT_EQ(test, real_func(i), real);
+ KUNIT_EXPECT_EQ(test, real_func_async(i), real);
+
+ /* allow to activate replacement with different arguments */
+ kunit_activate_global_stub(test, stubs.second_stub, super_replacement_func);
+ KUNIT_EXPECT_EQ(test, real_func(i), super);
+ KUNIT_EXPECT_EQ(test, real_func_async(i), super);
+
+ /* allow to deactivate twice */
+ kunit_deactivate_global_stub(test, stubs.second_stub);
+ kunit_deactivate_global_stub(test, stubs.second_stub);
+ KUNIT_EXPECT_EQ(test, real_func_async(i), real);
+ KUNIT_EXPECT_EQ(test, real_func(i), real);
+}
+
+static void flush_real_work(void *data)
+{
+ struct real_work *w = data;
+
+ flush_work(&w->work);
+}
+
+static void __kunit_global_stub_test_slow(struct kunit *test, bool replace)
+{
+ int real, replacement, other, i = replace ? 3 : 5;
+ struct real_work *w;
+
+ /* prerequisites */
+ KUNIT_ASSERT_EQ(test, real_func(i), real = real_func_async(i));
+ KUNIT_ASSERT_NE(test, real_func(i), replacement = slow_replacement_func(i));
+ KUNIT_ASSERT_NE(test, real_func(i), other = other_replacement_func(i));
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, w = kunit_kzalloc(test, sizeof(*w), GFP_KERNEL));
+ INIT_WORK(&w->work, real_work_func);
+ KUNIT_ASSERT_EQ(test, 0, kunit_add_action_or_reset(test, flush_real_work, w));
+ KUNIT_ASSERT_EQ(test, 0, kunit_add_action_or_reset(test, expect_deactivated, test));
+
+ /* allow to activate replacement */
+ kunit_activate_global_stub(test, stubs.first_stub, slow_replacement_func);
+ KUNIT_EXPECT_EQ(test, real_func_async(i), replacement);
+
+ w->param = i;
+ w->result = 0;
+ queue_work(system_long_wq, &w->work);
+
+ /* wait until work starts */
+ while (work_pending(&w->work))
+ schedule_timeout_interruptible(HZ / 100);
+ KUNIT_EXPECT_NE(test, work_busy(&w->work), 0);
+
+ /* wait until work enters the stub */
+ while (atomic_read(&stubs.first_stub.base.busy) < 2)
+ schedule_timeout_interruptible(HZ / 100);
+
+ /* stub should be still busy(2) at this point */
+ KUNIT_EXPECT_EQ(test, 2, atomic_read(&stubs.first_stub.base.busy));
+ KUNIT_EXPECT_EQ(test, w->result, 0);
+
+ if (replace) {
+ /* try replace the stub, it should be just activated(1) */
+ kunit_activate_global_stub(test, stubs.first_stub, other_replacement_func);
+ KUNIT_EXPECT_EQ(test, 1, atomic_read(&stubs.first_stub.base.busy));
+ } else {
+ /* try to deactivate the stub, it should be disabled(0) */
+ kunit_deactivate_global_stub(test, stubs.first_stub);
+ KUNIT_EXPECT_EQ(test, 0, atomic_read(&stubs.first_stub.base.busy));
+ }
+
+ /* and results from the worker should be available */
+ KUNIT_EXPECT_EQ(test, w->result, replacement);
+ KUNIT_EXPECT_NE(test, w->result, real);
+ KUNIT_EXPECT_NE(test, w->result, other);
+
+ if (replace)
+ KUNIT_EXPECT_EQ(test, real_func_async(i), other);
+ else
+ KUNIT_EXPECT_EQ(test, real_func_async(i), real);
+}
+
+static void kunit_global_stub_test_slow_deactivate(struct kunit *test)
+{
+ __kunit_global_stub_test_slow(test, false);
+}
+
+static void kunit_global_stub_test_slow_replace(struct kunit *test)
+{
+ __kunit_global_stub_test_slow(test, true);
+}
+
+static int kunit_global_stub_test_init(struct kunit *test)
+{
+ stubs.counter = 0;
+ return 0;
+}
+
+static struct kunit_case kunit_global_stub_test_cases[] = {
+ KUNIT_CASE(kunit_global_stub_test_activate),
+ KUNIT_CASE(kunit_global_stub_test_deactivate),
+ KUNIT_CASE_SLOW(kunit_global_stub_test_slow_deactivate),
+ KUNIT_CASE_SLOW(kunit_global_stub_test_slow_replace),
+ {}
+};
+
+static struct kunit_suite kunit_global_stub_suite = {
+ .name = "kunit_global_stub",
+ .init = kunit_global_stub_test_init,
+ .test_cases = kunit_global_stub_test_cases,
+};
+
kunit_test_suites(&kunit_try_catch_test_suite, &kunit_resource_test_suite,
&kunit_log_test_suite, &kunit_status_test_suite,
&kunit_current_test_suite, &kunit_device_test_suite,
- &kunit_fault_test_suite);
+ &kunit_fault_test_suite, &kunit_global_stub_suite);

MODULE_DESCRIPTION("KUnit test for core test infrastructure");
MODULE_LICENSE("GPL v2");
--
2.43.0

[Michal Wajdeczko]

v2 is trying to wait until all active calls finishes before changing or
deactivating this particular stub, I've also added simple test for it:

KTAP version 1
1..1
KTAP version 1
# Subtest: kunit_global_stub
# module: kunit_test

1..2

# kunit_global_stub_test_slow_deactivate: real_func: redirecting to
slow_replacement_func
# kunit_global_stub_test_slow_deactivate: real_func: redirecting to
slow_replacement_func
# kunit_global_stub_test_slow_deactivate: waiting for
slow_replacement_func
# kunit_global_stub_test_slow_deactivate.speed: slow

ok 1 kunit_global_stub_test_slow_deactivate

# kunit_global_stub_test_slow_replace: real_func: redirecting to
slow_replacement_func
# kunit_global_stub_test_slow_replace: real_func: redirecting to
slow_replacement_func
# kunit_global_stub_test_slow_replace: waiting for slow_replacement_func
# kunit_global_stub_test_slow_replace: real_func: redirecting to
other_replacement_func
# kunit_global_stub_test_slow_replace.speed: slow

ok 2 kunit_global_stub_test_slow_replace
# kunit_global_stub: pass:2 fail:0 skip:0 total:2
# Totals: pass:2 fail:0 skip:0 total:2

>
> More rambling below, but I think this is probably good once the
> atomic/thread-safety stuff is either sorted out or at least
> documented. As for the name, how about KUNIT_GLOBAL_STUB_REDIRECT()?
> I'm okay with FIXED_STUB if you prefer it, though.

renamed to KUNIT_GLOBAL_STUB_REDIRECT

as in v2 there is a dedicated struct/union to hold the address and the
type of the replacement function, I've also added there a kunit* so is
now everything is logged to the test log

>
>> + __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 { \
>
> Do we want to actually hook this into the struct kunit here? I suspect

yes, as we want automatic deactivation of the stub to avoid leaving
active stub after the test ends (may be important if using KUnit for
some live testing, like we do in Xe)

> we do, but it does mean that this has to either be called from the
> main thread, or the struct kunit* needs to be passed around.

we usually enable/activate stubs from the test case code, where kunit*
is already there, so this just enforces that redirection could be
controlled only from the main KUnit thread

v2 is using READ_ONCE()/WRITE_ONCE() to update stub pointers and
atomic_t to track whether stub is disabled(0) or activated(1) or in_use(2+)

>
> In general, though, what's the rule around activating a stub from a
> different thread to it being called? I thinki we definitely want to
> allow it, but _maybe_ we can get away with saying that the stub can't
> be activated/deactivated concurrently with being used?
>
>> + KUNIT_ASSERT_EQ(test, 0, kunit_add_action_or_reset(test, nullify_stub_ptr, stub_ptr));
>
> Again, we probably need to emphasise that any work done on another
> thread needs to be complete before the test ends on the main thread.
> This isn't specific to this feature, though.

v2 is trying to care of it and waits until any active calls finish

>
>> +}
>> +EXPORT_SYMBOL_GPL(__kunit_activate_fixed_stub);
>
>> --
>> 2.43.0
>>

[Lucas De Marchi]

On Tue, Aug 27, 2024 at 12:20:11AM GMT, Michal Wajdeczko wrote:
>The DECLARE_IF_KUNIT macro will introduces identifiers only if
>CONFIG_KUNIT is enabled. Otherwise if CONFIG_KUNIT is not enabled
>no identifiers from the param list will be defined.
>
>Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>
>Reviewed-by: Rae Moar <rm...@google.com>
>Reviewed-by: David Gow <davi...@google.com>
>---
>Cc: Lucas De Marchi <lucas.d...@intel.com>

up to kunit maintainers, but it doesn't seem the word "DECLARE" is
entirely correct. What it's doing is expanding arg, and it doesn't
matter if it's an expression, definition, declaration.

Looking at patch 3, I think it would be more obvious to the caller if we
have:

IF_KUNIT_ELSE_EMPTY()
IF_KUNIT_ELSE_ZERO()

>---
> include/kunit/visibility.h | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
>diff --git a/include/kunit/visibility.h b/include/kunit/visibility.h
>index 0dfe35feeec6..1c23773f826c 100644
>--- a/include/kunit/visibility.h
>+++ b/include/kunit/visibility.h
>@@ -11,6 +11,13 @@
> #define _KUNIT_VISIBILITY_H
>
> #if IS_ENABLED(CONFIG_KUNIT)
>+ /**
>+ * DECLARE_IF_KUNIT - A macro that introduces identifiers only if
>+ * CONFIG_KUNIT is enabled. Otherwise if CONFIG_KUNIT is not enabled
>+ * no identifiers will be defined.
>+ * @body: identifiers to be introduced conditionally

missing an example here with fields inside a struct

Lucas De Marchi

[Lucas De Marchi]

On Tue, Aug 27, 2024 at 12:20:14AM GMT, Michal Wajdeczko wrote:
>Add example how to use KUNIT_FIXED_STUB_REDIRECT and compare its

s/FIXED/GLOBAL/

Nice use of an outer storage so we can give it the same name. IMO this
should be encouraged.

s/fixed/global/


with those fixes,

Reviewed-by: Lucas De Marchi <lucas.d...@intel.com>

thanks
Lucas De Marchi

[Lucas De Marchi]

I'd call this `bool active_replacement` as it's not the same thing as
the active below.

s/fixed/global/ here and every where else below

shouldn't sanitize_ be unconditional and do nothing in this case?

>+ KUNIT_EXPECT_PTR_EQ(test, test, READ_ONCE(stub->owner));
>+
>+ reinit_completion(&stub->idle);
>+ if (!atomic_dec_and_test(&stub->busy)) {
>+ kunit_info(test, "waiting for %ps\n", stub->replacement);
>+ KUNIT_EXPECT_EQ(test, 0, wait_for_completion_interruptible(&stub->idle));

what's preventing stub->busy going to 1 again after this?

Lucas De Marchi

[Rae Moar]

On Mon, Aug 26, 2024 at 3:20 PM Michal Wajdeczko
<michal.w...@intel.com> wrote:
>
> The ONLY_IF_KUNIT macro will add expression statement only if the
> CONFIG_KUNIT is enabled. Otherwise if CONFIG_KUNIT is not enabled
> it will evaluate always to 0.
>
> Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>

Hello!

Thanks for the second version of this patch series!

I definitely could see this new macro as being useful but I currently
don't see an example of its use in the rest of the patch series. How
do you see this macro as being used or do you have a current use case
for this macro?

I would be fine adding this macro without being used as long as
examples on how and why to use it are clearly documented.

Thanks!
-Rae

[Michal Wajdeczko]

On 27.08.2024 21:04, Rae Moar wrote:
> On Mon, Aug 26, 2024 at 3:20 PM Michal Wajdeczko
> <michal.w...@intel.com> wrote:
>>
>> The ONLY_IF_KUNIT macro will add expression statement only if the
>> CONFIG_KUNIT is enabled. Otherwise if CONFIG_KUNIT is not enabled
>> it will evaluate always to 0.
>>
>> Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>
>
> Hello!
>
> Thanks for the second version of this patch series!
>
> I definitely could see this new macro as being useful but I currently
> don't see an example of its use in the rest of the patch series. How
> do you see this macro as being used or do you have a current use case
> for this macro?

in Xe driver we have this macro defined as XE_TEST_ONLY [1]

[1] https://elixir.bootlin.com/linux/v6.11-rc5/A/ident/XE_TEST_ONLY

>
> I would be fine adding this macro without being used as long as
> examples on how and why to use it are clearly documented.

sure, I'll try to add some usage in the example patch 5/6

[Michal Wajdeczko]

On 27.08.2024 15:45, Lucas De Marchi wrote:
> On Tue, Aug 27, 2024 at 12:20:11AM GMT, Michal Wajdeczko wrote:
>> The DECLARE_IF_KUNIT macro will introduces identifiers only if
>> CONFIG_KUNIT is enabled. Otherwise if CONFIG_KUNIT is not enabled
>> no identifiers from the param list will be defined.
>>
>> Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>
>> Reviewed-by: Rae Moar <rm...@google.com>
>> Reviewed-by: David Gow <davi...@google.com>
>> ---
>> Cc: Lucas De Marchi <lucas.d...@intel.com>
>
> up to kunit maintainers, but it doesn't seem the word "DECLARE" is
> entirely correct. What it's doing is expanding arg, and it doesn't
> matter if it's an expression, definition, declaration.

hmm, while this is true for statement & declarations (as both have
similar notation) but not sure about the expression (that's why we have
patch 3)

>
> Looking at patch 3, I think it would be more obvious to the caller if we
> have:
>
> IF_KUNIT_ELSE_EMPTY()
> IF_KUNIT_ELSE_ZERO()

existing macros in this file are named as xxx_IF_KUNIT so maybe we
should try to follow that pattern...

so maybe (like BUILD_BUG_ON_ZERO)

ONLY_IF_KUNIT(body...)
ONLY_IF_KUNIT_ZERO(expr...)

>
>> ---
>> include/kunit/visibility.h | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/include/kunit/visibility.h b/include/kunit/visibility.h
>> index 0dfe35feeec6..1c23773f826c 100644
>> --- a/include/kunit/visibility.h
>> +++ b/include/kunit/visibility.h
>> @@ -11,6 +11,13 @@
>> #define _KUNIT_VISIBILITY_H
>>
>> #if IS_ENABLED(CONFIG_KUNIT)
>> +    /**
>> +     * DECLARE_IF_KUNIT - A macro that introduces identifiers only if
>> +     * CONFIG_KUNIT is enabled. Otherwise if CONFIG_KUNIT is not enabled
>> +     * no identifiers will be defined.
>> +     * @body: identifiers to be introduced conditionally
>
> missing an example here with fields inside a struct

would this work?

Example:

struct example {
int foo;
/* private: test only */
DECLARE_IF_KUNIT(int bar);
};

[Michal Wajdeczko]

IMO 'active_replacement' would be even more confusing as by that name we
identify the address and here it's a flag

OTOH the 'active' in both places means more/less the same (in init below
it mean stub was 'activated' and in exit here that we used 'activated'
replacement function)

oops

I just didn't like early return here, but maybe it's more correct

>
>> +    KUNIT_EXPECT_PTR_EQ(test, test, READ_ONCE(stub->owner));
>> +
>> +    reinit_completion(&stub->idle);
>> +    if (!atomic_dec_and_test(&stub->busy)) {
>> +        kunit_info(test, "waiting for %ps\n", stub->replacement);
>> +        KUNIT_EXPECT_EQ(test, 0,
>> wait_for_completion_interruptible(&stub->idle));
>
> what's preventing stub->busy going to 1 again after this?

at the redirection point in kunit_global_stub_guard we have

atomic_inc_not_zero(&stub->busy);

and the activation/deactivation can only be done from the main KUnit
thread (which is here)

[Michal Wajdeczko]

hmm, in fact we don't need to check stub->busy prior to calling
kunit_release_action() since our goal is to detect whether stub was
activated, but this action will be released/called only if we have added
this action after the stub activation, so we can just rely on the action
management code and just keep the EXPECT_NE here as a guard

[Michal Wajdeczko]

v1: https://groups.google.com/g/kunit-dev/c/f4LIMLyofj8
v2: make it more complex and attempt to be thread safe

s/FIXED_STUB/GLOBAL_STUB (David, Lucas)
make it little more thread safe (Rae, David)
wait until stub call finishes before test end (David)
wait until stub call finishes before changing stub (David)
allow stub deactivation (Rae)
prefer kunit log (David)

add simple selftest (Michal)
also introduce ONLY_IF_KUNIT macro (Michal)

v3: include example for DECLARE_IF_KUNIT (Lucas)
rename s/ONLY_IF_KUNIT/VALUE_IF_KUNIT (Michal)
and add simple usage example for it (Rae)
fix s/fixed/global in comments (Lucas)
improve stub sanitize flow (Lucas, Michal)
reformat kernel-doc for better output (Michal)

Test outputs:

$ tools/testing/kunit/kunit.py run *example*.*global* \
--kunitconfig lib/kunit/.kunitconfig --raw_output

KTAP version 1
1..1
# example: initializing suite

KTAP version 1

# Subtest: example
# module: kunit_example_test
1..1
# example_global_stub_test: initializing
# example_global_stub_test: add_two: redirecting to subtract_one
# example_global_stub_test: add_two: redirecting to subtract_one
# example_global_stub_test: cleaning up
ok 1 example_global_stub_test
# example: exiting suite
ok 1 example

$ tools/testing/kunit/kunit.py run *global*.*global* \
--kunitconfig lib/kunit/.kunitconfig --raw_output

KTAP version 1
1..1
KTAP version 1
# Subtest: kunit_global_stub
# module: kunit_test

1..4
# kunit_global_stub_test_activate: real_void_func: redirecting to replacement_void_func
# kunit_global_stub_test_activate: real_func: redirecting to replacement_func
# kunit_global_stub_test_activate: real_func: redirecting to replacement_func
# kunit_global_stub_test_activate: real_func: redirecting to other_replacement_func
# kunit_global_stub_test_activate: real_func: redirecting to other_replacement_func
# kunit_global_stub_test_activate: real_func: redirecting to super_replacement_func
# kunit_global_stub_test_activate: real_func: redirecting to super_replacement_func
ok 1 kunit_global_stub_test_activate
ok 2 kunit_global_stub_test_deactivate

# kunit_global_stub_test_slow_deactivate: real_func: redirecting to slow_replacement_func
# kunit_global_stub_test_slow_deactivate: real_func: redirecting to slow_replacement_func
# kunit_global_stub_test_slow_deactivate: waiting for slow_replacement_func
# kunit_global_stub_test_slow_deactivate.speed: slow

ok 3 kunit_global_stub_test_slow_deactivate

# kunit_global_stub_test_slow_replace: real_func: redirecting to slow_replacement_func
# kunit_global_stub_test_slow_replace: real_func: redirecting to slow_replacement_func
# kunit_global_stub_test_slow_replace: waiting for slow_replacement_func
# kunit_global_stub_test_slow_replace: real_func: redirecting to other_replacement_func
# kunit_global_stub_test_slow_replace.speed: slow

ok 4 kunit_global_stub_test_slow_replace
# kunit_global_stub: pass:4 fail:0 skip:0 total:4

# Totals: pass:4 fail:0 skip:0 total:4
ok 1 kunit_global_stub

Cc: Rae Moar <rm...@google.com>
Cc: David Gow <davi...@google.com>

Cc: Lucas De Marchi <lucas.d...@intel.com>

Michal Wajdeczko (6):
kunit: Introduce kunit_is_running()
kunit: Add macro to conditionally expose declarations to tests
kunit: Add macro to conditionally expose expressions to tests
kunit: Allow function redirection outside of the KUnit thread
kunit: Add example with alternate function redirection method
kunit: Add some selftests for global stub redirection macros

include/kunit/static_stub.h | 158 ++++++++++++++++++++
include/kunit/test-bug.h | 12 +-

include/kunit/visibility.h | 40 ++++++

lib/kunit/kunit-example-test.c | 67 +++++++++
lib/kunit/kunit-test.c | 254 ++++++++++++++++++++++++++++++++-

lib/kunit/static_stub.c | 50 +++++++
6 files changed, 578 insertions(+), 3 deletions(-)

--
2.43.0

[Michal Wajdeczko]

Wrap uses of the static key 'kunit_running' into a helper macro
to allow future checks to be placed in the code residing outside
of the CONFIG_KUNIT. We will start using this in upcoming patch.

Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>
Reviewed-by: Rae Moar <rm...@google.com>

Reviewed-by: Lucas De Marchi <lucas.d...@intel.com>

Reviewed-by: David Gow <davi...@google.com>
---

[Michal Wajdeczko]

The DECLARE_IF_KUNIT macro will introduces identifiers only if
CONFIG_KUNIT is enabled. Otherwise if CONFIG_KUNIT is not enabled
no identifiers from the param list will be defined.

Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>
Reviewed-by: Rae Moar <rm...@google.com>

Reviewed-by: David Gow <davi...@google.com>
---

Cc: Lucas De Marchi <lucas.d...@intel.com>

---
v2: include struct example in macro comment (Lucas)

reformat kernel-doc for better output (Michal)

---
include/kunit/visibility.h | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)

diff --git a/include/kunit/visibility.h b/include/kunit/visibility.h
index 0dfe35feeec6..c1825381adfc 100644
--- a/include/kunit/visibility.h
+++ b/include/kunit/visibility.h
@@ -11,6 +11,25 @@

#define _KUNIT_VISIBILITY_H

#if IS_ENABLED(CONFIG_KUNIT)
+ /**

+ * DECLARE_IF_KUNIT - Conditionally introduce identifiers

+ * @body: identifiers to be introduced conditionally

+ *
+ * This macro introduces identifiers only if CONFIG_KUNIT is enabled.
+ * Otherwise if CONFIG_KUNIT is not enabled no identifiers will be defined.
+ *
+ * .. code-block:: C
+ *
+ * struct example {
+ * // @foo: regular data
+ * int foo;
+ *
+ * // private: data available only for CONFIG_KUNIT
+ * DECLARE_IF_KUNIT(int bar);
+ * };

+ */
+ #define DECLARE_IF_KUNIT(body...) body

+

/**
* VISIBLE_IF_KUNIT - A macro that sets symbols to be static if
* CONFIG_KUNIT is not enabled. Otherwise if CONFIG_KUNIT is enabled

@@ -26,6 +45,7 @@

[Michal Wajdeczko]

The VALUE_IF_KUNIT macro will add expression statement only if the

CONFIG_KUNIT is enabled. Otherwise if CONFIG_KUNIT is not enabled

it will evaluate always to 0.

Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>
---

Cc: Rae Moar <rm...@google.com>
Cc: David Gow <davi...@google.com>
Cc: Lucas De Marchi <lucas.d...@intel.com>

---
v2: add simple usage example (Rae)

reformat kernel-doc for better output (Michal)

s/ONLY_IF_KUNIT/VALUE_IF_KUNIT

---
include/kunit/visibility.h | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)

diff --git a/include/kunit/visibility.h b/include/kunit/visibility.h

index c1825381adfc..b156cafa4705 100644
--- a/include/kunit/visibility.h
+++ b/include/kunit/visibility.h
@@ -30,6 +30,25 @@
*/

#define DECLARE_IF_KUNIT(body...) body

+ /**

+ * VALUE_IF_KUNIT - Conditionally evaluate an expression
+ * @expr: the expression to be evaluated conditionally
+ *
+ * This macro evaluates expression statement only if CONFIG_KUNIT is enabled.
+ * Otherwise if CONFIG_KUNIT is not enabled it will evaluate always to 0.

+ *
+ * .. code-block:: C
+ *

+ * int real_func(int i)
+ * {
+ * if (VALUE_IF_KUNIT(i == 0xC0FFE))
+ * return 0;
+ *
+ * return i + 1;
+ * }
+ */
+ #define VALUE_IF_KUNIT(expr...) expr

+
/**
* VISIBLE_IF_KUNIT - A macro that sets symbols to be static if
* CONFIG_KUNIT is not enabled. Otherwise if CONFIG_KUNIT is enabled

@@ -46,6 +65,7 @@
EXPORTED_FOR_KUNIT_TESTING)
#else
#define DECLARE_IF_KUNIT(body...)
+ #define VALUE_IF_KUNIT(expr...) 0

[Michal Wajdeczko]

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 functionality 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.

These new stubs, named 'global', must be declared using dedicated
KUNIT_DECLARE_GLOBAL_STUB() macro and then can be placed either as
global static variables or as part of the other structures.

To properly maintain stubs lifecycle, they can be activated only
from the main KUnit context. Some precaution is taken to avoid
changing or deactivating replacement functions if one is still
used by other thread.

Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>
---

Cc: Rae Moar <rm...@google.com>
Cc: David Gow <davi...@google.com>

Cc: Daniel Latypov <dlat...@google.com>

Cc: Lucas De Marchi <lucas.d...@intel.com>

---
v2: s/FIXED_STUB/GLOBAL_STUB (David, Lucas)

make it little more thread safe (Rae, David)
wait until stub call finishes before test end (David)
wait until stub call finishes before changing stub (David)
allow stub deactivation (Rae)
prefer kunit log (David)

v3: fix s/fixed/global in comments (Lucas)
improve sanitize (Lucas, Michal)
---
include/kunit/static_stub.h | 158 ++++++++++++++++++++++++++++++++++++
lib/kunit/static_stub.c | 50 ++++++++++++
2 files changed, 208 insertions(+)

diff --git a/include/kunit/static_stub.h b/include/kunit/static_stub.h
index bf940322dfc0..350731eda523 100644

--- a/include/kunit/static_stub.h
+++ b/include/kunit/static_stub.h
@@ -12,12 +12,15 @@

/* If CONFIG_KUNIT is not enabled, these stubs quietly disappear. */
#define KUNIT_STATIC_STUB_REDIRECT(real_fn_name, args...) do {} while (0)
+#define KUNIT_GLOBAL_STUB_REDIRECT(stub_name, args...) do {} while (0)
+#define KUNIT_DECLARE_GLOBAL_STUB(stub_name, stub_type)

#else

#include <kunit/test.h>
#include <kunit/test-bug.h>

+#include <linux/cleanup.h> /* for CLASS */
#include <linux/compiler.h> /* for {un,}likely() */
#include <linux/sched.h> /* for task_struct */

@@ -109,5 +112,160 @@ void __kunit_activate_static_stub(struct kunit *test,
*/
void kunit_deactivate_static_stub(struct kunit *test, void *real_fn_addr);

+/**
+ * struct kunit_global_stub - Represents a context of global function stub.
+ * @replacement: The address of replacement function.
+ * @owner: The KUnit test that owns the stub, valid only when @busy > 0.
+ * @busy: The stub busyness counter incremented on entry to the replacement
+ * function, decremented on exit, used to signal if the stub is idle.
+ * @idle: The completion state to indicate when the stub is idle again.

+ *

+ * This structure is for KUnit internal use only.
+ * See KUNIT_DECLARE_GLOBAL_STUB().
+ */
+struct kunit_global_stub {
+ void *replacement;
+ struct kunit *owner;
+ atomic_t busy;
+ struct completion idle;
+};
+
+/**
+ * KUNIT_DECLARE_GLOBAL_STUB() - Declare a global function stub.
+ * @stub_name: The name of the stub, must be a valid identifier
+ * @stub_type: The type of the function that this stub will replace

+ *

+
+ if (active && !atomic_dec_return(&stub->busy))
+ complete_all(&stub->idle);
+ }),
+ ({
+ class_kunit_global_stub_guard_t guard;
+ bool active = !!atomic_inc_not_zero(&stub->busy);
+
+ guard.stub = stub;
+ guard.active_replacement = active ? READ_ONCE(stub->replacement) : NULL;
+
+ guard;
+ }),
+ struct kunit_global_stub *stub)
+
+/**

+ * KUNIT_GLOBAL_STUB_REDIRECT() - Call a global function stub if activated.

+ * @stub: The function stub declared using KUNIT_DECLARE_GLOBAL_STUB()
+ * @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 KUnit is not
+ * running or stub is not yet activated the function will continue execution
+ * as normal.
+ *
+ * The function stub must be declared with KUNIT_DECLARE_GLOBAL_STUB() that is
+ * stored in a place that is accessible from both the test code, which will
+ * activate this stub using kunit_activate_global_stub(), and from the function,
+ * where we will do this redirection using KUNIT_GLOBAL_STUB_REDIRECT().
+ *
+ * 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
+ *
+ * KUNIT_DECLARE_GLOBAL_STUB(func_stub, int (*)(int n));

+ *
+ * int real_func(int n)

+ * {
+ * KUNIT_GLOBAL_STUB_REDIRECT(func_stub, n);

+ * return n + 1;
+ * }

+ *
+ * int replacement_func(int n)

+ * {
+ * return n + 100;

+ * kunit_activate_global_stub() - Setup a global function stub.
+ * @test: Test case that wants to activate a global 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 deactivate the stub at the test end.
+ *
+ * The redirection can be disabled with kunit_deactivate_global_stub().
+ * The stub must be declared using KUNIT_DECLARE_GLOBAL_STUB().
+ */
+#define kunit_activate_global_stub(test, stub, replacement) do { \
+ typeof(stub) *__stub = &(stub); \
+ typecheck_fn(typeof(__stub->dummy), (replacement)); \
+ __kunit_activate_global_stub((test), &__stub->base, (replacement)); \
+} while (0)
+
+void __kunit_deactivate_global_stub(struct kunit *test, struct kunit_global_stub *stub);
+
+/**

+ * kunit_deactivate_global_stub() - Disable a global function stub.
+ * @test: Test case that wants to deactivate a global function stub

+ * @stub: The location of the function stub pointer
+ *
+ * The stub must be declared using KUNIT_DECLARE_GLOBAL_STUB().
+ */
+#define kunit_deactivate_global_stub(test, stub) do { \
+ typeof(stub) *__stub = &(stub); \
+ __kunit_deactivate_global_stub((test), &__stub->base); \
+} while (0)
+
#endif
#endif
diff --git a/lib/kunit/static_stub.c b/lib/kunit/static_stub.c

index 92b2cccd5e76..c7000c8dbabe 100644
--- a/lib/kunit/static_stub.c
+++ b/lib/kunit/static_stub.c
@@ -121,3 +121,53 @@ void __kunit_activate_static_stub(struct kunit *test,

}
}
EXPORT_SYMBOL_GPL(__kunit_activate_static_stub);
+
+static void sanitize_global_stub(void *data)
+{
+ struct kunit *test = kunit_get_current_test();
+ struct kunit_global_stub *stub = data;
+
+ KUNIT_EXPECT_NE(test, 0, atomic_read(&stub->busy));

+ KUNIT_EXPECT_PTR_EQ(test, test, READ_ONCE(stub->owner));
+
+ reinit_completion(&stub->idle);
+ if (!atomic_dec_and_test(&stub->busy)) {
+ kunit_info(test, "waiting for %ps\n", stub->replacement);
+ KUNIT_EXPECT_EQ(test, 0, wait_for_completion_interruptible(&stub->idle));

+ }
+
+ WRITE_ONCE(stub->owner, NULL);
+ WRITE_ONCE(stub->replacement, NULL);
+}
+
+/*
+ * Helper function for kunit_activate_global_stub(). The macro does
+ * typechecking, so use it instead.
+ */
+void __kunit_activate_global_stub(struct kunit *test,
+ struct kunit_global_stub *stub,
+ void *replacement_addr)
+{
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stub);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, replacement_addr);
+

+ kunit_release_action(test, sanitize_global_stub, stub);
+ KUNIT_EXPECT_EQ(test, 0, atomic_read(&stub->busy));
+

+ init_completion(&stub->idle);
+ WRITE_ONCE(stub->owner, test);
+ WRITE_ONCE(stub->replacement, replacement_addr);
+
+ KUNIT_ASSERT_EQ(test, 1, atomic_inc_return(&stub->busy));
+ KUNIT_ASSERT_EQ(test, 0, kunit_add_action_or_reset(test, sanitize_global_stub, stub));
+}
+EXPORT_SYMBOL_GPL(__kunit_activate_global_stub);
+
+/*
+ * Helper function for kunit_deactivate_global_stub(). Use it instead.
+ */
+void __kunit_deactivate_global_stub(struct kunit *test, struct kunit_global_stub *stub)
+{

+ kunit_release_action(test, sanitize_global_stub, stub);
+}
+EXPORT_SYMBOL_GPL(__kunit_deactivate_global_stub);
--
2.43.0

[Michal Wajdeczko]

Add example how to use KUNIT_GLBAL_STUB_REDIRECT and compare its
usage with the KUNIT_STATIC_STUB_REDIRECT.

Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>

Reviewed-by: Rae Moar <rm...@google.com> #v1
Reviewed-by: David Gow <davi...@google.com> #v1

Reviewed-by: Lucas De Marchi <lucas.d...@intel.com>
---
Cc: Daniel Latypov <dlat...@google.com>

---
v2: add missing testcase description (Rae) and rebase

v3: fix s/fixed/global (Lucas)
drop stale comment in commit message (Michal)

---
lib/kunit/kunit-example-test.c | 67 ++++++++++++++++++++++++++++++++++
1 file changed, 67 insertions(+)

diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c

index 3056d6bc705d..0a64f9c17b24 100644

--- a/lib/kunit/kunit-example-test.c
+++ b/lib/kunit/kunit-example-test.c
@@ -6,8 +6,10 @@
* Author: Brendan Higgins <brendan...@google.com>
*/

+#include <linux/workqueue.h>
#include <kunit/test.h>
#include <kunit/static_stub.h>
+#include <kunit/visibility.h>

/*
* This is the most fundamental element of KUnit, the test case. A test case
@@ -221,6 +223,70 @@ static void example_static_stub_using_fn_ptr_test(struct kunit *test)
KUNIT_EXPECT_EQ(test, add_one(1), 2);
}

+/* This could be a location of various global stub functions. */
+static struct {
+ KUNIT_DECLARE_GLOBAL_STUB(add_two, int (*)(int i));
+} stubs;
+
+/* This is a function we'll replace with stubs. */
+static int add_two(int i)
+{
+ /* This will trigger the stub if active. */
+ KUNIT_STATIC_STUB_REDIRECT(add_two, i);
+ KUNIT_GLOBAL_STUB_REDIRECT(stubs.add_two, i);

+}
+
+/*

+ * This test shows how to use KUNIT_GLOBAL_STUB_REDIRECT and compares its
+ * usage with the KUNIT_STATIC_STUB_REDIRECT.
+ */
+static void example_global_stub_test(struct kunit *test)
+{
+ /* static stub redirection works only for KUnit thread */
+ kunit_activate_static_stub(test, add_two, subtract_one);
+ KUNIT_EXPECT_EQ(test, add_two(1), subtract_one(1));
+ KUNIT_EXPECT_NE_MSG(test, add_two_async(1), subtract_one(1),
+ "stub shouldn't be active outside KUnit thread!");
+
+ kunit_deactivate_static_stub(test, add_two);
+ KUNIT_EXPECT_EQ(test, add_two(1), add_two(1));
+

+ /* global stub redirection works for KUnit and other threads */

[Michal Wajdeczko]

While we already have few ASSERT() within the implementation,
it's always better to have dedicated test cases. Add tests for:
- automatic deactivation of the stubs at the test end
- blocked deactivation until all active stub calls finish
- blocked stub change until all active stub calls finish
- safe abuse (deactivation without activation)

Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>
---

Cc: Rae Moar <rm...@google.com>
Cc: David Gow <davi...@google.com>
Cc: Lucas De Marchi <lucas.d...@intel.com>

---
lib/kunit/kunit-test.c | 254 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 253 insertions(+), 1 deletion(-)

diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
index 37e02be1e710..eb1bb312ad71 100644
--- a/lib/kunit/kunit-test.c
+++ b/lib/kunit/kunit-test.c

@@ -6,8 +6,10 @@
* Author: Brendan Higgins <brendan...@google.com>
*/

#include "linux/gfp_types.h"
+#include <kunit/static_stub.h>
#include <kunit/test.h>
#include <kunit/test-bug.h>

+#include <kunit/visibility.h>

#include <linux/device.h>
#include <kunit/device.h>
@@ -866,10 +868,260 @@ static struct kunit_suite kunit_current_test_suite = {
.test_cases = kunit_current_test_cases,
};

+static struct {
+ /* this stub matches the real function */
+ KUNIT_DECLARE_GLOBAL_STUB(first_stub, int (*)(int i));
+ /* this stub matches only return type of the real function */
+ KUNIT_DECLARE_GLOBAL_STUB(second_stub, int (*)(int bit, int data));
+ /* this is an example stub that returns void */
+ KUNIT_DECLARE_GLOBAL_STUB(void_stub, void (*)(void));
+ /* this is an example how to store additional data for use by stubs */
+ DECLARE_IF_KUNIT(int data);
+ DECLARE_IF_KUNIT(int counter);
+} stubs = {
+ DECLARE_IF_KUNIT(.data = 3),
+};
+
+static int real_func(int i)
+{
+ KUNIT_GLOBAL_STUB_REDIRECT(stubs.first_stub, i);
+ KUNIT_GLOBAL_STUB_REDIRECT(stubs.second_stub, BIT(i), stubs.data);

+
+ return i;
+}

+
+struct real_work {

+ struct work_struct work;
+ int param;
+ int result;
+};
+

+static void real_work_func(struct work_struct *work)
+{
+ struct real_work *w = container_of(work, typeof(*w), work);
+
+ w->result = real_func(w->param);
+}
+

+static int real_func_async(int i)
+{
+ struct real_work w = { .param = i, .result = -EINPROGRESS };
+
+ INIT_WORK_ONSTACK(&w.work, real_work_func);

+ schedule_work(&w.work);
+ flush_work(&w.work);
+ destroy_work_on_stack(&w.work);
+
+ return w.result;
+}
+

+static int replacement_func(int i)
+{

+ return i + 1;
+}
+

+static int other_replacement_func(int i)
+{

+static void expect_deactivated(void *data)

+{
+ struct kunit *test = kunit_get_current_test();
+

+ KUNIT_EXPECT_NULL(test, stubs.first_stub.base.owner);
+ KUNIT_EXPECT_NULL(test, stubs.first_stub.base.replacement);
+ KUNIT_EXPECT_NULL(test, stubs.second_stub.base.owner);
+ KUNIT_EXPECT_NULL(test, stubs.second_stub.base.replacement);
+ KUNIT_EXPECT_NULL(test, stubs.void_stub.base.owner);
+ KUNIT_EXPECT_NULL(test, stubs.void_stub.base.replacement);
+}
+

+static void kunit_global_stub_test_deactivate(struct kunit *test)
+{

[Rae Moar]

On Thu, Aug 29, 2024 at 2:14 PM Michal Wajdeczko
<michal.w...@intel.com> wrote:
>
> While we already have few ASSERT() within the implementation,
> it's always better to have dedicated test cases. Add tests for:
> - automatic deactivation of the stubs at the test end
> - blocked deactivation until all active stub calls finish
> - blocked stub change until all active stub calls finish
> - safe abuse (deactivation without activation)
>
> Signed-off-by: Michal Wajdeczko <michal.w...@intel.com>

Hello!

Thanks for this new patch series version! This test suite is looking
good! Just a few comments.

One small thing: I find "selftests" in the title of the patch a bit
confusing due to the other main Kernel tests being kselftests. I think
I would replace "selftests" with just "tests".

I understand that this is demonstrating the use of the
DECLARE_IF_KUNIT macro but I think I find the uses in this struct to
reduce the readability too much.

I really appreciate the amount of detail and work that went into these
two long tests. But they don't read as unit tests to me. Is there any
way we can streamline these? There are also a lot of helper functions
which really inflates the kunit-test.c file. Is it necessary to test
for activating a replacement with different arguments or the void_stub
and use of the counter? Let me know what you think about this. I am
happy to hear more specifically if you view these instances as
necessary.