[PATCH 0/2] kunit: Function Redirection ("static stub") support

[PATCH 0/2] kunit: Function Redirection ("static stub") support

[David Gow]

When writing tests, it'd often be very useful to be able to intercept
calls to a function in the code being tested and replace it with a
test-specific stub. This has always been an obviously missing piece of
KUnit, and the solutions always involve some tradeoffs with cleanliness,
performance, or impact on non-test code. See the folowing document for
some of the challenges:
https://kunit.dev/mocking.html

This series introduces a new "static_stub" feature add support for this
sort of redirection to KUnit tests.

Any function which might want to be intercepted adds a
call to a macro which checks if a test has redirected calls to it, and
calls the corresponding replacement.

Note that at alternate implementation (based on ftrace) was also
proposed in an earlier RFC:
https://lore.kernel.org/linux-kselftest/20220910212804.670622-3-davidgow@google.com/

This series only implements "static" stubbing, as it is more compatible
across different architectures, and more flexible w/r/t inlined code,
but we don't rule out offering the ftrace-based solution as well if the
demand is there in the future.

This feature was presented at LPC 2022, see:
- https://lpc.events/event/16/contributions/1308/
- https://www.youtube.com/watch?v=0Nm06EdXWsE

The KUnit 'example' test suite now includes an example of static stubs
being used, and the new 'Function Redirection' API documentation
provides a step-by-step walkthrough for using the new feature.

In addition, an (in-progress) test for the atkbd driver, which provides
an example of static stubs being used, can be found here:
https://kunit-review.googlesource.com/c/linux/+/5631

Cheers,
-- David

---
David Gow (1):
  kunit: Expose 'static stub' API to redirect functions

Sadiya Kazi (1):
  Documentation: Add Function Redirection API docs

 .../kunit/api/functionredirection.rst         | 162 ++++++++++++++++++
 Documentation/dev-tools/kunit/api/index.rst   |  13 +-
 include/kunit/static_stub.h                   | 117 +++++++++++++
 lib/kunit/Makefile                            |   1 +
 lib/kunit/kunit-example-test.c                |  38 ++++
 lib/kunit/static_stub.c                       | 123 +++++++++++++
 6 files changed, 451 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/dev-tools/kunit/api/functionredirection.rst
 create mode 100644 include/kunit/static_stub.h
 create mode 100644 lib/kunit/static_stub.c

-- 
2.39.0.rc0.267.gcb52ba06e7-goog

[PATCH 1/2] kunit: Expose 'static stub' API to redirect functions

[David Gow]

Add a simple way of redirecting calls to functions by including a
special prologue in the "real" function which checks to see if the
replacement function should be called (and, if so, calls it).

To redirect calls to a function, make the first (non-declaration) line
of the function:

	KUNIT_STATIC_STUB_REDIRECT(function_name, [function arguments]);

(This will compile away to nothing if KUnit is not enabled, otherwise it
will check if a redirection is active, call the replacement function,
and return. This check is protected by a static branch, so has very
little overhead when there are no KUnit tests running.)

Calls to the real function can be redirected to a replacement using:

	kunit_activate_static_stub(test, real_fn, replacement_fn);

The redirection will only affect calls made from within the kthread of
the current test, and will be automatically disabled when the test
completes. It can also be manually disabled with
kunit_deactivate_static_stub().

The 'example' KUnit test suite has a more complete example.

Co-developed-by: Daniel Latypov <dlatypov@google.com>
Signed-off-by: Daniel Latypov <dlatypov@google.com>
Signed-off-by: David Gow <davidgow@google.com>
---

Note that checkpatch.pl does warn about control flow in the
KUNIT_STATIC_STUB_REDIRECT() macro. This is an intentional design choice
(we think it makes the feature easier to use), though if there are
strong objections, we can of course reconsider.

Changes since RFC v2:
https://lore.kernel.org/linux-kselftest/20220910212804.670622-2-davidgow@google.com/
- Now uses the kunit_get_current_test() function, which uses the static
  key to reduce overhead.
  - Thanks Kees for the suggestion.
  - Note that this does prevent redirections from working when
    CONFIG_KUNIT=m -- this is a restriction of kunit_get_current_test()
    which will be removed in a future patch.
- Several tidy-ups to the inline documentation.

Changes since RFC v1:
https://lore.kernel.org/lkml/20220318021314.3225240-2-davidgow@google.com/
- Use typecheck_fn() to fix typechecking in some cases (thanks Brendan)


---
 include/kunit/static_stub.h    | 117 +++++++++++++++++++++++++++++++
 lib/kunit/Makefile             |   1 +
 lib/kunit/kunit-example-test.c |  38 ++++++++++
 lib/kunit/static_stub.c        | 123 +++++++++++++++++++++++++++++++++
 4 files changed, 279 insertions(+)
 create mode 100644 include/kunit/static_stub.h
 create mode 100644 lib/kunit/static_stub.c

