[RFC PATCH] kunit: Add "hooks" to call into KUnit when it's built as a module

[David Gow]

KUnit has several macros and functions intended for use from non-test
code. These hooks, currently the kunit_get_current_test() and
kunit_fail_current_test() macros, didn't work when CONFIG_KUNIT=m.

In order to support this case, the required functions and static data
need to be available unconditionally, even when KUnit itself is not
built-in. The new 'hooks.c' file is therefore always included, and has
both the static key required for kunit_get_current_test(), and a
function pointer to the real implementation of
__kunit_fail_current_test(), which is populated when the KUnit module is
loaded.

This can then be extended for future features which require similar
"hook" behaviour, such as static stubs:
https://lore.kernel.org/all/20221208061841.2...@google.com/

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

This is basically a prerequisite for the stub features working when
KUnit is built as a module, and should nicely make a few other tests
work then, too.

I'm not 100% sold on the whole "fill in a table of function pointers
when kunit.ko is loaded" trick: it is basically just working around the
sensible limitations on depending on modules. I think it should be safe
here, as the functions/macros all have fallback behaviour when no test
is running, and this is just another case of that.

Similarly, I'm sure there must be a better way to compile hooks.o in
when KUNIT=y or KUNIT=m, but the trick of adding it separately as an
obj-y in the lib/ Makefile, then having an #if IS_ENABLED() check in the
file is the only one I've been able to come up with using my meagre
knowledge of Kbuild. Better suggestions welcome!
---
Documentation/dev-tools/kunit/usage.rst | 14 ++++++--------
include/kunit/test-bug.h | 15 ++++++++-------
lib/Makefile | 4 ++++
lib/kunit/Makefile | 3 +++
lib/kunit/hooks.c | 23 +++++++++++++++++++++++
lib/kunit/test.c | 10 ++++------
6 files changed, 48 insertions(+), 21 deletions(-)
create mode 100644 lib/kunit/hooks.c

diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
index 48f8196d5aad..6424493b93cb 100644
--- a/Documentation/dev-tools/kunit/usage.rst
+++ b/Documentation/dev-tools/kunit/usage.rst
@@ -648,10 +648,9 @@ We can do this via the ``kunit_test`` field in ``task_struct``, which we can
access using the ``kunit_get_current_test()`` function in ``kunit/test-bug.h``.

``kunit_get_current_test()`` is safe to call even if KUnit is not enabled. If
-KUnit is not enabled, was built as a module (``CONFIG_KUNIT=m``), or no test is
-running in the current task, it will return ``NULL``. This compiles down to
-either a no-op or a static key check, so will have a negligible performance
-impact when no test is running.
+KUnit is not enabled, or if no test is running in the current task, it will
+return ``NULL``. This compiles down to either a no-op or a static key check,
+so will have a negligible performance impact when no test is running.

The example below uses this to implement a "mock" implementation of a function, ``foo``:

@@ -726,8 +725,7 @@ structures as shown below:
#endif

``kunit_fail_current_test()`` is safe to call even if KUnit is not enabled. If
-KUnit is not enabled, was built as a module (``CONFIG_KUNIT=m``), or no test is
-running in the current task, it will do nothing. This compiles down to either a
-no-op or a static key check, so will have a negligible performance impact when
-no test is running.
+KUnit is not enabled, or if no test is running in the current task, it will do
+nothing. This compiles down to either a no-op or a static key check, so will
+have a negligible performance impact when no test is running.

diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h
index c1b2e14eab64..122f50198903 100644
--- a/include/kunit/test-bug.h
+++ b/include/kunit/test-bug.h
@@ -1,6 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0 */
/*
- * KUnit API allowing dynamic analysis tools to interact with KUnit tests
+ * KUnit API providing hooks for non-test code to interact with tests.
*
* Copyright (C) 2020, Google LLC.
* Author: Uriel Guajardo <urielg...@google.com>
@@ -9,7 +9,7 @@
#ifndef _KUNIT_TEST_BUG_H
#define _KUNIT_TEST_BUG_H

-#if IS_BUILTIN(CONFIG_KUNIT)
+#if IS_ENABLED(CONFIG_KUNIT)

#include <linux/jump_label.h> /* For static branch */
#include <linux/sched.h>
@@ -43,20 +43,21 @@ static inline struct kunit *kunit_get_current_test(void)
* kunit_fail_current_test() - If a KUnit test is running, fail it.
*
* If a KUnit test is running in the current task, mark that test as failed.
- *
- * This macro will only work if KUnit is built-in (though the tests
- * themselves can be modules). Otherwise, it compiles down to nothing.
*/
#define kunit_fail_current_test(fmt, ...) do { \
if (static_branch_unlikely(&kunit_running)) { \
+ /* Guaranteed to be non-NULL when kunit_running true*/ \
__kunit_fail_current_test(__FILE__, __LINE__, \
fmt, ##__VA_ARGS__); \
} \
} while (0)


-extern __printf(3, 4) void __kunit_fail_current_test(const char *file, int line,
- const char *fmt, ...);
+/* Function pointer defined as a hook in hooks.c, and implemented in test.c */
+typedef __printf(3, 4) void kunit_hook_fn_fail_current_test(const char *file,
+ int line,
+ const char *fmt, ...);
+extern kunit_hook_fn_fail_current_test *__kunit_fail_current_test;

#else

diff --git a/lib/Makefile b/lib/Makefile
index 4d9461bfea42..9031de6ca73c 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -126,6 +126,10 @@ CFLAGS_test_fpu.o += $(FPU_CFLAGS)
obj-$(CONFIG_TEST_LIVEPATCH) += livepatch/

obj-$(CONFIG_KUNIT) += kunit/
+# Include the KUnit hooks unconditionally. They'll compile to nothing if
+# CONFIG_KUNIT=n, otherwise will be a small table of static data (static key,
+# function pointers) which need to be built-in even when KUnit is a module.
+obj-y += kunit/hooks.o

ifeq ($(CONFIG_DEBUG_KOBJECT),y)
CFLAGS_kobject.o += -DDEBUG
diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
index 29aff6562b42..deeb46cc879b 100644
--- a/lib/kunit/Makefile
+++ b/lib/kunit/Makefile
@@ -11,6 +11,9 @@ ifeq ($(CONFIG_KUNIT_DEBUGFS),y)
kunit-objs += debugfs.o
endif

+# KUnit 'hooks' are built-in even when KUnit is built as a module.
+lib-y += hooks.o
+
obj-$(CONFIG_KUNIT_TEST) += kunit-test.o

