Hi,
On Sat, 2023-04-15 at 17:14 +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.
My only question here is whether assertions (not expectations) are OK within the exit/cleanup handler. That said, it wouldn't be clear whether we should try to continue cleaning up after every failure, so probably it is reasonable to not do that.
But, I did see that at least the clock tests currently use assertions inside the init function. And, in those tests, if the context allocation fails the exit handler will dereference the NULL pointer.
So, nothing for this patch, but maybe it would make sense to mark tests as failed (or print a warning) if they use assertions inside init, exit or cleanup handlers?
Benjamin
Fix this by setting the active KUnit context for the duration of the test shutdown procedure. When the test exits normally, this does nothing. When run from the KUits previous value (probably NULL) afterwards.
This should 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 becomes useful with the current kunit_add_action() implementation, as actions do not get the KUnit context passed in by default: https://lore.kernel.org/linux-kselftest/CABVgOSmjs0wLUa4=ErkB9tH8p6A1P6N33be...
I think it's probably correct anyway, though, so we should either do this, or totally rule out using kunit_get_current_test() here at all, by resetting current->kunit_test to NULL before running cleanup even in the normal case.
I've only given this the most cursory testing so far (I'm not sure how much of the executor innards I want to expose to be able to actually write a proper test for it), so more eyes and/or suggestions are welcome.
Cheers, -- David
lib/kunit/test.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index e2910b261112..2d7cad249863 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -392,10 +392,21 @@ static void kunit_case_internal_cleanup(struct kunit *test) static void kunit_run_case_cleanup(struct kunit *test, struct kunit_suite *suite) { + /* + * If we're no-longer running from within the test kthread() because it failed + * or timed out, we still need the context to be okay when running exit and + * cleanup functions. + */ + struct kunit *old_current = current->kunit_test;
+ current->kunit_test = test; if (suite->exit) suite->exit(test); kunit_case_internal_cleanup(test);
+ /* Restore the thread's previous test context (probably NULL or test). */ + current->kunit_test = old_current; } struct kunit_try_catch_context {