diff --git a/include/kunit/static_stub.h b/include/kunit/static_stub.h
new file mode 100644
index 000000000000..df27c40b6c1e
--- /dev/null
+++ b/include/kunit/static_stub.h
@@ -0,0 +1,117 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * KUnit function redirection (static stubbing) API.
+ *
+ * Copyright (C) 2022, Google LLC.
+ * Author: David Gow <davidgow@google.com>
+ */
+#ifndef _KUNIT_STATIC_STUB_H
+#define _KUNIT_STATIC_STUB_H
+
+#if !IS_REACHABLE(CONFIG_KUNIT)
+
+/* If CONFIG_KUNIT is not enabled, these stubs quietly disappear. */
+#define KUNIT_TRIGGER_STATIC_STUB(real_fn_name, args...) do {} while (0)
+
+#else
+
+#include <kunit/test.h>
+
+#include <linux/compiler.h> /* for {un,}likely() */
+#include <linux/sched.h> /* for task_struct */
+
+/* Returns the address of the replacement function. */
+void *__kunit_get_static_stub_address(struct kunit *test, void *real_fn_addr);
+
+/**
+ * KUNIT_STATIC_STUB_REDIRECT() - call a replacement 'static stub' if one exists
+ * @real_fn_name: The name of this function (as an identifier, not a string)
+ * @args: All of the arguments passed to this function
+ *
+ * This is a function prologue which is used to allow calls to the current
+ * function to be redirected by a KUnit test. KUnit tests can call
+ * kunit_activate_static_stub() to pass a replacement function in. The
+ * replacement function will be called by KUNIT_TRIGGER_STATIC_STUB(), which
+ * will then return from the function. If the caller is not in a KUnit context,
+ * the function will continue execution as normal.
+ *
+ * Example:
+ *
+ * .. code-block:: c
+ *
+ *	int real_func(int n)
+ *	{
+ *		KUNIT_STATIC_STUB_REDIRECT(real_func, n);
+ *		return 0;
+ *	}
+ *
+ *	void replacement_func(int n)
+ *	{
+ *		return 42;
+ *	}
+ *
+ *	void example_test(struct kunit *test)
+ *	{
+ *		kunit_activate_static_stub(test, real_func, replacement_func);
+ *		KUNIT_EXPECT_EQ(test, real_func(1), 42);
+ *	}
+ *
+ */
+#define KUNIT_STATIC_STUB_REDIRECT(real_fn_name, args...)		\
+do {									\
+	typeof(&real_fn_name) replacement;				\
+	struct kunit *current_test = current->kunit_test;		\
+									\
+	if (likely(!current_test))					\
+		break;							\
+									\
+	KUNIT_ASSERT_STREQ_MSG(current_test, __func__, #real_fn_name,	\
+			       "Static stub function name mismatch");	\
+									\
+	replacement = __kunit_get_static_stub_address(current_test,	\
+							&real_fn_name);	\
+									\
+	if (unlikely(replacement))					\
+		return replacement(args);				\
+} while (0)
+
+/* Helper function for kunit_activate_static_stub(). The macro does
+ * typechecking, so use it instead.
+ */
+void __kunit_activate_static_stub(struct kunit *test,
+				  void *real_fn_addr,
+				  void *replacement_addr);
+
+/**
+ * kunit_activate_static_stub() - replace a function using static stubs.
+ * @test: A pointer to the 'struct kunit' test context for the current test.
+ * @real_fn_addr: The address of the function to replace.
+ * @replacement_addr: The address of the function to replace it with.
+ *
+ * When activated, calls to real_fn_addr from within this test (even if called
+ * indirectly) will instead call replacement_addr. The function pointed to by
+ * real_fn_addr must begin with the static stub prologue in
+ * KUNIT_TRIGGER_STATIC_STUB() for this to work. real_fn_addr and
+ * replacement_addr must have the same type.
+ *
+ * The redirection can be disabled again with kunit_deactivate_static_stub().
+ */
+#define kunit_activate_static_stub(test, real_fn_addr, replacement_addr) do {	\
+	typecheck_fn(typeof(&real_fn_addr), replacement_addr);			\
+	__kunit_activate_static_stub(test, real_fn_addr, replacement_addr);	\
+} while (0)
+
+
+/**
+ * kunit_deactivate_static_stub() - disable a function redirection
+ * @test: A pointer to the 'struct kunit' test context for the current test.
+ * @real_fn_addr: The address of the function to no-longer redirect
+ *
+ * Deactivates a redirection configured with kunit_activate_static_stub(). After
+ * this function returns, calls to real_fn_addr() will execute the original
+ * real_fn, not any previously-configured replacement.
+ */
+void kunit_deactivate_static_stub(struct kunit *test, void *real_fn_addr);
+
+#endif
+#endif
diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
index 29aff6562b42..78d9d3414b67 100644
--- a/lib/kunit/Makefile
+++ b/lib/kunit/Makefile
@@ -2,6 +2,7 @@ obj-$(CONFIG_KUNIT) +=			kunit.o
 
 kunit-objs +=				test.o \
 					resource.o \
+					static_stub.o \
 					string-stream.o \
 					assert.o \
 					try-catch.o \
diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
index 66cc4e2365ec..cd8b7e51d02b 100644
--- a/lib/kunit/kunit-example-test.c
+++ b/lib/kunit/kunit-example-test.c
@@ -7,6 +7,7 @@
  */
 
 #include <kunit/test.h>
+#include <kunit/static_stub.h>
 
 /*
  * This is the most fundamental element of KUnit, the test case. A test case
@@ -130,6 +131,42 @@ static void example_all_expect_macros_test(struct kunit *test)
 	KUNIT_ASSERT_GT_MSG(test, sizeof(int), 0, "Your ints are 0-bit?!");
 }
 
+/* This is a function we'll replace with static stubs. */
+static int add_one(int i)
+{
+	/* This will trigger the stub if active. */
+	KUNIT_STATIC_STUB_REDIRECT(add_one, i);
+
+	return i + 1;
+}
+
+/* This is used as a replacement for the above function. */
+static int subtract_one(int i)
+{
+	/* We don't need to trigger the stub from the replacement. */
+
+	return i - 1;
+}
+
+/*
+ * This test shows the use of static stubs.
+ */
+static void example_static_stub_test(struct kunit *test)
+{
+	/* By default, function is not stubbed. */
+	KUNIT_EXPECT_EQ(test, add_one(1), 2);
+
+	/* Replace add_one() with subtract_one(). */
+	kunit_activate_static_stub(test, add_one, subtract_one);
+
+	/* add_one() is now replaced. */
+	KUNIT_EXPECT_EQ(test, add_one(1), 0);
+
+	/* Return add_one() to normal. */
+	kunit_deactivate_static_stub(test, add_one);
+	KUNIT_EXPECT_EQ(test, add_one(1), 2);
+}
+
 /*
  * Here we make a list of all the test cases we want to add to the test suite
  * below.
@@ -145,6 +182,7 @@ static struct kunit_case example_test_cases[] = {
 	KUNIT_CASE(example_skip_test),
 	KUNIT_CASE(example_mark_skipped_test),
 	KUNIT_CASE(example_all_expect_macros_test),
+	KUNIT_CASE(example_static_stub_test),
 	{}
 };
 
diff --git a/lib/kunit/static_stub.c b/lib/kunit/static_stub.c
new file mode 100644
index 000000000000..fd6c34f34131
--- /dev/null
+++ b/lib/kunit/static_stub.c
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * KUnit function redirection (static stubbing) API.
+ *
+ * Copyright (C) 2022, Google LLC.
+ * Author: David Gow <davidgow@google.com>
+ */
+
+#include <kunit/test.h>
+#include <kunit/static_stub.h>
+
+
+/* Context for a static stub. This is stored in the resource data. */
+struct kunit_static_stub_ctx {
+	void *real_fn_addr;
+	void *replacement_addr;
+};
+
+static void __kunit_static_stub_resource_free(struct kunit_resource *res)
+{
+	kfree(res->data);
+}
+
+/* Matching function for kunit_find_resource(). match_data is real_fn_addr. */
+static bool __kunit_static_stub_resource_match(struct kunit *test,
+						struct kunit_resource *res,
+						void *match_real_fn_addr)
+{
+	/* This pointer is only valid if res is a static stub resource. */
+	struct kunit_static_stub_ctx *ctx = res->data;
+
+	/* Make sure the resource is a static stub resource. */
+	if (res->free != &__kunit_static_stub_resource_free)
+		return false;
+
+	return ctx->real_fn_addr == match_real_fn_addr;
+}
+
+/* Returns the address of the replacement function. */
+void *__kunit_get_static_stub_address(struct kunit *test, void *real_fn_addr)
+{
+	struct kunit_resource *res;
+	struct kunit_static_stub_ctx *ctx;
+	void *replacement_addr;
+
+	res = kunit_find_resource(test,
+				  __kunit_static_stub_resource_match,
+				  real_fn_addr);
+
+	if (!res)
+		return NULL;
+
+	ctx = res->data;
+	replacement_addr = ctx->replacement_addr;
+	kunit_put_resource(res);
+	return replacement_addr;
+}
+EXPORT_SYMBOL_GPL(__kunit_get_static_stub_address);
+
+void kunit_deactivate_static_stub(struct kunit *test, void *real_fn_addr)
+{
+	struct kunit_resource *res;
+
+	KUNIT_ASSERT_PTR_NE_MSG(test, real_fn_addr, NULL,
+				"Tried to deactivate a NULL stub.");
+
+	/* Look up the existing stub for this function. */
+	res = kunit_find_resource(test,
+				  __kunit_static_stub_resource_match,
+				  real_fn_addr);
+
+	/* Error out if the stub doesn't exist. */
+	KUNIT_ASSERT_PTR_NE_MSG(test, res, NULL,
+				"Tried to deactivate a nonexistent stub.");
+
+	/* Free the stub. We 'put' twice, as we got a reference
+	 * from kunit_find_resource()
+	 */
+	kunit_remove_resource(test, res);
+	kunit_put_resource(res);
+}
+EXPORT_SYMBOL_GPL(kunit_deactivate_static_stub);
+
+/* Helper function for kunit_activate_static_stub(). The macro does
+ * typechecking, so use it instead.
+ */
+void __kunit_activate_static_stub(struct kunit *test,
+				  void *real_fn_addr,
+				  void *replacement_addr)
+{
+	struct kunit_static_stub_ctx *ctx;
+	struct kunit_resource *res;
+
+	KUNIT_ASSERT_PTR_NE_MSG(test, real_fn_addr, NULL,
+				"Tried to activate a stub for function NULL");
+
+	/* If the replacement address is NULL, deactivate the stub. */
+	if (!replacement_addr) {
+		kunit_deactivate_static_stub(test, replacement_addr);
+		return;
+	}
+
+	/* Look up any existing stubs for this function, and replace them. */
+	res = kunit_find_resource(test,
+				  __kunit_static_stub_resource_match,
+				  real_fn_addr);
+	if (res) {
+		ctx = res->data;
+		ctx->replacement_addr = replacement_addr;
+
+		/* We got an extra reference from find_resource(), so put it. */
+		kunit_put_resource(res);
+	} else {
+		ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
+		KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
+		ctx->real_fn_addr = real_fn_addr;
+		ctx->replacement_addr = replacement_addr;
+		res = kunit_alloc_resource(test, NULL,
+				     &__kunit_static_stub_resource_free,
+				     GFP_KERNEL, ctx);
+	}
+}
+EXPORT_SYMBOL_GPL(__kunit_activate_static_stub);
-- 
2.39.0.rc0.267.gcb52ba06e7-goog

