Quoting Brendan Higgins (2019-08-12 21:57:55)
On Mon, Aug 12, 2019 at 9:22 PM Stephen Boyd sboyd@kernel.org wrote:
Quoting Brendan Higgins (2019-08-12 11:24:12)
diff --git a/include/kunit/test.h b/include/kunit/test.h index 2625bcfeb19ac..93381f841e09f 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -176,6 +178,11 @@ struct kunit { */ bool success; /* Read only after test_case finishes! */ spinlock_t lock; /* Gaurds all mutable test state. */
/** death_test may be both set and unset from multiple threads in a test* case.*/bool death_test; /* Protected by lock. */ /* * Because resources is a list that may be updated multiple times (with * new resources) from any thread associated with a test case, we must@@ -184,6 +191,13 @@ struct kunit { struct list_head resources; /* Protected by lock. */ };
+static inline void kunit_set_death_test(struct kunit *test, bool death_test) +{
spin_lock(&test->lock);test->death_test = death_test;spin_unlock(&test->lock);+}
These getters and setters are using spinlocks again. It doesn't make any sense. It probably needs a rework like was done for the other bool member, success.
No, this is intentional. death_test can transition from false to true and then back to false within the same test. Maybe that deserves a comment?
Yes. How does it transition from true to false again?
Either way, having a spinlock around a read/write API doesn't make sense because it just makes sure that two writes don't overlap, but otherwise does nothing to keep things synchronized. For example a set to true after a set to false when the two calls to set true or false aren't synchronized means they can happen in any order. So I don't see how it needs a spinlock. The lock needs to be one level higher.
void kunit_init_test(struct kunit *test, const char *name);
int kunit_run_tests(struct kunit_suite *suite); diff --git a/include/kunit/try-catch.h b/include/kunit/try-catch.h new file mode 100644 index 0000000000000..8a414a9af0b64 --- /dev/null +++ b/include/kunit/try-catch.h
[...]
+/*
- struct kunit_try_catch - provides a generic way to run code which might fail.
- @context: used to pass user data to the try and catch functions.
- kunit_try_catch provides a generic, architecture independent way to execute
- an arbitrary function of type kunit_try_catch_func_t which may bail out by
- calling kunit_try_catch_throw(). If kunit_try_catch_throw() is called, @try
- is stopped at the site of invocation and @catch is catch is called.
- struct kunit_try_catch provides a generic interface for the functionality
- needed to implement kunit->abort() which in turn is needed for implementing
- assertions. Assertions allow stating a precondition for a test simplifying
- how test cases are written and presented.
- Assertions are like expectations, except they abort (call
- kunit_try_catch_throw()) when the specified condition is not met. This is
- useful when you look at a test case as a logical statement about some piece
- of code, where assertions are the premises for the test case, and the
- conclusion is a set of predicates, rather expectations, that must all be
- true. If your premises are violated, it does not makes sense to continue.
- */
+struct kunit_try_catch {
/* private: internal use only. */struct kunit *test;struct completion *try_completion;int try_result;kunit_try_catch_func_t try;kunit_try_catch_func_t catch;Can these other variables be documented in the kernel doc? And should context be marked as 'public'?
Sure, I can document them.
But I don't think context should be public; it should only be accessed by kunit_try_catch_* functions. context should only be populated by *_init, and will be passed into *try and *catch when they are called internally.
Ok. Then I guess just document them all but keep them all marked as private.
- */
+void kunit_generic_try_catch_init(struct kunit_try_catch *try_catch);
+#endif /* _KUNIT_TRY_CATCH_H */ diff --git a/kunit/test.c b/kunit/test.c index e5080a2c6b29c..995cb53fe4ee9 100644 --- a/kunit/test.c +++ b/kunit/test.c @@ -158,6 +171,21 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert) kunit_print_string_stream(test, stream); }
+void __noreturn kunit_abort(struct kunit *test) +{
kunit_set_death_test(test, true);kunit_try_catch_throw(&test->try_catch);/** Throw could not abort from test.** XXX: we should never reach this line! As kunit_try_catch_throw is* marked __noreturn.*/WARN_ONCE(true, "Throw could not abort from test!\n");Should this just be a BUG_ON? It's supposedly impossible.
It should be impossible; it will only reach this line if there is a bug in kunit_try_catch_throw. The reason I didn't use BUG_ON was because I previously got yelled at for having BUG_ON in this code path.
Nevertheless, I think BUG_ON is more correct, so if you will stand by it, then that's what I will do.
Yeah BUG_ON is appropriate here and self documenting so please use it.
return;}if (kunit_get_death_test(test)) {/** EXPECTED DEATH: kunit_run_case_internal encountered* anticipated fatal error. Everything should be in a safe* state.*/kunit_run_case_cleanup(test, suite);} else {/** UNEXPECTED DEATH: kunit_run_case_internal encountered an* unanticipated fatal error. We have no idea what the state of* the test case is in.*/kunit_handle_test_crash(test, suite, test_case);kunit_set_failure(test);Like was done here.
Sorry, like what?
Just saying this has braces for the if-else.