[PATCH v2 6/7] kunit: Print last test location on fault
Mickaël Salaün
mic at digikod.net
Tue Mar 12 12:15:43 UTC 2024
On Tue, Mar 12, 2024 at 12:54:48PM +0800, David Gow wrote:
> On Sat, 2 Mar 2024 at 03:40, Mickaël Salaün <mic at digikod.net> wrote:
> >
> > This helps identify the location of test faults.
> >
> > Cc: Brendan Higgins <brendanhiggins at google.com>
> > Cc: David Gow <davidgow at google.com>
> > Cc: Rae Moar <rmoar at google.com>
> > Cc: Shuah Khan <skhan at linuxfoundation.org>
> > Reviewed-by: Kees Cook <keescook at chromium.org>
> > Signed-off-by: Mickaël Salaün <mic at digikod.net>
> > Link: https://lore.kernel.org/r/20240301194037.532117-7-mic@digikod.net
> > ---
>
> 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.
This patch adds opportunistic markers to test code. Because an uncaught
fault would be a bug, I think in practice nobody would use
KUNIT_MARKER() explicitly. I found _KUNIT_SAVE_LOC() to be useful while
writing tests or debugging them. With this patch, it is still possible
to call KUNIT_SUCCESS() if someone wants to add an explicit mark, and I
think it would make more sense.
>
> 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.
We can do that but I'm not convinced it would be useful.
>
> > +
> > /**
> > * 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.
OK
>
> > + 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?
I agree that "try faulted" would be enough. I'm totally OK for you to
update this patch directly. Please let me know if you'd prefer me to
send another version with these fixes though.
Do you plan to merge this patches with the v6.9 merge window?
>
>
> > + } 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
> >
More information about the Linux-security-module-archive
mailing list