[PATCH 2/2] Documentation: Add Function Redirection API docs

[David Gow]

From: Sadiya Kazi <sadiyakazi@google.com>

Added a new page (functionredirection.rst) that describes the Function
Redirection (static stubbing) API. This page will be expanded if we add,
for example, ftrace-based stubbing.

In addition,
1. Updated the api/index.rst page to create an entry for function
   redirection api
2. Updated the toctree to be hidden, reducing redundancy on the
   generated page.

Signed-off-by: Sadiya Kazi <sadiyakazi@google.com>
Co-developed-by: David Gow <davidgow@google.com>
Signed-off-by: David Gow <davidgow@google.com>
---

Note that this patch is new to v1 of the series, and wasn't included in
the previous RFCs.

---
 .../kunit/api/functionredirection.rst         | 162 ++++++++++++++++++
 Documentation/dev-tools/kunit/api/index.rst   |  13 +-
 2 files changed, 172 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/dev-tools/kunit/api/functionredirection.rst

diff --git a/Documentation/dev-tools/kunit/api/functionredirection.rst b/Documentation/dev-tools/kunit/api/functionredirection.rst
new file mode 100644
index 000000000000..fc7644dfea65
--- /dev/null
+++ b/Documentation/dev-tools/kunit/api/functionredirection.rst
@@ -0,0 +1,162 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+========================
+Function Redirection API
+========================
+
+Overview
+========
+
+When writing unit tests, it's important to be able to isolate the code being
+tested from other parts of the kernel. This ensures the reliability of the test
+(it won't be affected by external factors), reduces dependencies on specific
+hardware or config options (making the test easier to run), and protects the
+stability of the rest of the system (making it less likely for test-specific
+state to interfere with the rest of the system).
+
+While for some code (typically generic data structures, helpers, and toher
+"pure function") this is trivial, for others (like device drivers, filesystems,
+core subsystems) the code is heavily coupled with other parts of the kernel.
+
+This often involves global state in some way: be it global lists of devices,
+the filesystem, or hardware state, this needs to be either carefully managed,
+isolated, and restored, or avoided altogether by replacing access to and
+mutation of this state with a "fake" or "mock" variant.
+
+This can be done by refactoring the code to abstract out access to such state,
+by introducing a layer of indirection which can use or emulate a separate set of
+test state. However, such refactoring comes with its own costs (and undertaking
+significant refactoring before being able to write tests is suboptimal).
+
+A simpler way to intercept some of the function calls is to use function
+redirection via static stubs.
+
+
+Static Stubs
+============
+
+Static stubs are a way of redirecting calls to one function (the "real"
+function) to another function (the "replacement" function).
+
+It works by adding a macro to the "real" function which checks to see if a test
+is running, and if a replacement function is available. If so, that function is
+called in place of the original.
+
+Using static stubs is pretty straightforward:
+
+1. Add the KUNIT_STATIC_STUB_REDIRECT() macro to the start of the "real"
+   function.
+
+   This should be the first statement in the function, after any variable
+   declarations. KUNIT_STATIC_STUB_REDIRECT() takes the name of the
+   function, followed by all of the arguments passed to the real function.
+
+   For example:
+
+   .. code-block:: c
+
+	void send_data_to_hardware(const char *str)
+	{
+		KUNIT_STATIC_STUB_REDIRECT(send_data_to_hardware, str);
+		/* real implementation */
+	}
+
+2. Write one or more replacement functions.
+
+   These functions should have the same function signature as the real function.
+   In the event they need to access or modify test-specific state, they can use
+   kunit_get_current_test() to get a struct kunit pointer. This can then
+   be passed to the expectation/assertion macros, or used to look up KUnit
+   resources.
+
+   For example:
+
+   .. code-block:: c
+
+	void fake_send_data_to_hardware(const char *str)
+	{
+		struct kunit *test = kunit_get_current_test();
+		KUNIT_EXPECT_STREQ(test, str, "Hello World!");
+	}
+
+3. Activate the static stub from your test.
+
+   From within a test, the redirection can be enabled with
+   kunit_activate_static_stub(), which accepts a struct kunit pointer,
+   the real function, and the replacement function. You can call this several
+   times with different replacement functions to swap out implementations of the
+   function.
+
+   In our example, this would be
+
+   .. code-block:: c
+
+        kunit_activate_static_stub(test,
+                                   send_data_to_hardware,
+                                   fake_send_data_to_hardware);
+
+4. Call (perhaps indirectly) the real function.
+
+   Once the redirection is activated, any call to the real function will call
+   the replacement function instead. Such calls may be buried deep in the
+   implementation of another function, but must occur from the test's kthread.
+
+   For example:
+
+   .. code-block:: c
+
+        send_data_to_hardware("Hello World!"); /* Succeeds */
+        send_data_to_hardware("Something else"); /* Fails the test. */
+
+5. (Optionally) disable the stub.
+
+   When you no longer need it, the redirection can be disabled (and hence the
+   original behaviour of the 'real' function resumed) using
+   kunit_deactivate_static_stub(). If the stub is not manually deactivated, it
+   will nevertheless be disabled when the test finishes.
+
+   For example:
+
+   .. code-block:: c
+
+        kunit_deactivate_static_stub(test, send_data_to_hardware);
+
+
+It's also possible to use these replacement functions to test to see if a
+function is called at all, for example:
+
+.. code-block:: c
+
+	void send_data_to_hardware(const char *str)
+	{
+		KUNIT_STATIC_STUB_REDIRECT(send_data_to_hardware, str);
+		/* real implementation */
+	}
+
+	/* In test file */
+	int times_called = 0;
+	void fake_send_data_to_hardware(const char *str)
+	{
+		/* fake implementation */
+		times_called++;
+	}
+	...
+	/* In the test case, redirect calls for the duration of the test */
+	kunit_activate_static_stub(test, send_data_to_hardware, fake_send_data_to_hardware);
+
+	send_data_to_hardware("hello");
+	KUNIT_EXPECT_EQ(test, times_called, 1);
+
+	/* Can also deactivate the stub early, if wanted */
+	kunit_deactivate_static_stub(test, send_data_to_hardware);
+
+	send_data_to_hardware("hello again");
+	KUNIT_EXPECT_EQ(test, times_called, 1);
+
+
+
+API Reference
+=============
+
+.. kernel-doc:: include/kunit/static_stub.h
+   :internal:
diff --git a/Documentation/dev-tools/kunit/api/index.rst b/Documentation/dev-tools/kunit/api/index.rst
index 45ce04823f9f..2d8f756aab56 100644
--- a/Documentation/dev-tools/kunit/api/index.rst
+++ b/Documentation/dev-tools/kunit/api/index.rst
@@ -4,17 +4,24 @@
 API Reference
 =============
 .. toctree::
+	:hidden:
 
 	test
 	resource
+	functionredirection
 
-This section documents the KUnit kernel testing API. It is divided into the
+
+This page documents the KUnit kernel testing API. It is divided into the
 following sections:
 
 Documentation/dev-tools/kunit/api/test.rst
 
- - documents all of the standard testing API
+ - Documents all of the standard testing API
 
 Documentation/dev-tools/kunit/api/resource.rst
 
- - documents the KUnit resource API
+ - Documents the KUnit resource API
+
+Documentation/dev-tools/kunit/api/functionredirection.rst
+
+ - Documents the KUnit Function Redirection API
-- 
2.39.0.rc0.267.gcb52ba06e7-goog

Re: [PATCH 2/2] Documentation: Add Function Redirection API docs

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 7777 bytes --]

