On Mon, 17 Apr 2023 at 18:43, Benjamin Berg <benjamin@xxxxxxxxxxxxxxxx> wrote: > > 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. Excellent point. In general: - It's okay to use expectations within exit and cleanup functions. - It's not okay for assertions to fail within an exit / cleanup handler. - It's not okay to access anything which was allocated on the stack from within a test in exit / cleanup handlers. - It's okay to access and free resources from within cleanup / exit handlers, though it's not permitted to create new resources from cleanup handlers (exit is okay). I do think we need to document this better, at the very least. What I think we'll end up doing is implementing a different system: - The test (and, if successful, cleanup) runs in a kthread. - If it aborts (e.g., due to an assertion), cleanup runs in another kthread. - If this second kthread aborts early, no further cleanup is run. This would protect the KUnit executor thread from misbehaving cleanup functions, and if an assertion happens in a cleanup function, we'll leak things (which is bad), but not dereference a bunch of invalid pointers (which is worse). I've got this mostly working here, and will send it out as a replacement to this patch (that second kthread will have current->kunit_test set, rendering this change redundant). > > 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. Hmm... which clock test is that? Regardless, it sounds like a bug in the test. I think that ultimately, the right solution for dealing with the context pointer issue is to use resources (or, when available, kunit_add_action()), which nicely enforces the ordering as well. In the meantime, though, I guess it just needs wrapping in a lot of "if (ctx)" or similar... > 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? > I think we'll still want to support assertions in init functions (albeit possibly with some documentation pointing out any pitfalls). If actions or resources are used, it's not a problem. Also, a lot of these issues also apply to kunit_skip(), which is used _heavily_ in init functions. Warnings for assertions in exit or cleanup make sense. I _could_ see a reason to allow assertions if we wanted to have, e.g., helpers which asserted that their inputs were valid -- it'd be a bit rough to deny their use if the inputs are known to be okay -- but I'm not aware of any such case in practice yet, so I suspect it's still worth failing tests (and seeing if anyone complains). Cheers, -- David
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
