Hi, On Fri, 2023-04-21 at 12:02 +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. For example: > # example_simple_test: test aborted during cleanup. continuing without cleaning up > > 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@xxxxxxxxxx> Great! Looks good to me. Reviewed-by: Benjamin Berg <benjamin.berg@xxxxxxxxx> > --- > > 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 v2: > https://lore.kernel.org/linux-kselftest/20230419085426.1671703-1-davidgow@xxxxxxxxxx/ > - Always run cleanup in its own kthread > - Therefore, never attempt to re-run it if it exits > - Thanks, Benjamin. > Changes since v1: > https://lore.kernel.org/linux-kselftest/20230415091401.681395-1-davidgow@xxxxxxxxxx/ > - Move cleanup execution to another kthread > - (Thanks, Benjamin, for pointing out the assertion issues) > > --- > lib/kunit/test.c | 55 ++++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 48 insertions(+), 7 deletions(-) > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index e2910b261112..2025e51941e6 100644 > --- a/lib/kunit/test.c > +++ b/lib/kunit/test.c > @@ -419,10 +419,50 @@ static void kunit_try_run_case(void *data) > * thread will resume control and handle any necessary clean up. > */ > kunit_run_case_internal(test, suite, test_case); > - /* This line may never be reached. */ > +} > + > +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_context *ctx = data; > @@ -448,12 +488,6 @@ static void kunit_catch_run_case(void *data) > } > return; > } > - > - /* > - * 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. > - */ > - kunit_run_case_cleanup(test, suite); > } > > /* > @@ -478,6 +512,13 @@ static void kunit_run_case_catch_errors(struct kunit_suite *suite, > context.test_case = test_case; > kunit_try_catch_run(try_catch, &context); > > + /* Now run the cleanup */ > + kunit_try_catch_init(try_catch, > + test, > + kunit_try_run_case_cleanup, > + kunit_catch_run_case_cleanup); > + kunit_try_catch_run(try_catch, &context); > + > /* Propagate the parameter result to the test case. */ > if (test->status == KUNIT_FAILURE) > test_case->status = KUNIT_FAILURE;