# string-stream-test compiles built-in only.
diff --git a/lib/kunit/hooks.c b/lib/kunit/hooks.c
new file mode 100644
index 000000000000..48189567a774
--- /dev/null
+++ b/lib/kunit/hooks.c
@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit 'Hooks' implementation.
+ *
+ * This file contains code / structures which should be built-in even when
+ * KUnit itself is built as a module.
+ *
+ * Copyright (C) 2022, Google LLC.
+ * Author: David Gow <davi...@google.com>
+ */
+
+/* This file is always built-in, so make sure it's empty if CONFIG_KUNIT=n */
+#if IS_ENABLED(CONFIG_KUNIT)
+
+#include <kunit/test-bug.h>
+
+DEFINE_STATIC_KEY_FALSE(kunit_running);
+EXPORT_SYMBOL(kunit_running);
+
+/* Function pointers for hooks. */
+kunit_hook_fn_fail_current_test *__kunit_fail_current_test;
+EXPORT_SYMBOL_GPL(__kunit_fail_current_test);
+#endif
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index c9ebf975e56b..711fdcce6de8 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -20,13 +20,10 @@
#include "string-stream.h"
#include "try-catch-impl.h"

-DEFINE_STATIC_KEY_FALSE(kunit_running);
-
-#if IS_BUILTIN(CONFIG_KUNIT)
/*
* Fail the current test and print an error message to the log.
*/
-void __kunit_fail_current_test(const char *file, int line, const char *fmt, ...)
+void __kunit_fail_current_test_impl(const char *file, int line, const char *fmt, ...)
{
va_list args;
int len;
@@ -53,8 +50,6 @@ void __kunit_fail_current_test(const char *file, int line, const char *fmt, ...)
kunit_err(current->kunit_test, "%s:%d: %s", file, line, buffer);
kunit_kfree(current->kunit_test, buffer);
}
-EXPORT_SYMBOL_GPL(__kunit_fail_current_test);
-#endif

/*
* Enable KUnit tests to run.
@@ -777,6 +772,9 @@ EXPORT_SYMBOL_GPL(kunit_cleanup);

static int __init kunit_init(void)
{
+ /* Install the KUnit hook functions. */
+ __kunit_fail_current_test = __kunit_fail_current_test_impl;
+
kunit_debugfs_init();
#ifdef CONFIG_MODULES
return register_module_notifier(&kunit_mod_nb);
--
2.39.0.314.g84b9a713c41-goog

[Rae Moar]

On Tue, Jan 17, 2023 at 9:27 AM David Gow <davi...@google.com> wrote:
>
> KUnit has several macros and functions intended for use from non-test
> code. These hooks, currently the kunit_get_current_test() and
> kunit_fail_current_test() macros, didn't work when CONFIG_KUNIT=m.
>
> In order to support this case, the required functions and static data
> need to be available unconditionally, even when KUnit itself is not
> built-in. The new 'hooks.c' file is therefore always included, and has
> both the static key required for kunit_get_current_test(), and a
> function pointer to the real implementation of
> __kunit_fail_current_test(), which is populated when the KUnit module is
> loaded.
>
> This can then be extended for future features which require similar
> "hook" behaviour, such as static stubs:
> https://lore.kernel.org/all/20221208061841.2...@google.com/
>
> Signed-off-by: David Gow <davi...@google.com>

Hi David!

After testing and reviewing, this all looks good. Thanks for doing
this. This will certainly be useful when dealing with static stubbing
and fixing up some issues with building KUnit as a module. Just a few
points below.

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

Nit: This is unrelated to your changes, but I personally find the
wording of “so will have a negligible performance impact when no test
is running” to be odd. This issue should likely be addressed in a
different patch but could we change this to be “so there will be
negligible performance impact when no test is running”, “which will
have a negligible performance impact when no test is running", or
something similar.

At first I was unsure if using typedef is necessary here. However,
after more consideration and inspection. I believe this is likely the
cleanest solution to creating a type for the pointer.

> #else
>
> diff --git a/lib/Makefile b/lib/Makefile
> index 4d9461bfea42..9031de6ca73c 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -126,6 +126,10 @@ CFLAGS_test_fpu.o += $(FPU_CFLAGS)
> obj-$(CONFIG_TEST_LIVEPATCH) += livepatch/
>
> obj-$(CONFIG_KUNIT) += kunit/
> +# Include the KUnit hooks unconditionally. They'll compile to nothing if
> +# CONFIG_KUNIT=n, otherwise will be a small table of static data (static key,
> +# function pointers) which need to be built-in even when KUnit is a module.
> +obj-y += kunit/hooks.o
>

I have to admit I also have limited Kbuild knowledge. However, I found
this to work as an alternative to the above addition:

ifeq ($(CONFIG_KUNIT), m)
obj-y += kunit/hooks.o
else
obj-$(CONFIG_KUNIT) += kunit/hooks.o
endif

To my understanding, this would allow the kunit/hooks file to not be
built-in in the case of CONFIG_KUNIT=n, while ensuring it is built-in
in the case of CONFIG_KUNIT=y or m.

With that said, I would still approve this patch if this wasn’t
changed. There are plenty of other files included in the Makefile
unconditionally. Let me know what you think of the suggestion above.

As an aside, I wonder if we could add test coverage for the use of
current->kunit_test.

Now that it works when KUnit is built as a module thanks to this
patch, we could add some tests for the uses of current->kunit_test as
well as kunit_get_current_test() and maybe kunit_fail_current_test()
(although there may be limitations for the latter).

These changes should likely be added in a separate patch.

Thanks again David!

Rae

[David Gow]

KUnit has several macros and functions intended for use from non-test
code. These hooks, currently the kunit_get_current_test() and
kunit_fail_current_test() macros, didn't work when CONFIG_KUNIT=m.

In order to support this case, the required functions and static data
need to be available unconditionally, even when KUnit itself is not
built-in. The new 'hooks.c' file is therefore always included, and has
both the static key required for kunit_get_current_test(), and a
function pointer to the real implementation of
__kunit_fail_current_test(), which is populated when the KUnit module is
loaded.

A new header, kunit/hooks-table.h, contains a table of all hooks, and is
repeatedly included with different definitions of the KUNIT_HOOK() in
order to automatically generate the needed function pointer tables. When
KUnit is disabled, or the module is not loaded, these function pointers
are all NULL. This shouldn't be a problem, as they're all used behind
wrappers which check kunit_running and/or that the pointer is non-NULL.

This can then be extended for future features which require similar
"hook" behaviour, such as static stubs:
https://lore.kernel.org/all/20221208061841.2...@google.com/

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

---

This is basically a prerequisite for the stub features working when
KUnit is built as a module, and should nicely make a few other tests
work then, too.

v2 adds a slightly-excessive macro-based system for defining hooks. This
made adding the static stub hooks absolutely trivial, and the complexity
is totally hidden from the user (being an internal KUnit implementation
detail), so I'm more comfortable with this than some other macro magic.

