On Sat, 2 Mar 2024 at 03:40, Mickaël Salaün <mic@xxxxxxxxxxx> wrote: > > This helps identify the location of test faults. > > Cc: Brendan Higgins <brendanhiggins@xxxxxxxxxx> > Cc: David Gow <davidgow@xxxxxxxxxx> > Cc: Rae Moar <rmoar@xxxxxxxxxx> > Cc: Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx> > Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx> > Signed-off-by: Mickaël Salaün <mic@xxxxxxxxxxx> > Link: https://lore.kernel.org/r/20240301194037.532117-7-mic@xxxxxxxxxxx > --- I like the idea of this, but am a little bit worried about how confusing it might be, given that the location only updates on those particular macros. Maybe the answer is to make the __KUNIT_SAVE_LOC() macro, or something equivalent, a supported API. One possibility would be to have a KUNIT_MARKER() macro. If we really wanted to, we could expand it to take a string so we can have a more user-friendly KUNIT_MARKER(test, "parsing packet") description of where things went wrong. Another could be to extend this to use the code tagging framework[1], if that lands. That being said, I think this is still an improvement without any of those features. I've left a few comments below. Let me know what you think. Cheers, -- David [1]: https://lwn.net/Articles/906660/ > > Changes since v1: > * Added Kees's Reviewed-by. > --- > include/kunit/test.h | 24 +++++++++++++++++++++--- > lib/kunit/try-catch.c | 10 +++++++--- > 2 files changed, 28 insertions(+), 6 deletions(-) > > diff --git a/include/kunit/test.h b/include/kunit/test.h > index fcb4a4940ace..f3aa66eb0087 100644 > --- a/include/kunit/test.h > +++ b/include/kunit/test.h > @@ -301,6 +301,8 @@ struct kunit { > struct list_head resources; /* Protected by lock. */ > > char status_comment[KUNIT_STATUS_COMMENT_SIZE]; > + /* Saves the last seen test. Useful to help with faults. */ > + struct kunit_loc last_seen; > }; > > static inline void kunit_set_failure(struct kunit *test) > @@ -567,6 +569,15 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt, > #define kunit_err(test, fmt, ...) \ > kunit_printk(KERN_ERR, test, fmt, ##__VA_ARGS__) > > +/* > + * Must be called at the beginning of each KUNIT_*_ASSERTION(). > + * Cf. KUNIT_CURRENT_LOC. > + */ > +#define _KUNIT_SAVE_LOC(test) do { \ > + WRITE_ONCE(test->last_seen.file, __FILE__); \ > + WRITE_ONCE(test->last_seen.line, __LINE__); \ > +} while (0) Can we get rid of the leading '_', make this public, and document it? If we want to rename it to KUNIT_MARKER() or similar, that might work better, too. > + > /** > * KUNIT_SUCCEED() - A no-op expectation. Only exists for code clarity. > * @test: The test context object. > @@ -575,7 +586,7 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt, > * words, it does nothing and only exists for code clarity. See > * KUNIT_EXPECT_TRUE() for more information. > */ > -#define KUNIT_SUCCEED(test) do {} while (0) > +#define KUNIT_SUCCEED(test) _KUNIT_SAVE_LOC(test) > > void __noreturn __kunit_abort(struct kunit *test); > > @@ -601,14 +612,16 @@ void __kunit_do_failed_assertion(struct kunit *test, > } while (0) > > > -#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...) \ > +#define KUNIT_FAIL_ASSERTION(test, assert_type, fmt, ...) do { \ > + _KUNIT_SAVE_LOC(test); \ > _KUNIT_FAILED(test, \ > assert_type, \ > kunit_fail_assert, \ > kunit_fail_assert_format, \ > {}, \ > fmt, \ > - ##__VA_ARGS__) > + ##__VA_ARGS__); \ > +} while (0) > > /** > * KUNIT_FAIL() - Always causes a test to fail when evaluated. > @@ -637,6 +650,7 @@ void __kunit_do_failed_assertion(struct kunit *test, > fmt, \ > ...) \ > do { \ > + _KUNIT_SAVE_LOC(test); \ > if (likely(!!(condition_) == !!expected_true_)) \ > break; \ > \ > @@ -698,6 +712,7 @@ do { \ > .right_text = #right, \ > }; \ > \ > + _KUNIT_SAVE_LOC(test); \ > if (likely(__left op __right)) \ > break; \ > \ > @@ -758,6 +773,7 @@ do { \ > .right_text = #right, \ > }; \ > \ > + _KUNIT_SAVE_LOC(test); \ > if (likely((__left) && (__right) && (strcmp(__left, __right) op 0))) \ > break; \ > \ > @@ -791,6 +807,7 @@ do { \ > .right_text = #right, \ > }; \ > \ > + _KUNIT_SAVE_LOC(test); \ > if (likely(__left && __right)) \ > if (likely(memcmp(__left, __right, __size) op 0)) \ > break; \ > @@ -815,6 +832,7 @@ do { \ > do { \ > const typeof(ptr) __ptr = (ptr); \ > \ > + _KUNIT_SAVE_LOC(test); \ > if (!IS_ERR_OR_NULL(__ptr)) \ > break; \ > \ > diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c > index c6ee4db0b3bd..2ec21c6918f3 100644 > --- a/lib/kunit/try-catch.c > +++ b/lib/kunit/try-catch.c > @@ -91,9 +91,13 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context) > > if (exit_code == -EFAULT) > try_catch->try_result = 0; > - else if (exit_code == -EINTR) > - kunit_err(test, "try faulted\n"); > - else if (exit_code == -ETIMEDOUT) > + else if (exit_code == -EINTR) { > + if (test->last_seen.file) > + kunit_err(test, "try faulted after %s:%d\n", > + test->last_seen.file, test->last_seen.line); It's possibly a bit confusing to just say "after file:line", particularly if we then loop or call a function "higher up" in the file. Maybe something like "try faulted: last line seen %s:%d" is clearer. > + else > + kunit_err(test, "try faulted before the first test\n"); I don't like using "test" here, as it introduces ambiguity between "kunit tests" and "assertions/expectations" if we call them both tests. Maybe just "try faulted" here, or "try faulted (no markers seen)" or similar? > + } else if (exit_code == -ETIMEDOUT) > kunit_err(test, "try timed out\n"); > else if (exit_code) > kunit_err(test, "Unknown error: %d\n", exit_code); > -- > 2.44.0 >
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
