KUnit tests run in a kthread, with the current->kunit_test pointer set to the test's context. This allows the kunit_get_current_test() and kunit_fail_current_test() macros to work. Normally, this pointer is still valid during test shutdown (i.e., the suite->exit function, and any resource cleanup). However, if the test has exited early (e.g., due to a failed assertion), the cleanup is done in the parent KUnit thread, which does not have an active context.
Instead, in the event test terminates early, run the test exit and cleanup from a new 'cleanup' kthread, which sets current->kunit_test, and better isolates the rest of KUnit from issues which arise in test cleanup.
If a test cleanup function itself aborts (e.g., due to an assertion failing), there will be no further attempts to clean up: an error will be logged and the test failed.
This should also make it easier to get access to the KUnit context, particularly from within resource cleanup functions, which may, for example, need access to data in test->priv.
Signed-off-by: David Gow davidgow@google.com --- This is an updated version of / replacement of "kunit: Set the current KUnit context when cleaning up", which instead creates a new kthread for cleanup tasks if the original test kthread is aborted. This protects us from failed assertions during cleanup, if the test exited early.
Changes since v1: https://lore.kernel.org/linux-kselftest/20230415091401.681395-1-davidgow@goo... - Move cleanup execution to another kthread - (Thanks, Benjamin, for pointing out the assertion issues)
--- lib/kunit/test.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 52 insertions(+), 2 deletions(-)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index e2910b261112..caeae0dfd82b 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -423,8 +423,51 @@ static void kunit_try_run_case(void *data) kunit_run_case_cleanup(test, suite); }
+static void kunit_try_run_case_cleanup(void *data) +{ + struct kunit_try_catch_context *ctx = data; + struct kunit *test = ctx->test; + struct kunit_suite *suite = ctx->suite; + + current->kunit_test = test; + + kunit_run_case_cleanup(test, suite); +} + +static void kunit_catch_run_case_cleanup(void *data) +{ + struct kunit_try_catch_context *ctx = data; + struct kunit *test = ctx->test; + int try_exit_code = kunit_try_catch_get_result(&test->try_catch); + + /* It is always a failure if cleanup aborts. */ + kunit_set_failure(test); + + if (try_exit_code) { + /* + * Test case could not finish, we have no idea what state it is + * in, so don't do clean up. + */ + if (try_exit_code == -ETIMEDOUT) { + kunit_err(test, "test case cleanup timed out\n"); + /* + * Unknown internal error occurred preventing test case from + * running, so there is nothing to clean up. + */ + } else { + kunit_err(test, "internal error occurred during test case cleanup: %d\n", + try_exit_code); + } + return; + } + + kunit_err(test, "test aborted during cleanup. continuing without cleaning up\n"); +} + + static void kunit_catch_run_case(void *data) { + struct kunit_try_catch cleanup; struct kunit_try_catch_context *ctx = data; struct kunit *test = ctx->test; struct kunit_suite *suite = ctx->suite; @@ -451,9 +494,16 @@ static void kunit_catch_run_case(void *data)
/* * Test case was run, but aborted. It is the test case's business as to - * whether it failed or not, we just need to clean up. + * whether it failed or not, we just need to clean up. Do this in a new + * try / catch context, in case it asserts, too. */ - kunit_run_case_cleanup(test, suite); + kunit_try_catch_init(&cleanup, + test, + kunit_try_run_case_cleanup, + kunit_catch_run_case_cleanup); + ctx->test = test; + ctx->suite = suite; + kunit_try_catch_run(&cleanup, ctx); }
/*
As assertions abort the test cleanup process, they should be avoided from within a suite's exit function, or from within resource 'free' functions. Unlike with initialisation or main test execution, no further cleanup will be performed after a failed assertion, potentially causing a leak of resources.
Signed-off-by: David Gow davidgow@google.com ---
This patch is new in v2.
--- Documentation/dev-tools/kunit/usage.rst | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst index 9faf2b4153fc..9f720f1317d3 100644 --- a/Documentation/dev-tools/kunit/usage.rst +++ b/Documentation/dev-tools/kunit/usage.rst @@ -121,6 +121,12 @@ there's an allocation error. ``return`` so they only work from the test function. In KUnit, we stop the current kthread on failure, so you can call them from anywhere.
+.. note:: + Warning: There is an exception to the above rule. You shouldn't use assertions + in the suite's exit() function, or in the free function for a resource. These + run when a test is shutting down, and an assertion here prevents further + cleanup code from running, potentially leading to a memory leak. + Customizing error messages --------------------------
KUnit's exit functions will run even if the corresponding init function fails. It's easy, when writing an exit function, to assume the init function succeeded, and (for example) access uninitialised memory or dereference NULL pointers.
Note that this case exists and should be handled in the documentation.
Suggested-by: Benjamin Berg benjamin@sipsolutions.net Link: https://lore.kernel.org/linux-kselftest/a39af0400abedb2e9b31d84c37551cecc3ee... Signed-off-by: David Gow davidgow@google.com ---
This patch is new in v2.
--- Documentation/dev-tools/kunit/usage.rst | 12 ++++++++++-- include/kunit/test.h | 3 +++ 2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst index 9f720f1317d3..f6d6c9a9ff54 100644 --- a/Documentation/dev-tools/kunit/usage.rst +++ b/Documentation/dev-tools/kunit/usage.rst @@ -166,7 +166,12 @@ many similar tests. In order to reduce duplication in these closely related tests, most unit testing frameworks (including KUnit) provide the concept of a *test suite*. A test suite is a collection of test cases for a unit of code with optional setup and teardown functions that run before/after the whole -suite and/or every test case. For example: +suite and/or every test case. + +.. note:: + A test case will only run if it is associated with a test suite. + +For example:
.. code-block:: c
@@ -196,7 +201,10 @@ after everything else. ``kunit_test_suite(example_test_suite)`` registers the test suite with the KUnit test framework.
.. note:: - A test case will only run if it is associated with a test suite. + The ``exit`` and ``suite_exit`` functions will run even if ``init`` or + ``suite_init`` fail. Make sure that they can handle any inconsistent + state which may result from ``init`` or ``suite_init`` encoutering errors + or exiting early.
``kunit_test_suite(...)`` is a macro which tells the linker to put the specified test suite in a special linker section so that it can be run by KUnit diff --git a/include/kunit/test.h b/include/kunit/test.h index 57b309c6ca27..3028a1a3fcad 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -168,6 +168,9 @@ static inline char *kunit_status_to_ok_not_ok(enum kunit_status status) * test case, similar to the notion of a *test fixture* or a *test class* * in other unit testing frameworks like JUnit or Googletest. * + * Note that @exit and @suite_exit will run even if @init or @suite_init + * fail: make sure they can handle any inconsistent state which may result. + * * Every &struct kunit_case must be associated with a kunit_suite for KUnit * to run it. */
Hi David,
On Wed, 2023-04-19 at 16:54 +0800, David Gow wrote:
KUnit tests run in a kthread, with the current->kunit_test pointer set to the test's context. This allows the kunit_get_current_test() and kunit_fail_current_test() macros to work. Normally, this pointer is still valid during test shutdown (i.e., the suite->exit function, and any resource cleanup). However, if the test has exited early (e.g., due to a failed assertion), the cleanup is done in the parent KUnit thread, which does not have an active context.
Instead, in the event test terminates early, run the test exit and cleanup from a new 'cleanup' kthread, which sets current->kunit_test, and better isolates the rest of KUnit from issues which arise in test cleanup.
If a test cleanup function itself aborts (e.g., due to an assertion failing), there will be no further attempts to clean up: an error will be logged and the test failed.
This should also make it easier to get access to the KUnit context, particularly from within resource cleanup functions, which may, for example, need access to data in test->priv.
Signed-off-by: David Gow davidgow@google.com
This is an updated version of / replacement of "kunit: Set the current KUnit context when cleaning up", which instead creates a new kthread for cleanup tasks if the original test kthread is aborted. This protects us from failed assertions during cleanup, if the test exited early.
Changes since v1: https://lore.kernel.org/linux-kselftest/20230415091401.681395-1-davidgow@goo...
- Move cleanup execution to another kthread
- (Thanks, Benjamin, for pointing out the assertion issues)
Nice, I think this is looking promising. After thinking about it a bit, maybe one thing to improve is to always start the new cleanup kthread from kunit_run_case_catch_errors.
That way there is only one codepath. But, more importantly, it means if the cleanup fails the first time, we do not risk running it a second time and we get slightly nicer error reporting. Not that this happening would be a big issue.
Benjamin
lib/kunit/test.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 52 insertions(+), 2 deletions(-)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index e2910b261112..caeae0dfd82b 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -423,8 +423,51 @@ static void kunit_try_run_case(void *data) kunit_run_case_cleanup(test, suite); } +static void kunit_try_run_case_cleanup(void *data) +{ + struct kunit_try_catch_context *ctx = data; + struct kunit *test = ctx->test; + struct kunit_suite *suite = ctx->suite;
+ current->kunit_test = test;
+ kunit_run_case_cleanup(test, suite); +}
+static void kunit_catch_run_case_cleanup(void *data) +{ + struct kunit_try_catch_context *ctx = data; + struct kunit *test = ctx->test; + int try_exit_code = kunit_try_catch_get_result(&test->try_catch);
+ /* It is always a failure if cleanup aborts. */ + kunit_set_failure(test);
+ if (try_exit_code) { + /* + * Test case could not finish, we have no idea what state it is + * in, so don't do clean up. + */ + if (try_exit_code == -ETIMEDOUT) { + kunit_err(test, "test case cleanup timed out\n"); + /* + * Unknown internal error occurred preventing test case from + * running, so there is nothing to clean up. + */ + } else { + kunit_err(test, "internal error occurred during test case cleanup: %d\n", + try_exit_code); + } + return; + }
+ kunit_err(test, "test aborted during cleanup. continuing without cleaning up\n"); +}
static void kunit_catch_run_case(void *data) { + struct kunit_try_catch cleanup; struct kunit_try_catch_context *ctx = data; struct kunit *test = ctx->test; struct kunit_suite *suite = ctx->suite; @@ -451,9 +494,16 @@ static void kunit_catch_run_case(void *data) /* * Test case was run, but aborted. It is the test case's business as to - * whether it failed or not, we just need to clean up. + * whether it failed or not, we just need to clean up. Do this in a new + * try / catch context, in case it asserts, too. */ - kunit_run_case_cleanup(test, suite); + kunit_try_catch_init(&cleanup, + test, + kunit_try_run_case_cleanup, + kunit_catch_run_case_cleanup); + ctx->test = test; + ctx->suite = suite; + kunit_try_catch_run(&cleanup, ctx); } /*
linux-kselftest-mirror@lists.linaro.org
-
Benjamin Berg -
David Gow