On Thu, Dec 08, 2022 at 02:18:41PM +0800, David Gow wrote:
> From: Sadiya Kazi <sadiyakazi@google.com>
> 
> Added a new page (functionredirection.rst) that describes the Function
> Redirection (static stubbing) API. This page will be expanded if we add,
> for example, ftrace-based stubbing.

s/Added/Add

> diff --git a/Documentation/dev-tools/kunit/api/functionredirection.rst b/Documentation/dev-tools/kunit/api/functionredirection.rst
> new file mode 100644
> index 000000000000..fc7644dfea65
> --- /dev/null
> +++ b/Documentation/dev-tools/kunit/api/functionredirection.rst
> @@ -0,0 +1,162 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +========================
> +Function Redirection API
> +========================
> +
> +Overview
> +========
> +
> +When writing unit tests, it's important to be able to isolate the code being
> +tested from other parts of the kernel. This ensures the reliability of the test
> +(it won't be affected by external factors), reduces dependencies on specific
> +hardware or config options (making the test easier to run), and protects the
> +stability of the rest of the system (making it less likely for test-specific
> +state to interfere with the rest of the system).

Test reliability is test independence, right?

> +
> +While for some code (typically generic data structures, helpers, and toher
> +"pure function") this is trivial, for others (like device drivers, filesystems,
> +core subsystems) the code is heavily coupled with other parts of the kernel.
> +
> +This often involves global state in some way: be it global lists of devices,
> +the filesystem, or hardware state, this needs to be either carefully managed,
> +isolated, and restored, or avoided altogether by replacing access to and
> +mutation of this state with a "fake" or "mock" variant.

"... or hardware state; this needs ..."

> +
> +This can be done by refactoring the code to abstract out access to such state,
> +by introducing a layer of indirection which can use or emulate a separate set of
> +test state. However, such refactoring comes with its own costs (and undertaking
> +significant refactoring before being able to write tests is suboptimal).
> +
> +A simpler way to intercept some of the function calls is to use function
> +redirection via static stubs.
> +
> +
> +Static Stubs
> +============
> +
> +Static stubs are a way of redirecting calls to one function (the "real"
> +function) to another function (the "replacement" function).
> +
> +It works by adding a macro to the "real" function which checks to see if a test
> +is running, and if a replacement function is available. If so, that function is
> +called in place of the original.
> +
> +Using static stubs is pretty straightforward:
> +
> +1. Add the KUNIT_STATIC_STUB_REDIRECT() macro to the start of the "real"
> +   function.
> +
> +   This should be the first statement in the function, after any variable
> +   declarations. KUNIT_STATIC_STUB_REDIRECT() takes the name of the
> +   function, followed by all of the arguments passed to the real function.
> +
> +   For example:
> +
> +   .. code-block:: c
> +
> +	void send_data_to_hardware(const char *str)
> +	{
> +		KUNIT_STATIC_STUB_REDIRECT(send_data_to_hardware, str);
> +		/* real implementation */
> +	}
> +
> +2. Write one or more replacement functions.
> +
> +   These functions should have the same function signature as the real function.
> +   In the event they need to access or modify test-specific state, they can use
> +   kunit_get_current_test() to get a struct kunit pointer. This can then
> +   be passed to the expectation/assertion macros, or used to look up KUnit
> +   resources.
> +
> +   For example:
> +
> +   .. code-block:: c
> +
> +	void fake_send_data_to_hardware(const char *str)
> +	{
> +		struct kunit *test = kunit_get_current_test();
> +		KUNIT_EXPECT_STREQ(test, str, "Hello World!");
> +	}
> +
> +3. Activate the static stub from your test.
> +
> +   From within a test, the redirection can be enabled with
> +   kunit_activate_static_stub(), which accepts a struct kunit pointer,
> +   the real function, and the replacement function. You can call this several
> +   times with different replacement functions to swap out implementations of the
> +   function.
> +
> +   In our example, this would be
> +
> +   .. code-block:: c
> +
> +        kunit_activate_static_stub(test,
> +                                   send_data_to_hardware,
> +                                   fake_send_data_to_hardware);
> +
> +4. Call (perhaps indirectly) the real function.
> +
> +   Once the redirection is activated, any call to the real function will call
> +   the replacement function instead. Such calls may be buried deep in the
> +   implementation of another function, but must occur from the test's kthread.
> +
> +   For example:
> +
> +   .. code-block:: c
> +
> +        send_data_to_hardware("Hello World!"); /* Succeeds */
> +        send_data_to_hardware("Something else"); /* Fails the test. */
> +
> +5. (Optionally) disable the stub.
> +
> +   When you no longer need it, the redirection can be disabled (and hence the
> +   original behaviour of the 'real' function resumed) using
> +   kunit_deactivate_static_stub(). If the stub is not manually deactivated, it
> +   will nevertheless be disabled when the test finishes.
> +
> +   For example:
> +
> +   .. code-block:: c
> +
> +        kunit_deactivate_static_stub(test, send_data_to_hardware);
> +
> +
> +It's also possible to use these replacement functions to test to see if a
> +function is called at all, for example:
> +
> +.. code-block:: c
> +
> +	void send_data_to_hardware(const char *str)
> +	{
> +		KUNIT_STATIC_STUB_REDIRECT(send_data_to_hardware, str);
> +		/* real implementation */
> +	}
> +
> +	/* In test file */
> +	int times_called = 0;
> +	void fake_send_data_to_hardware(const char *str)
> +	{
> +		/* fake implementation */
> +		times_called++;
> +	}
> +	...
> +	/* In the test case, redirect calls for the duration of the test */
> +	kunit_activate_static_stub(test, send_data_to_hardware, fake_send_data_to_hardware);
> +
> +	send_data_to_hardware("hello");
> +	KUNIT_EXPECT_EQ(test, times_called, 1);
> +
> +	/* Can also deactivate the stub early, if wanted */
> +	kunit_deactivate_static_stub(test, send_data_to_hardware);
> +
> +	send_data_to_hardware("hello again");
> +	KUNIT_EXPECT_EQ(test, times_called, 1);
> +
> +
> +
> +API Reference
> +=============
> +
> +.. kernel-doc:: include/kunit/static_stub.h
> +   :internal:
> diff --git a/Documentation/dev-tools/kunit/api/index.rst b/Documentation/dev-tools/kunit/api/index.rst
> index 45ce04823f9f..2d8f756aab56 100644
> --- a/Documentation/dev-tools/kunit/api/index.rst
> +++ b/Documentation/dev-tools/kunit/api/index.rst
> @@ -4,17 +4,24 @@
>  API Reference
>  =============
>  .. toctree::
> +	:hidden:
>  
>  	test
>  	resource
> +	functionredirection
>  
> -This section documents the KUnit kernel testing API. It is divided into the
> +
> +This page documents the KUnit kernel testing API. It is divided into the
>  following sections:
>  
>  Documentation/dev-tools/kunit/api/test.rst
>  
> - - documents all of the standard testing API
> + - Documents all of the standard testing API
>  
>  Documentation/dev-tools/kunit/api/resource.rst
>  
> - - documents the KUnit resource API
> + - Documents the KUnit resource API
> +
> +Documentation/dev-tools/kunit/api/functionredirection.rst
> +
> + - Documents the KUnit Function Redirection API

Otherwise LGTM.

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/2] Documentation: Add Function Redirection API docs

