Re: [PATCH v2 6/7] kunit: Print last test location on fault

From: David Gow
Date: Tue Mar 12 2024 - 00:55:20 EST

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