It does however result in a huge number of checkpatch.pl errors, as
we're using macros in unconventional ways, and checkpatch just can't
work out the syntax. These are mostly "Macros with complex values should
be enclosed in parentheses", "Macros with multiple statements should be
enclosed in a do - while loop", and similar, which don't apply due to
the macros not being expressions: they are mostly declarations or
assignment statements. There are a few others where checkpatch thinks
that the return value is the function name and similar, so complains
about the style.

Open questions:
- Is this macro-based system worth it, or was v1 better?
- Should we rename test-bug.h to hooks.h or similar.
(I think so, but would rather do it in a separate patch, to make it
easier to review. There are a few includes of it scattered about.)
- Is making these NULL when KUnit isn't around sensible, or should we
auto-generate a "default" implementation. This is a pretty easy
extension to the macros here.
(I think NULL is good for now, as we have wrappers for these anyway.
If we want to change our minds later as we add more hooks, it's easy.)
- Any other thoughts?

Cheers,
-- David

Changes since RFC v1:
https://lore.kernel.org/all/20230117142737.2...@google.com/
- Major refit to auto-generate the hook code using macros.
- (Note that previous Reviewed-by tags have not been added, as this is a
big enough change it probably needs a re-reviews. Thanks Rae for
reviewing RFC v1 previously, though!)
---
Documentation/dev-tools/kunit/usage.rst | 14 +++++-----
include/kunit/hooks-table.h | 34 +++++++++++++++++++++++++
include/kunit/test-bug.h | 24 +++++++++--------
lib/Makefile | 4 +++
lib/kunit/Makefile | 3 +++
lib/kunit/hooks.c | 27 ++++++++++++++++++++
lib/kunit/test.c | 22 +++++++++++-----
7 files changed, 103 insertions(+), 25 deletions(-)
create mode 100644 include/kunit/hooks-table.h

diff --git a/include/kunit/hooks-table.h b/include/kunit/hooks-table.h
new file mode 100644
index 000000000000..0b5eafd199ed
--- /dev/null
+++ b/include/kunit/hooks-table.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * KUnit 'Hooks' function pointer table
+ *
+ * This file is included multiple times, each time with a different definition
+ * of KUNIT_HOOK. This provides one place where all of the hooks can be listed
+ * which can then be converted into function / implementation declarations, or
+ * code to set function pointers.
+ *
+ * Copyright (C) 2023, Google LLC.

+ * Author: David Gow <davi...@google.com>
+ */
+
+/*

+ * To declare a hook, use:
+ * KUNIT_HOOK(name, retval, args), where:
+ * - name: the function name of the exported hook
+ * - retval: the type of the return value of the hook
+ * - args: the arguments to the hook, of the form (int a, int b)
+ *
+ * Note that the argument list should be contained within the brackets (),
+ * and that the implementation of the hook should be in a <name>_impl
+ * function, which should not be declared static, but need not be exported.
+ */
+
+#ifndef KUNIT_HOOK
+#error KUNIT_HOOK must be defined before including the hooks table
+#endif
+
+KUNIT_HOOK(__kunit_fail_current_test, __printf(3, 4) void,
+ (const char *file, int line, const char *fmt, ...));
+
+/* Undefine KUNIT_HOOK at the end, ready for the next use. */
+#undef KUNIT_HOOK
diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h
index c1b2e14eab64..3203ffc0a08b 100644

+/* Declare all of the available hooks. */
+#define KUNIT_HOOK(name, retval, args) \
+ extern retval (*name)args
+
+#include "kunit/hooks-table.h"

#else

@@ -66,10 +67,11 @@ static inline struct kunit *kunit_get_current_test(void) { return NULL; }
#define kunit_fail_current_test(fmt, ...) \
__kunit_fail_current_test(__FILE__, __LINE__, fmt, ##__VA_ARGS__)

-static inline __printf(3, 4) void __kunit_fail_current_test(const char *file, int line,

- const char *fmt, ...)

-{
-}
+/* No-op stubs if KUnit is not enabled. */
+#define KUNIT_HOOK(name, retval, args) \
+ static retval (*name)args = NULL
+
+#include "kunit/hooks-table.h"

#endif

diff --git a/lib/Makefile b/lib/Makefile
index 4d9461bfea42..9031de6ca73c 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -126,6 +126,10 @@ CFLAGS_test_fpu.o += $(FPU_CFLAGS)
obj-$(CONFIG_TEST_LIVEPATCH) += livepatch/

obj-$(CONFIG_KUNIT) += kunit/
+# Include the KUnit hooks unconditionally. They'll compile to nothing if
+# CONFIG_KUNIT=n, otherwise will be a small table of static data (static key,
+# function pointers) which need to be built-in even when KUnit is a module.
+obj-y += kunit/hooks.o

ifeq ($(CONFIG_DEBUG_KOBJECT),y)
CFLAGS_kobject.o += -DDEBUG
diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
index 29aff6562b42..deeb46cc879b 100644
--- a/lib/kunit/Makefile
+++ b/lib/kunit/Makefile
@@ -11,6 +11,9 @@ ifeq ($(CONFIG_KUNIT_DEBUGFS),y)
kunit-objs += debugfs.o
endif

+# KUnit 'hooks' are built-in even when KUnit is built as a module.
+lib-y += hooks.o
+
obj-$(CONFIG_KUNIT_TEST) += kunit-test.o

# string-stream-test compiles built-in only.
diff --git a/lib/kunit/hooks.c b/lib/kunit/hooks.c
new file mode 100644

index 000000000000..29e81614f486
--- /dev/null
+++ b/lib/kunit/hooks.c
@@ -0,0 +1,27 @@

+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit 'Hooks' implementation.
+ *
+ * This file contains code / structures which should be built-in even when
+ * KUnit itself is built as a module.
+ *
+ * Copyright (C) 2022, Google LLC.
+ * Author: David Gow <davi...@google.com>
+ */
+
+/* This file is always built-in, so make sure it's empty if CONFIG_KUNIT=n */
+#if IS_ENABLED(CONFIG_KUNIT)
+
+#include <kunit/test-bug.h>
+
+DEFINE_STATIC_KEY_FALSE(kunit_running);
+EXPORT_SYMBOL(kunit_running);
+
+/* Function pointers for hooks. */

+#define KUNIT_HOOK(name, retval, args) \
+ retval (*name)args; \
+ EXPORT_SYMBOL(name)
+
+#include "kunit/hooks-table.h"
+

+#endif
diff --git a/lib/kunit/test.c b/lib/kunit/test.c

index c9ebf975e56b..b6c88f722b68 100644

--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -20,13 +20,10 @@
#include "string-stream.h"
#include "try-catch-impl.h"

-DEFINE_STATIC_KEY_FALSE(kunit_running);
-
-#if IS_BUILTIN(CONFIG_KUNIT)
/*
* Fail the current test and print an error message to the log.
*/
-void __kunit_fail_current_test(const char *file, int line, const char *fmt, ...)
+void __kunit_fail_current_test_impl(const char *file, int line, const char *fmt, ...)
{
va_list args;
int len;
@@ -53,8 +50,6 @@ void __kunit_fail_current_test(const char *file, int line, const char *fmt, ...)
kunit_err(current->kunit_test, "%s:%d: %s", file, line, buffer);
kunit_kfree(current->kunit_test, buffer);
}
-EXPORT_SYMBOL_GPL(__kunit_fail_current_test);
-#endif

/*
* Enable KUnit tests to run.

@@ -775,8 +770,18 @@ void kunit_cleanup(struct kunit *test)
}
EXPORT_SYMBOL_GPL(kunit_cleanup);

+/* Declarations for the hook implemetnations */
+#define KUNIT_HOOK(name, retval, args) \
+ extern retval name##_impl args
+#include "kunit/hooks-table.h"
+

static int __init kunit_init(void)
{
+ /* Install the KUnit hook functions. */

+#define KUNIT_HOOK(name, retval, args) \
+ name = name##_impl
+#include "kunit/hooks-table.h"

+
kunit_debugfs_init();
#ifdef CONFIG_MODULES
return register_module_notifier(&kunit_mod_nb);

@@ -788,6 +793,11 @@ late_initcall(kunit_init);

static void __exit kunit_exit(void)
{
+ /* Remove the KUnit hook functions. */
+#define KUNIT_HOOK(name, retval, args) \
+ name = NULL
+#include "kunit/hooks-table.h"
+
#ifdef CONFIG_MODULES
unregister_module_notifier(&kunit_mod_nb);
#endif
--
2.39.0.246.g2a6d74b583-goog

[Brendan Higgins]

On Tue, Jan 24, 2023 at 3:04 AM 'David Gow' via KUnit Development
<kuni...@googlegroups.com> wrote:
>
> KUnit has several macros and functions intended for use from non-test
> code. These hooks, currently the kunit_get_current_test() and
> kunit_fail_current_test() macros, didn't work when CONFIG_KUNIT=m.
>
> In order to support this case, the required functions and static data
> need to be available unconditionally, even when KUnit itself is not
> built-in. The new 'hooks.c' file is therefore always included, and has
> both the static key required for kunit_get_current_test(), and a
> function pointer to the real implementation of
> __kunit_fail_current_test(), which is populated when the KUnit module is
> loaded.
>
> A new header, kunit/hooks-table.h, contains a table of all hooks, and is
> repeatedly included with different definitions of the KUNIT_HOOK() in
> order to automatically generate the needed function pointer tables. When
> KUnit is disabled, or the module is not loaded, these function pointers
> are all NULL. This shouldn't be a problem, as they're all used behind
> wrappers which check kunit_running and/or that the pointer is non-NULL.
>
> This can then be extended for future features which require similar
> "hook" behaviour, such as static stubs:
> https://lore.kernel.org/all/20221208061841.2...@google.com/

Devilishly clever. Maybe too clever, but I don't have any better ideas, so:

Reviewed-by: Brendan Higgins <brendan...@google.com>

Nevertheless, see my comments below:

> Signed-off-by: David Gow <davi...@google.com>
> ---
>
> This is basically a prerequisite for the stub features working when
> KUnit is built as a module, and should nicely make a few other tests
> work then, too.
>
> v2 adds a slightly-excessive macro-based system for defining hooks. This
> made adding the static stub hooks absolutely trivial, and the complexity
> is totally hidden from the user (being an internal KUnit implementation
> detail), so I'm more comfortable with this than some other macro magic.
>
> It does however result in a huge number of checkpatch.pl errors, as
> we're using macros in unconventional ways, and checkpatch just can't
> work out the syntax. These are mostly "Macros with complex values should
> be enclosed in parentheses", "Macros with multiple statements should be
> enclosed in a do - while loop", and similar, which don't apply due to
> the macros not being expressions: they are mostly declarations or
> assignment statements. There are a few others where checkpatch thinks
> that the return value is the function name and similar, so complains
> about the style.

Shuah, what are your thoughts here? I think it's OK, but I don't want
to go any further down this path unless you are OK with it too.

> Open questions:
> - Is this macro-based system worth it, or was v1 better?

I think this is definitely better if we had more than one function to
hook. With just one function - I am less confident, but I like having
a set way to do it.

> - Should we rename test-bug.h to hooks.h or similar.
> (I think so, but would rather do it in a separate patch, to make it
> easier to review. There are a few includes of it scattered about.)

Agreed, that confused me at first.

> - Is making these NULL when KUnit isn't around sensible, or should we
> auto-generate a "default" implementation. This is a pretty easy
> extension to the macros here.
> (I think NULL is good for now, as we have wrappers for these anyway.
> If we want to change our minds later as we add more hooks, it's easy.)

Yeah, I'm fine with either.

> - Any other thoughts?

My primary concern was with the naming of test-bug.h, but you said
you'd handle that in another patch, which makes sense to me.

I also want to make sure Shuah is OK with the checkpatch warnings.

I did find two nits below:

nit: I think it would be good to add a comment here about this being a
hooked function or something.

> +void __kunit_fail_current_test_impl(const char *file, int line, const char *fmt, ...)
> {
> va_list args;
> int len;
> @@ -53,8 +50,6 @@ void __kunit_fail_current_test(const char *file, int line, const char *fmt, ...)
> kunit_err(current->kunit_test, "%s:%d: %s", file, line, buffer);
> kunit_kfree(current->kunit_test, buffer);
> }
> -EXPORT_SYMBOL_GPL(__kunit_fail_current_test);
> -#endif
>
> /*
> * Enable KUnit tests to run.
> @@ -775,8 +770,18 @@ void kunit_cleanup(struct kunit *test)
> }
> EXPORT_SYMBOL_GPL(kunit_cleanup);
>
> +/* Declarations for the hook implemetnations */

nit: spelling

[David Gow]

Thanks for checking this out! I definitely agree that this is verging
on over-complicated, but I do think it'll be worth it.

I'll send out a proper v1 in a day or two with your comments below
addressed (and a few other minor issues).
One option is to just include this in the same series as the static
stub feature (the next version of which will use it), but that may
just make things more confusing.

Yeah, I agree that it's not worth it for just one function (hence the
first RFC just hardcoding these), but I think it pays of surprisingly
quickly as we add more.

In particular, the static stub code lives in a different file, so
would've needed an extra header for the "_impl" version anyway, as
it'd need to be accessible from the test.c init code which sets up the
function pointers. Once we're adding several headers, each with very
closely related definitions, this way already looks like a bit of a
win.

(And I have plans to add some more hooks going forward, particularly
for "data stubbing", as well as to expose some way of looking up
resources.)

>
> > - Should we rename test-bug.h to hooks.h or similar.
> > (I think so, but would rather do it in a separate patch, to make it
> > easier to review. There are a few includes of it scattered about.)
>
> Agreed, that confused me at first.

The other option we could do is to add "hooks.h" now, and temporarily
make "test-bug.h" just transitively include that until we clean up any
existing users.

>
> > - Is making these NULL when KUnit isn't around sensible, or should we
> > auto-generate a "default" implementation. This is a pretty easy
> > extension to the macros here.
> > (I think NULL is good for now, as we have wrappers for these anyway.
> > If we want to change our minds later as we add more hooks, it's easy.)
>
> Yeah, I'm fine with either.
>

I think we'll leave it as-is until we have enough hooks that it
justifies even more complexity.

> > - Any other thoughts?
>
> My primary concern was with the naming of test-bug.h, but you said
> you'd handle that in another patch, which makes sense to me.
>
> I also want to make sure Shuah is OK with the checkpatch warnings.
>

Yeah, the checkpatch warnings are the biggest worry I have. There are
good reasons to consider them false positives, but it still leaves a
bit of a bad taste in my mouth.

Here's the full list, with some notes:
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#116:
new file mode 100644

(The new files are in KUnit directories, so are already covered by MAINTAINERS)

ERROR: Macros with complex values should be enclosed in parentheses
#196: FILE: include/kunit/test-bug.h:57:

+#define KUNIT_HOOK(name, retval, args) \
+ extern retval (*name)args

(This is a global variable declaration, so can't be enclosed with
parentheses or do {} while (0))

WARNING: space prohibited between function name and open parenthesis '('
#197: FILE: include/kunit/test-bug.h:58:
+ extern retval (*name)args

(retval is not the function name, it's the return value. name is the
name, the parentheses are for the function pointer syntax.)

ERROR: Macros with complex values should be enclosed in parentheses
#212: FILE: include/kunit/test-bug.h:71:

+#define KUNIT_HOOK(name, retval, args) \
+ static retval (*name)args = NULL

(This is a global variable declaration, so can't be enclosed with
parentheses or do {} while (0))

WARNING: space prohibited between function name and open parenthesis '('
#213: FILE: include/kunit/test-bug.h:72:

+ static retval (*name)args = NULL

(retval is not the function name, it's the return value. name is the
name, the parentheses are for the function pointer syntax.)

WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
#271: FILE: lib/kunit/hooks.c:18:
+EXPORT_SYMBOL(kunit_running);

(This does immediately follow the DEFINE_STATIC_KEY macro)

ERROR: Macros with multiple statements should be enclosed in a do - while loop
#274: FILE: lib/kunit/hooks.c:21:

+#define KUNIT_HOOK(name, retval, args) \
+ retval (*name)args; \
+ EXPORT_SYMBOL(name)

(These are global declarations, so can't live in a do {} while (0) loop)

WARNING: space prohibited between function name and open parenthesis '('
#275: FILE: lib/kunit/hooks.c:22:
+ retval (*name)args; \

(retval is not the function name, it's the return value. name is the
name, the parentheses are for the function pointer syntax.)

ERROR: Macros with complex values should be enclosed in parentheses
#314: FILE: lib/kunit/test.c:774:

+#define KUNIT_HOOK(name, retval, args) \
+ extern retval name##_impl args

(This is a global variable declaration, so can't be enclosed with
parentheses or do {} while (0))

ERROR: Macros with complex values should be enclosed in parentheses
#321: FILE: lib/kunit/test.c:781:

+#define KUNIT_HOOK(name, retval, args) \
+ name = name##_impl

(I _guess_ this one and the following could be put in a do {} while
(0) loop, but parentheses wouldn't work, as this is an assignment.)

ERROR: Macros with complex values should be enclosed in parentheses
#333: FILE: lib/kunit/test.c:797:

+#define KUNIT_HOOK(name, retval, args) \
+ name = NULL

(I _guess_ this one and the previous could be put in a do {} while (0)
loop, but parentheses wouldn't work, as this is an assignment.)

total: 6 errors, 5 warnings, 211 lines checked

Makes sense, will do.

>
> > +void __kunit_fail_current_test_impl(const char *file, int line, const char *fmt, ...)
> > {
> > va_list args;
> > int len;
> > @@ -53,8 +50,6 @@ void __kunit_fail_current_test(const char *file, int line, const char *fmt, ...)
> > kunit_err(current->kunit_test, "%s:%d: %s", file, line, buffer);
> > kunit_kfree(current->kunit_test, buffer);
> > }
> > -EXPORT_SYMBOL_GPL(__kunit_fail_current_test);
> > -#endif
> >
> > /*
> > * Enable KUnit tests to run.
> > @@ -775,8 +770,18 @@ void kunit_cleanup(struct kunit *test)
> > }
> > EXPORT_SYMBOL_GPL(kunit_cleanup);
> >
> > +/* Declarations for the hook implemetnations */
>
> nit: spelling
>

Nice catch, thanks!

[Daniel Latypov]

On Tue, Jan 24, 2023 at 12:04 AM David Gow <davi...@google.com> wrote:
>
> KUnit has several macros and functions intended for use from non-test
> code. These hooks, currently the kunit_get_current_test() and
> kunit_fail_current_test() macros, didn't work when CONFIG_KUNIT=m.
>
> In order to support this case, the required functions and static data
> need to be available unconditionally, even when KUnit itself is not
> built-in. The new 'hooks.c' file is therefore always included, and has
> both the static key required for kunit_get_current_test(), and a
> function pointer to the real implementation of
> __kunit_fail_current_test(), which is populated when the KUnit module is
> loaded.
>
> A new header, kunit/hooks-table.h, contains a table of all hooks, and is
> repeatedly included with different definitions of the KUNIT_HOOK() in
> order to automatically generate the needed function pointer tables. When

Perhaps I'm overlooking something and this is a dumb question.

Is there a reason we can't go with a less-clever approach?
Like have a global struct?
We could memset it to 0 to clear it instead of defining a macro to set
individual variables to NULL?

i.e.

// hooks.h
extern struct kunit_hook_table {
__printf(3, 4) void (*fail_current_test)(const char*, int,
const char*, ...);
} kunit_hooks;

//hooks.c
struct kunit_hook_table kunit_hooks;

// in test.c
// here all the functions should be in scope for us to use
static void kunit_set_hooks(void)
{
kunit_hooks.fail_current_test = __kunit_fail_current_test;
...
}

static int __init kunit_init(void)
{
...
kunit_set_hooks();
...
}

static void __exit kunit_exit(void)
{
...
memset(&kunit_hooks, 0, sizeof(kunit_hooks));

[David Gow]

On Fri, 27 Jan 2023 at 13:38, 'Daniel Latypov' via KUnit Development
<kuni...@googlegroups.com> wrote:
>
> On Tue, Jan 24, 2023 at 12:04 AM David Gow <davi...@google.com> wrote:
> >
> > KUnit has several macros and functions intended for use from non-test
> > code. These hooks, currently the kunit_get_current_test() and
> > kunit_fail_current_test() macros, didn't work when CONFIG_KUNIT=m.
> >
> > In order to support this case, the required functions and static data
> > need to be available unconditionally, even when KUnit itself is not
> > built-in. The new 'hooks.c' file is therefore always included, and has
> > both the static key required for kunit_get_current_test(), and a
> > function pointer to the real implementation of
> > __kunit_fail_current_test(), which is populated when the KUnit module is
> > loaded.
> >
> > A new header, kunit/hooks-table.h, contains a table of all hooks, and is
> > repeatedly included with different definitions of the KUNIT_HOOK() in
> > order to automatically generate the needed function pointer tables. When
>
> Perhaps I'm overlooking something and this is a dumb question.
>
> Is there a reason we can't go with a less-clever approach?
> Like have a global struct?
> We could memset it to 0 to clear it instead of defining a macro to set
> individual variables to NULL?
>

I didn't think of that: it'd definitely fix the need to have a macro
for resetting the pointers to NULL, as well as the definitions.

We'd still have the repetition of the function names in the
kunit_set_hooks function and the table definition. (As well as needing
all of the implementation functions around.)

Still, I think there's value in putting this in a struct, even if we
also use the macro magic for other things. If we, for instance,
instead of setting individual members of struct kunit_hook_table, we
re-initialised it in one go, that might give the compiler enough
context to warn about uninitialised values if we missed one.

The only downside of the struct is that it's slightly uglier if people
want to call the hook function pointer directly. This is not as likely
at the moment, as it could be NULL, but it'd be possible to extend the
macro to generate a stub implementation which just returned nothing.
(Still, it'd be equally possible to autogenerate a wrapper function
which checks kunit_running, so this isn't a dealbreaker.)

> i.e.
>
> // hooks.h
> extern struct kunit_hook_table {
> __printf(3, 4) void (*fail_current_test)(const char*, int,
> const char*, ...);
> } kunit_hooks;
>
> //hooks.c
> struct kunit_hook_table kunit_hooks;
>
> // in test.c
> // here all the functions should be in scope for us to use

This is actually the bit which pushed me over the line and made me
write the macro-based version: if the hook implementations are not all
in test.c (and the static stub ones are in static_stub.c), we'd have
to declare the hook implementation function somewhere, either
introducing a new hooks-impl.h or having a bunch of function
declarations in test.c.

My thought was, if we were going to need an extra header with more
definitions, we might as well just have one, which would also stop us
from worrying about missing an assignment in one place or the other.

> static void kunit_set_hooks(void)
> {
> kunit_hooks.fail_current_test = __kunit_fail_current_test;
> ...
> }
>
> static int __init kunit_init(void)
> {
> ...
> kunit_set_hooks();
> ...
> }
>
> static void __exit kunit_exit(void)
> {
> ...
> memset(&kunit_hooks, 0, sizeof(kunit_hooks));
> }
>

I'll give moving this to a struct a go, it should be an improvement
even if I still use the macros to generate the struct.

The real advantage of the macro is only having to add the new hook in
two or three places:
- The hooks-table.h entry
- The _impl function (which can be in any file)
- (Optionally) a wrapper so that people don't need to check for NULL /
kunit_running themselves.

Without it, they'll have to:
- Add an entry to the struct kunit_hooks_table
- Write the implementation function.
- Add a declaration for the _impl function in test.c, if the function
isn't defined there.
- Add an entry to kunit_set_hooks()
- (Optionally) a wrapper, etc.

That's potentially twice as many things to get right. Still, it's a
lot better with the struct than doing each function individually, so
it's a closer tradeoff.
Personally, I still feel the macro-based version will eventually be
useful, but it's probably 50/50 whether it's worth it for just two or
three hooks.

Worst-case, we can do just the manual struct-based version, and
replace it with the macro one later.

Thanks,
-- David

> > KUnit is disabled, or the module is not loaded, these function pointers
> > are all NULL. This shouldn't be a problem, as they're all used behind
> > wrappers which check kunit_running and/or that the pointer is non-NULL.
> >
> > This can then be extended for future features which require similar
> > "hook" behaviour, such as static stubs:
> > https://lore.kernel.org/all/20221208061841.2...@google.com/
> >
> > Signed-off-by: David Gow <davi...@google.com>
>

> --
> 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/CAGS_qxq4vWvRJ89477S%2BrxHYLvnc2xN435GQ4%2BBvpLgqon8miw%40mail.gmail.com.

[David Gow]

KUnit has several macros and functions intended for use from non-test
code. These hooks, currently the kunit_get_current_test() and
kunit_fail_current_test() macros, didn't work when CONFIG_KUNIT=m.

In order to support this case, the required functions and static data
need to be available unconditionally, even when KUnit itself is not
built-in. The new 'hooks.c' file is therefore always included, and has

both the static key required for kunit_get_current_test(), and a table
of function pointers in struct kunit_hooks_table. This is filled in with
the real implementations by kunit_install_hooks(), which is kept in
hooks-impl.h and called when the kunit module is loaded.

This can be extended for future features which require similar
"hook" behaviour, such as static stubs, by simply adding new entries to
the struct, and the appropriate code to set them.

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

This is basically a prerequisite for the stub features working when
KUnit is built as a module, and should nicely make a few other tests

work, too.

This version uses a struct, rather than a bunch of separate function
pointers, to define the list of hooks in one place. It also doesn't use
the macro magic from RFC v2 (which we could reintroduce later if we end
up with enough hooks that it'd make sense). It does get rid of all of
the nasty checkpatch.pl warnings, though, save for:

WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable

#230: FILE: lib/kunit/hooks.c:16:
+EXPORT_SYMBOL(kunit_running);

This is a false-positive, as the EXPORT_SYMBOL() immediately follows the
DEFINE_STATIC_KEY_FALSE() macro, which checkpatch doesn't recognise as a
definition.

Cheers,
-- David

Changes since RFC v2:
https://lore.kernel.org/linux-kselftest/20230124080350.2...@google.com/
- Get rid of the macro magic, and keep the function pointers in a
struct.
- Also, reset them to NULL using memset, so we don't need to loop
through all of them manually.
- Thanks Daniel!
- Properly forward-declare all of the implementations, now in
"hooks-impl.h", so they can easily be split across different files.
(Needed for the stubs implementation.)
- Extract the stub installation into a separate function,
kunit_install_hooks().
- Thanks Daniel!

Changes since RFC v1:
https://lore.kernel.org/all/20230117142737.2...@google.com/
- Major refit to auto-generate the hook code using macros.
- (Note that previous Reviewed-by tags have not been added, as this is a
big enough change it probably needs a re-reviews. Thanks Rae for
reviewing RFC v1 previously, though!)

---

Documentation/dev-tools/kunit/usage.rst | 14 ++++++-------
include/kunit/test-bug.h | 28 +++++++++----------------
lib/Makefile | 8 +++++++
lib/kunit/Makefile | 3 +++
lib/kunit/hooks-impl.h | 27 ++++++++++++++++++++++++
lib/kunit/hooks.c | 21 +++++++++++++++++++
lib/kunit/test.c | 14 ++++++-------
7 files changed, 82 insertions(+), 33 deletions(-)
create mode 100644 lib/kunit/hooks-impl.h

diff --git a/include/kunit/test-bug.h b/include/kunit/test-bug.h
index c1b2e14eab64..2b505a95b641 100644

--- a/include/kunit/test-bug.h
+++ b/include/kunit/test-bug.h
@@ -1,6 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0 */
/*
- * KUnit API allowing dynamic analysis tools to interact with KUnit tests
+ * KUnit API providing hooks for non-test code to interact with tests.
*
* Copyright (C) 2020, Google LLC.
* Author: Uriel Guajardo <urielg...@google.com>
@@ -9,7 +9,7 @@
#ifndef _KUNIT_TEST_BUG_H
#define _KUNIT_TEST_BUG_H

-#if IS_BUILTIN(CONFIG_KUNIT)
+#if IS_ENABLED(CONFIG_KUNIT)

#include <linux/jump_label.h> /* For static branch */
#include <linux/sched.h>

@@ -17,6 +17,11 @@
/* Static key if KUnit is running any tests. */
DECLARE_STATIC_KEY_FALSE(kunit_running);

+/* Hooks table: a table of function pointers filled in when kunit loads */
+extern struct kunit_hooks_table {
+ __printf(3, 4) void (*fail_current_test)(const char*, int, const char*, ...);
+} kunit_hooks;
+
/**
* kunit_get_current_test() - Return a pointer to the currently running
* KUnit test.
@@ -43,33 +48,20 @@ static inline struct kunit *kunit_get_current_test(void)

* kunit_fail_current_test() - If a KUnit test is running, fail it.
*
* If a KUnit test is running in the current task, mark that test as failed.
- *
- * This macro will only work if KUnit is built-in (though the tests
- * themselves can be modules). Otherwise, it compiles down to nothing.
*/
#define kunit_fail_current_test(fmt, ...) do { \
if (static_branch_unlikely(&kunit_running)) { \

- __kunit_fail_current_test(__FILE__, __LINE__, \

+ /* Guaranteed to be non-NULL when kunit_running true*/ \

+ kunit_hooks.fail_current_test(__FILE__, __LINE__, \

fmt, ##__VA_ARGS__); \
} \
} while (0)

-
-extern __printf(3, 4) void __kunit_fail_current_test(const char *file, int line,
- const char *fmt, ...);

-
#else

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

-/* We define this with an empty helper function so format string warnings work */
-#define kunit_fail_current_test(fmt, ...) \
- __kunit_fail_current_test(__FILE__, __LINE__, fmt, ##__VA_ARGS__)
-

-static inline __printf(3, 4) void __kunit_fail_current_test(const char *file, int line,
- const char *fmt, ...)
-{
-}

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

#endif

diff --git a/lib/Makefile b/lib/Makefile

index 4d9461bfea42..55fd04a7d0fb 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -126,6 +126,14 @@ CFLAGS_test_fpu.o += $(FPU_CFLAGS)

obj-$(CONFIG_TEST_LIVEPATCH) += livepatch/

obj-$(CONFIG_KUNIT) += kunit/
+# Include the KUnit hooks unconditionally. They'll compile to nothing if
+# CONFIG_KUNIT=n, otherwise will be a small table of static data (static key,
+# function pointers) which need to be built-in even when KUnit is a module.

+ifeq ($(CONFIG_KUNIT), m)
+obj-y += kunit/hooks.o
+else
+obj-$(CONFIG_KUNIT) += kunit/hooks.o
+endif

ifeq ($(CONFIG_DEBUG_KOBJECT),y)
CFLAGS_kobject.o += -DDEBUG
diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
index 29aff6562b42..deeb46cc879b 100644
--- a/lib/kunit/Makefile
+++ b/lib/kunit/Makefile
@@ -11,6 +11,9 @@ ifeq ($(CONFIG_KUNIT_DEBUGFS),y)
kunit-objs += debugfs.o
endif

+# KUnit 'hooks' are built-in even when KUnit is built as a module.
+lib-y += hooks.o
+
obj-$(CONFIG_KUNIT_TEST) += kunit-test.o

# string-stream-test compiles built-in only.

diff --git a/lib/kunit/hooks-impl.h b/lib/kunit/hooks-impl.h
new file mode 100644
index 000000000000..d911f40f76db
--- /dev/null
+++ b/lib/kunit/hooks-impl.h
@@ -0,0 +1,27 @@

+/* SPDX-License-Identifier: GPL-2.0 */
+/*

+ * Declarations for hook implementations.
+ *
+ * These will be set as the function pointers in struct kunit_hook_table,
+ * found in include/kunit/test-bug.h.

+ *
+ * Copyright (C) 2023, Google LLC.
+ * Author: David Gow <davi...@google.com>
+ */
+

+#ifndef _KUNIT_HOOKS_IMPL_H
+#define _KUNIT_HOOKS_IMPL_H
+
+#include <kunit/test-bug.h>
+
+/* List of declarations. */
+void __kunit_fail_current_test_impl(const char *file, int line, const char *fmt, ...);
+
+/* Code to set all of the function pointers. */
+static inline void kunit_install_hooks(void)
+{

+ /* Install the KUnit hook functions. */

+ kunit_hooks.fail_current_test = __kunit_fail_current_test_impl;
+}
+
+#endif /* _KUNIT_HOOKS_IMPL_H */

diff --git a/lib/kunit/hooks.c b/lib/kunit/hooks.c
new file mode 100644

index 000000000000..365d98d4953c
--- /dev/null
+++ b/lib/kunit/hooks.c
@@ -0,0 +1,21 @@

+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit 'Hooks' implementation.
+ *
+ * This file contains code / structures which should be built-in even when
+ * KUnit itself is built as a module.
+ *
+ * Copyright (C) 2022, Google LLC.
+ * Author: David Gow <davi...@google.com>
+ */
+

+
+#include <kunit/test-bug.h>
+
+DEFINE_STATIC_KEY_FALSE(kunit_running);
+EXPORT_SYMBOL(kunit_running);
+
+/* Function pointers for hooks. */

+struct kunit_hooks_table kunit_hooks;
+EXPORT_SYMBOL(kunit_hooks);
+
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index c9ebf975e56b..51cae59d8aae 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -17,16 +17,14 @@
#include <linux/sched.h>

#include "debugfs.h"
+#include "hooks-impl.h"

#include "string-stream.h"
#include "try-catch-impl.h"

-DEFINE_STATIC_KEY_FALSE(kunit_running);
-
-#if IS_BUILTIN(CONFIG_KUNIT)
/*

- * Fail the current test and print an error message to the log.
+ * Hook to fail the current test and print an error message to the log.

*/
-void __kunit_fail_current_test(const char *file, int line, const char *fmt, ...)

+void __kunit_fail_current_test_impl(const char *file, int line, const char *fmt, ...)
{
va_list args;
int len;

@@ -53,8 +51,6 @@ void __kunit_fail_current_test(const char *file, int line, const char *fmt, ...)

kunit_err(current->kunit_test, "%s:%d: %s", file, line, buffer);
kunit_kfree(current->kunit_test, buffer);
}
-EXPORT_SYMBOL_GPL(__kunit_fail_current_test);
-#endif

/*
* Enable KUnit tests to run.

@@ -777,6 +773,9 @@ EXPORT_SYMBOL_GPL(kunit_cleanup);

static int __init kunit_init(void)
{
+ /* Install the KUnit hook functions. */

+ kunit_install_hooks();

+
kunit_debugfs_init();
#ifdef CONFIG_MODULES
return register_module_notifier(&kunit_mod_nb);

@@ -788,6 +787,7 @@ late_initcall(kunit_init);

static void __exit kunit_exit(void)
{
+ memset(&kunit_hooks, 0, sizeof(kunit_hooks));

#ifdef CONFIG_MODULES
unregister_module_notifier(&kunit_mod_nb);
#endif
--

2.39.1.456.gfc5497dd1b-goog

[Rae Moar]

On Sat, Jan 28, 2023 at 2:10 AM David Gow <davi...@google.com> wrote:
>
> KUnit has several macros and functions intended for use from non-test
> code. These hooks, currently the kunit_get_current_test() and
> kunit_fail_current_test() macros, didn't work when CONFIG_KUNIT=m.
>
> In order to support this case, the required functions and static data
> need to be available unconditionally, even when KUnit itself is not
> built-in. The new 'hooks.c' file is therefore always included, and has
> both the static key required for kunit_get_current_test(), and a table
> of function pointers in struct kunit_hooks_table. This is filled in with
> the real implementations by kunit_install_hooks(), which is kept in
> hooks-impl.h and called when the kunit module is loaded.
>
> This can be extended for future features which require similar
> "hook" behaviour, such as static stubs, by simply adding new entries to
> the struct, and the appropriate code to set them.
> Signed-off-by: David Gow <davi...@google.com>
> ---
>
> This is basically a prerequisite for the stub features working when
> KUnit is built as a module, and should nicely make a few other tests
> work, too.
>
> This version uses a struct, rather than a bunch of separate function
> pointers, to define the list of hooks in one place. It also doesn't use
> the macro magic from RFC v2 (which we could reintroduce later if we end
> up with enough hooks that it'd make sense). It does get rid of all of

Hi David!

I have reviewed and tested the patch and it looks great to me! I think
this is the cleanest of the versions discussed in the previous
patches. The "macro magic" from the previous version was very neat and
I can see that working well with more hooks as you said above.
Something to keep in the back of our minds in the future. However,
this seems a bit tidier for the moment.

Thanks!

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

> the nasty checkpatch.pl warnings, though, save for:
>
> WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> #230: FILE: lib/kunit/hooks.c:16:
> +EXPORT_SYMBOL(kunit_running);
>
> This is a false-positive, as the EXPORT_SYMBOL() immediately follows the
> DEFINE_STATIC_KEY_FALSE() macro, which checkpatch doesn't recognise as a
> definition.
>

Great that we don't need to deal with most of the checkpatch warnings
from the previous version! This warning seems difficult to avoid in
this case, so this seems alright to me.

I slightly question adding a new file to set the function pointer.
However, as more hooks are added, I understand this will help remove
clutter from test.c, so this looks good to me.

[Brendan Higgins]

On Sat, Jan 28, 2023 at 2:10 AM 'David Gow' via KUnit Development
<kuni...@googlegroups.com> wrote:
>
> KUnit has several macros and functions intended for use from non-test
> code. These hooks, currently the kunit_get_current_test() and
> kunit_fail_current_test() macros, didn't work when CONFIG_KUNIT=m.
>
> In order to support this case, the required functions and static data
> need to be available unconditionally, even when KUnit itself is not
> built-in. The new 'hooks.c' file is therefore always included, and has
> both the static key required for kunit_get_current_test(), and a table
> of function pointers in struct kunit_hooks_table. This is filled in with
> the real implementations by kunit_install_hooks(), which is kept in
> hooks-impl.h and called when the kunit module is loaded.
>
> This can be extended for future features which require similar
> "hook" behaviour, such as static stubs, by simply adding new entries to
> the struct, and the appropriate code to set them.
> Signed-off-by: David Gow <davi...@google.com>

I agree with Rae that a new file for just setting the pointer seems a
bit much, but I also understand the point of separating it out now -
not sure of a better place for it to live. Aside from that, looks good
to me:

Reviewed-by: Brendan Higgins <brendan...@google.com>

[David Gow]

Thanks Rae and Brendan.

The reasoning behind the separate file here is that, because the hook
implementations are not "static" (as they can be found spread across
different files), they need to have a separate prototype. This
would've required two prototypes (one in the file which had the
implementation, one in the file which set the pointer), so it made
sense to combine these into one header.

Adding the code to set the function pointers to that header made sense
just to keep a couple of the lists of hooks together. It's still a
pain that these are spread between "hooks-impl.h" and "test-bug.h",
but it's probably worth it to avoid either making all of the hook
implementations even more public, or using the more complicated macro
magic (at least for now).

Cheers,
-- David