[Daniel Latypov]

On Wed, Dec 7, 2022 at 10:18 PM 'David Gow' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> From: Sadiya Kazi <sadiyakazi@google.com>
>
> Added a new page (functionredirection.rst) that describes the Function
> Redirection (static stubbing) API. This page will be expanded if we add,
> for example, ftrace-based stubbing.
>
> In addition,
> 1. Updated the api/index.rst page to create an entry for function
>    redirection api
> 2. Updated the toctree to be hidden, reducing redundancy on the
>    generated page.
>
> Signed-off-by: Sadiya Kazi <sadiyakazi@google.com>
> Co-developed-by: David Gow <davidgow@google.com>
> Signed-off-by: David Gow <davidgow@google.com>

Since I wrote the example code snippets (over at
https://kunit.dev/mocking.html#compile-time), I wasn't sure if I
should give an Rb tag.
But the majority of this doc is text I had no part in writing, so with
that caveat:

Reviewed-by: Daniel Latypov <dlatypov@google.com>

I noticed a few typos we could fix.
The rest of my comments are optional suggestions about rewording some
bits and adding `` to identifiers.

> ---
>
> Note that this patch is new to v1 of the series, and wasn't included in
> the previous RFCs.
>
> ---
>  .../kunit/api/functionredirection.rst         | 162 ++++++++++++++++++
>  Documentation/dev-tools/kunit/api/index.rst   |  13 +-
>  2 files changed, 172 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/dev-tools/kunit/api/functionredirection.rst
>
> diff --git a/Documentation/dev-tools/kunit/api/functionredirection.rst b/Documentation/dev-tools/kunit/api/functionredirection.rst
> new file mode 100644
> index 000000000000..fc7644dfea65
> --- /dev/null
> +++ b/Documentation/dev-tools/kunit/api/functionredirection.rst
> @@ -0,0 +1,162 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +========================
> +Function Redirection API
> +========================
> +
> +Overview
> +========
> +
> +When writing unit tests, it's important to be able to isolate the code being
> +tested from other parts of the kernel. This ensures the reliability of the test
> +(it won't be affected by external factors), reduces dependencies on specific
> +hardware or config options (making the test easier to run), and protects the
> +stability of the rest of the system (making it less likely for test-specific
> +state to interfere with the rest of the system).
> +
> +While for some code (typically generic data structures, helpers, and toher

s/toher/other

> +"pure function") this is trivial, for others (like device drivers, filesystems,

s/function/functions, perhaps?

> +core subsystems) the code is heavily coupled with other parts of the kernel.
> +
> +This often involves global state in some way: be it global lists of devices,

s/be it/be it a

> +the filesystem, or hardware state, this needs to be either carefully managed,
> +isolated, and restored, or avoided altogether by replacing access to and
> +mutation of this state with a "fake" or "mock" variant.

optional nit: this sentence feels a bit long.
If we can find a way to split it up, that would be nice.

Perhaps something like:
This coupling is often due to global state: be it a global list of devices...
Tests need to either carefully manage, isolate, and restore state or
they can avoid it altogether by...

> +
> +This can be done by refactoring the code to abstract out access to such state,
> +by introducing a layer of indirection which can use or emulate a separate set of

optional nit: "abstract our access... by introducing a layer of
indirection" feels a bit redundant.
These are the same thing.

Perhaps instead: "abstract out access to such state so tests can..."

> +test state. However, such refactoring comes with its own costs (and undertaking
> +significant refactoring before being able to write tests is suboptimal).
> +
> +A simpler way to intercept some of the function calls is to use function
> +redirection via static stubs.

Maybs s/intercept/replace?
Intercept makes it sounds like we're supporting "test spies", but if
you use the macro below, you have no way of implementing such a thing.

E.g. it makes it sound like we can have
  int func() {
    if (intercepted) { ++func_called; }
    // still run the rest of func
  }

> +
> +
> +Static Stubs
> +============
> +
> +Static stubs are a way of redirecting calls to one function (the "real"
> +function) to another function (the "replacement" function).
> +
> +It works by adding a macro to the "real" function which checks to see if a test
> +is running, and if a replacement function is available. If so, that function is
> +called in place of the original.
> +
> +Using static stubs is pretty straightforward:
> +
> +1. Add the KUNIT_STATIC_STUB_REDIRECT() macro to the start of the "real"

nit: should we use ``KUNIT_STATIC_STUB_REDIRECT()`` to format it as code?

> +   function.
> +
> +   This should be the first statement in the function, after any variable
> +   declarations. KUNIT_STATIC_STUB_REDIRECT() takes the name of the

ditto ``

> +   function, followed by all of the arguments passed to the real function.
> +
> +   For example:
> +
> +   .. code-block:: c
> +
> +       void send_data_to_hardware(const char *str)
> +       {
> +               KUNIT_STATIC_STUB_REDIRECT(send_data_to_hardware, str);
> +               /* real implementation */
> +       }
> +
> +2. Write one or more replacement functions.
> +
> +   These functions should have the same function signature as the real function.
> +   In the event they need to access or modify test-specific state, they can use
> +   kunit_get_current_test() to get a struct kunit pointer. This can then

ditto for ``kunit_get_current_test`` and ``struct kunit``

> +   be passed to the expectation/assertion macros, or used to look up KUnit
> +   resources.
> +
> +   For example:
> +
> +   .. code-block:: c
> +
> +       void fake_send_data_to_hardware(const char *str)
> +       {
> +               struct kunit *test = kunit_get_current_test();
> +               KUNIT_EXPECT_STREQ(test, str, "Hello World!");
> +       }
> +
> +3. Activate the static stub from your test.
> +
> +   From within a test, the redirection can be enabled with
> +   kunit_activate_static_stub(), which accepts a struct kunit pointer,

ditto here

> +   the real function, and the replacement function. You can call this several
> +   times with different replacement functions to swap out implementations of the
> +   function.
> +
> +   In our example, this would be
> +
> +   .. code-block:: c
> +
> +        kunit_activate_static_stub(test,
> +                                   send_data_to_hardware,
> +                                   fake_send_data_to_hardware);
> +
> +4. Call (perhaps indirectly) the real function.
> +
> +   Once the redirection is activated, any call to the real function will call
> +   the replacement function instead. Such calls may be buried deep in the
> +   implementation of another function, but must occur from the test's kthread.
> +
> +   For example:
> +
> +   .. code-block:: c
> +
> +        send_data_to_hardware("Hello World!"); /* Succeeds */
> +        send_data_to_hardware("Something else"); /* Fails the test. */
> +
> +5. (Optionally) disable the stub.
> +
> +   When you no longer need it, the redirection can be disabled (and hence the
> +   original behaviour of the 'real' function resumed) using
> +   kunit_deactivate_static_stub(). If the stub is not manually deactivated, it
> +   will nevertheless be disabled when the test finishes.

optional nit: this block of text feels overly long to me, personally.

Perhaps something shorter like:
When you no longer need it, you can disable the stub manually by
calling ``kunit_deactive_static_stub()``.
Otherwise, it will be disabled automatically at the end of the test.

> +
> +   For example:
> +
> +   .. code-block:: c
> +
> +        kunit_deactivate_static_stub(test, send_data_to_hardware);
> +
> +
> +It's also possible to use these replacement functions to test to see if a
> +function is called at all, for example:
> +
> +.. code-block:: c
> +
> +       void send_data_to_hardware(const char *str)
> +       {
> +               KUNIT_STATIC_STUB_REDIRECT(send_data_to_hardware, str);
> +               /* real implementation */
> +       }
> +
> +       /* In test file */
> +       int times_called = 0;
> +       void fake_send_data_to_hardware(const char *str)
> +       {
> +               /* fake implementation */

minor nit: in the original example, this body was basically a placeholder.
Given we're starting this section with saying "here's how you can
count the function calls", this is the only thing you'd ever put in
the body.

So I'd prefer we just drop the comment.

> +               times_called++;
> +       }
> +       ...
> +       /* In the test case, redirect calls for the duration of the test */
> +       kunit_activate_static_stub(test, send_data_to_hardware, fake_send_data_to_hardware);
> +
> +       send_data_to_hardware("hello");
> +       KUNIT_EXPECT_EQ(test, times_called, 1);
> +
> +       /* Can also deactivate the stub early, if wanted */
> +       kunit_deactivate_static_stub(test, send_data_to_hardware);
> +
> +       send_data_to_hardware("hello again");
> +       KUNIT_EXPECT_EQ(test, times_called, 1);
> +
> +
> +
> +API Reference
> +=============
> +
> +.. kernel-doc:: include/kunit/static_stub.h
> +   :internal:
> diff --git a/Documentation/dev-tools/kunit/api/index.rst b/Documentation/dev-tools/kunit/api/index.rst
> index 45ce04823f9f..2d8f756aab56 100644
> --- a/Documentation/dev-tools/kunit/api/index.rst
> +++ b/Documentation/dev-tools/kunit/api/index.rst
> @@ -4,17 +4,24 @@
>  API Reference
>  =============
>  .. toctree::
> +       :hidden:
>
>         test
>         resource
> +       functionredirection
>
> -This section documents the KUnit kernel testing API. It is divided into the
> +
> +This page documents the KUnit kernel testing API. It is divided into the
>  following sections:
>
>  Documentation/dev-tools/kunit/api/test.rst
>
> - - documents all of the standard testing API
> + - Documents all of the standard testing API
>
>  Documentation/dev-tools/kunit/api/resource.rst
>
> - - documents the KUnit resource API
> + - Documents the KUnit resource API
> +
> +Documentation/dev-tools/kunit/api/functionredirection.rst
> +
> + - Documents the KUnit Function Redirection API
> --
> 2.39.0.rc0.267.gcb52ba06e7-goog

Re: [PATCH 2/2] Documentation: Add Function Redirection API docs

[Daniel Latypov]

On Thu, Dec 8, 2022 at 2:07 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
>
> On Thu, Dec 08, 2022 at 02:18:41PM +0800, David Gow wrote:
> > From: Sadiya Kazi <sadiyakazi@google.com>
> >
> > Added a new page (functionredirection.rst) that describes the Function
> > Redirection (static stubbing) API. This page will be expanded if we add,
> > for example, ftrace-based stubbing.
>
> s/Added/Add
>
> > diff --git a/Documentation/dev-tools/kunit/api/functionredirection.rst b/Documentation/dev-tools/kunit/api/functionredirection.rst
> > new file mode 100644
> > index 000000000000..fc7644dfea65
> > --- /dev/null
> > +++ b/Documentation/dev-tools/kunit/api/functionredirection.rst
> > @@ -0,0 +1,162 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +========================
> > +Function Redirection API
> > +========================
> > +
> > +Overview
> > +========
> > +
> > +When writing unit tests, it's important to be able to isolate the code being
> > +tested from other parts of the kernel. This ensures the reliability of the test
> > +(it won't be affected by external factors), reduces dependencies on specific
> > +hardware or config options (making the test easier to run), and protects the
> > +stability of the rest of the system (making it less likely for test-specific
> > +state to interfere with the rest of the system).
>
> Test reliability is test independence, right?

Just my 2c, but I'd disagree.

E.g. a test can depend on CONFIG_FOO being built-in, even though it
doesn't really need it.
Having an extra kconfig dependency doesn't necessarily make the test
less reliable.

You could define "test independence" wrt shared runtime state (e.g.
multiple tests touching the same global var), but this text
specifically says "hardware or config options."

Daniel

Re: [PATCH 2/2] Documentation: Add Function Redirection API docs

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 12375 bytes --]

On Fri, 16 Dec 2022 at 02:55, 'Daniel Latypov' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> On Wed, Dec 7, 2022 at 10:18 PM 'David Gow' via KUnit Development
> <kunit-dev@googlegroups.com> wrote:
> >
> > From: Sadiya Kazi <sadiyakazi@google.com>
> >
> > Added a new page (functionredirection.rst) that describes the Function
> > Redirection (static stubbing) API. This page will be expanded if we add,
> > for example, ftrace-based stubbing.
> >
> > In addition,
> > 1. Updated the api/index.rst page to create an entry for function
> >    redirection api
> > 2. Updated the toctree to be hidden, reducing redundancy on the
> >    generated page.
> >
> > Signed-off-by: Sadiya Kazi <sadiyakazi@google.com>
> > Co-developed-by: David Gow <davidgow@google.com>
> > Signed-off-by: David Gow <davidgow@google.com>
>
> Since I wrote the example code snippets (over at
> https://kunit.dev/mocking.html#compile-time), I wasn't sure if I
> should give an Rb tag.
> But the majority of this doc is text I had no part in writing, so with
> that caveat:
>
> Reviewed-by: Daniel Latypov <dlatypov@google.com>
>

Thanks: I'd forgotten we'd adapted that code from the kunit.dev
website. We'll add you as a Co-developed-by in the next version.

> I noticed a few typos we could fix.
> The rest of my comments are optional suggestions about rewording some
> bits and adding `` to identifiers.
>

Most of the lack of `` in identifiers is deliberate: because the
kerneldoc comments are included, having the identifiers (particularly
functions and function-like macros, with the () after them)
automatically get turned into links to the reference documentation
below by sphinx.

> > ---
> >
> > Note that this patch is new to v1 of the series, and wasn't included in
> > the previous RFCs.
> >
> > ---
> >  .../kunit/api/functionredirection.rst         | 162 ++++++++++++++++++
> >  Documentation/dev-tools/kunit/api/index.rst   |  13 +-
> >  2 files changed, 172 insertions(+), 3 deletions(-)
> >  create mode 100644 Documentation/dev-tools/kunit/api/functionredirection.rst
> >
> > diff --git a/Documentation/dev-tools/kunit/api/functionredirection.rst b/Documentation/dev-tools/kunit/api/functionredirection.rst
> > new file mode 100644
> > index 000000000000..fc7644dfea65
> > --- /dev/null
> > +++ b/Documentation/dev-tools/kunit/api/functionredirection.rst
> > @@ -0,0 +1,162 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +========================
> > +Function Redirection API
> > +========================
> > +
> > +Overview
> > +========
> > +
> > +When writing unit tests, it's important to be able to isolate the code being
> > +tested from other parts of the kernel. This ensures the reliability of the test
> > +(it won't be affected by external factors), reduces dependencies on specific
> > +hardware or config options (making the test easier to run), and protects the
> > +stability of the rest of the system (making it less likely for test-specific
> > +state to interfere with the rest of the system).
> > +
> > +While for some code (typically generic data structures, helpers, and toher
>
> s/toher/other
>

Nice catch, thanks.

> > +"pure function") this is trivial, for others (like device drivers, filesystems,
>
> s/function/functions, perhaps?
>

Will fix, thanks.

> > +core subsystems) the code is heavily coupled with other parts of the kernel.
> > +
> > +This often involves global state in some way: be it global lists of devices,
>
> s/be it/be it a
>

Will change to "be it a global list" (singular).

> > +the filesystem, or hardware state, this needs to be either carefully managed,
> > +isolated, and restored, or avoided altogether by replacing access to and
> > +mutation of this state with a "fake" or "mock" variant.
>
> optional nit: this sentence feels a bit long.
> If we can find a way to split it up, that would be nice.
>
> Perhaps something like:
> This coupling is often due to global state: be it a global list of devices...
> Tests need to either carefully manage, isolate, and restore state or
> they can avoid it altogether by...
>

Sounds good to me! Will go with this.

> > +
> > +This can be done by refactoring the code to abstract out access to such state,
> > +by introducing a layer of indirection which can use or emulate a separate set of
>
> optional nit: "abstract our access... by introducing a layer of
> indirection" feels a bit redundant.
> These are the same thing.
>
> Perhaps instead: "abstract out access to such state so tests can..."
>

Hmm... I see what you mean, but do feel that explicitly calling out "a
layer of indirection" is more clear than just making it more
"abstract".
I'll play around with the wording of this.

> > +test state. However, such refactoring comes with its own costs (and undertaking
> > +significant refactoring before being able to write tests is suboptimal).
> > +
> > +A simpler way to intercept some of the function calls is to use function
> > +redirection via static stubs.
>
> Maybs s/intercept/replace?
> Intercept makes it sounds like we're supporting "test spies", but if
> you use the macro below, you have no way of implementing such a thing.
>
> E.g. it makes it sound like we can have
>   int func() {
>     if (intercepted) { ++func_called; }
>     // still run the rest of func
>   }
>

Yeah, test spies may be a feature we want to add later, but I agree
this could be confusing.

> > +
> > +
> > +Static Stubs
> > +============
> > +
> > +Static stubs are a way of redirecting calls to one function (the "real"
> > +function) to another function (the "replacement" function).
> > +
> > +It works by adding a macro to the "real" function which checks to see if a test
> > +is running, and if a replacement function is available. If so, that function is
> > +called in place of the original.
> > +
> > +Using static stubs is pretty straightforward:
> > +
> > +1. Add the KUNIT_STATIC_STUB_REDIRECT() macro to the start of the "real"
>
> nit: should we use ``KUNIT_STATIC_STUB_REDIRECT()`` to format it as code?
>

As noted above, sphinx will link to the reference for the macro if we
don't use quotes.

> > +   function.
> > +
> > +   This should be the first statement in the function, after any variable
> > +   declarations. KUNIT_STATIC_STUB_REDIRECT() takes the name of the
>
> ditto ``
>

Again, sphinx links without ``.

> > +   function, followed by all of the arguments passed to the real function.
> > +
> > +   For example:
> > +
> > +   .. code-block:: c
> > +
> > +       void send_data_to_hardware(const char *str)
> > +       {
> > +               KUNIT_STATIC_STUB_REDIRECT(send_data_to_hardware, str);
> > +               /* real implementation */
> > +       }
> > +
> > +2. Write one or more replacement functions.
> > +
> > +   These functions should have the same function signature as the real function.
> > +   In the event they need to access or modify test-specific state, they can use
> > +   kunit_get_current_test() to get a struct kunit pointer. This can then
>
> ditto for ``kunit_get_current_test`` and ``struct kunit``
>

Sphinx will also recognise the 'struct' keyword here, and should link
to the documentation for struct kunit.

> > +   be passed to the expectation/assertion macros, or used to look up KUnit
> > +   resources.
> > +
> > +   For example:
> > +
> > +   .. code-block:: c
> > +
> > +       void fake_send_data_to_hardware(const char *str)
> > +       {
> > +               struct kunit *test = kunit_get_current_test();
> > +               KUNIT_EXPECT_STREQ(test, str, "Hello World!");
> > +       }
> > +
> > +3. Activate the static stub from your test.
> > +
> > +   From within a test, the redirection can be enabled with
> > +   kunit_activate_static_stub(), which accepts a struct kunit pointer,
>
> ditto here
>
> > +   the real function, and the replacement function. You can call this several
> > +   times with different replacement functions to swap out implementations of the
> > +   function.
> > +
> > +   In our example, this would be
> > +
> > +   .. code-block:: c
> > +
> > +        kunit_activate_static_stub(test,
> > +                                   send_data_to_hardware,
> > +                                   fake_send_data_to_hardware);
> > +
> > +4. Call (perhaps indirectly) the real function.
> > +
> > +   Once the redirection is activated, any call to the real function will call
> > +   the replacement function instead. Such calls may be buried deep in the
> > +   implementation of another function, but must occur from the test's kthread.
> > +
> > +   For example:
> > +
> > +   .. code-block:: c
> > +
> > +        send_data_to_hardware("Hello World!"); /* Succeeds */
> > +        send_data_to_hardware("Something else"); /* Fails the test. */
> > +
> > +5. (Optionally) disable the stub.
> > +
> > +   When you no longer need it, the redirection can be disabled (and hence the
> > +   original behaviour of the 'real' function resumed) using
> > +   kunit_deactivate_static_stub(). If the stub is not manually deactivated, it
> > +   will nevertheless be disabled when the test finishes.
>
> optional nit: this block of text feels overly long to me, personally.
>
> Perhaps something shorter like:
> When you no longer need it, you can disable the stub manually by
> calling ``kunit_deactive_static_stub()``.
> Otherwise, it will be disabled automatically at the end of the test.
>

Hmm... I'm not sure if the explicit mention that this resumes the
normal "real" function behaviour is helpful. Will consider for v2.

> > +
> > +   For example:
> > +
> > +   .. code-block:: c
> > +
> > +        kunit_deactivate_static_stub(test, send_data_to_hardware);
> > +
> > +
> > +It's also possible to use these replacement functions to test to see if a
> > +function is called at all, for example:
> > +
> > +.. code-block:: c
> > +
> > +       void send_data_to_hardware(const char *str)
> > +       {
> > +               KUNIT_STATIC_STUB_REDIRECT(send_data_to_hardware, str);
> > +               /* real implementation */
> > +       }
> > +
> > +       /* In test file */
> > +       int times_called = 0;
> > +       void fake_send_data_to_hardware(const char *str)
> > +       {
> > +               /* fake implementation */
>
> minor nit: in the original example, this body was basically a placeholder.
> Given we're starting this section with saying "here's how you can
> count the function calls", this is the only thing you'd ever put in
> the body.
>
> So I'd prefer we just drop the comment.
>

Makes sense, will do.

> > +               times_called++;
> > +       }
> > +       ...
> > +       /* In the test case, redirect calls for the duration of the test */
> > +       kunit_activate_static_stub(test, send_data_to_hardware, fake_send_data_to_hardware);
> > +
> > +       send_data_to_hardware("hello");
> > +       KUNIT_EXPECT_EQ(test, times_called, 1);
> > +
> > +       /* Can also deactivate the stub early, if wanted */
> > +       kunit_deactivate_static_stub(test, send_data_to_hardware);
> > +
> > +       send_data_to_hardware("hello again");
> > +       KUNIT_EXPECT_EQ(test, times_called, 1);
> > +
> > +
> > +
> > +API Reference
> > +=============
> > +
> > +.. kernel-doc:: include/kunit/static_stub.h
> > +   :internal:
> > diff --git a/Documentation/dev-tools/kunit/api/index.rst b/Documentation/dev-tools/kunit/api/index.rst
> > index 45ce04823f9f..2d8f756aab56 100644
> > --- a/Documentation/dev-tools/kunit/api/index.rst
> > +++ b/Documentation/dev-tools/kunit/api/index.rst
> > @@ -4,17 +4,24 @@
> >  API Reference
> >  =============
> >  .. toctree::
> > +       :hidden:
> >
> >         test
> >         resource
> > +       functionredirection
> >
> > -This section documents the KUnit kernel testing API. It is divided into the
> > +
> > +This page documents the KUnit kernel testing API. It is divided into the
> >  following sections:
> >
> >  Documentation/dev-tools/kunit/api/test.rst
> >
> > - - documents all of the standard testing API
> > + - Documents all of the standard testing API
> >
> >  Documentation/dev-tools/kunit/api/resource.rst
> >
> > - - documents the KUnit resource API
> > + - Documents the KUnit resource API
> > +
> > +Documentation/dev-tools/kunit/api/functionredirection.rst
> > +
> > + - Documents the KUnit Function Redirection API
> > --
> > 2.39.0.rc0.267.gcb52ba06e7-goog
>
>

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH 1/2] kunit: Expose 'static stub' API to redirect functions

[Brendan Higgins]

On Thu, Dec 8, 2022 at 1:18 AM 'David Gow' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> Add a simple way of redirecting calls to functions by including a
> special prologue in the "real" function which checks to see if the
> replacement function should be called (and, if so, calls it).
>
> To redirect calls to a function, make the first (non-declaration) line
> of the function:
>
>         KUNIT_STATIC_STUB_REDIRECT(function_name, [function arguments]);
>
> (This will compile away to nothing if KUnit is not enabled, otherwise it
> will check if a redirection is active, call the replacement function,
> and return. This check is protected by a static branch, so has very
> little overhead when there are no KUnit tests running.)
>
> Calls to the real function can be redirected to a replacement using:
>
>         kunit_activate_static_stub(test, real_fn, replacement_fn);
>
> The redirection will only affect calls made from within the kthread of
> the current test, and will be automatically disabled when the test
> completes. It can also be manually disabled with
> kunit_deactivate_static_stub().
>
> The 'example' KUnit test suite has a more complete example.
>
> Co-developed-by: Daniel Latypov <dlatypov@google.com>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> Signed-off-by: David Gow <davidgow@google.com>

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

Re: [PATCH 2/2] Documentation: Add Function Redirection API docs

[Brendan Higgins]

On Thu, Dec 8, 2022 at 1:18 AM 'David Gow' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> From: Sadiya Kazi <sadiyakazi@google.com>
>
> Added a new page (functionredirection.rst) that describes the Function
> Redirection (static stubbing) API. This page will be expanded if we add,
> for example, ftrace-based stubbing.
>
> In addition,
> 1. Updated the api/index.rst page to create an entry for function
>    redirection api
> 2. Updated the toctree to be hidden, reducing redundancy on the
>    generated page.
>
> Signed-off-by: Sadiya Kazi <sadiyakazi@google.com>
> Co-developed-by: David Gow <davidgow@google.com>
> Signed-off-by: David Gow <davidgow@google.com>

Aside from the comments that have already been made, everything looks
good to me.

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