Received-SPF: pass (google.com: domain of linux-kernel+bounces-100247-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 12 Mar 2024 13:15:43 +0100
From: =?utf-8?Q?Micka=C3=ABl_Sala=C3=BCn?= <mic@digikod.net>
To: David Gow <davidgow@google.com>
Cc: Brendan Higgins <brendanhiggins@google.com>, 
	Kees Cook <keescook@chromium.org>, Rae Moar <rmoar@google.com>, 
	Shuah Khan <skhan@linuxfoundation.org>, Alan Maguire <alan.maguire@oracle.com>, 
	Borislav Petkov <bp@alien8.de>, Dave Hansen <dave.hansen@linux.intel.com>, 
	"H . Peter Anvin" <hpa@zytor.com>, Ingo Molnar <mingo@redhat.com>, 
	James Morris <jamorris@linux.microsoft.com>, Luis Chamberlain <mcgrof@kernel.org>, 
	"Madhavan T . Venkataraman" <madvenka@linux.microsoft.com>, Marco Pagani <marpagan@redhat.com>, 
	Paolo Bonzini <pbonzini@redhat.com>, Sean Christopherson <seanjc@google.com>, 
	Stephen Boyd <sboyd@kernel.org>, Thara Gopinath <tgopinath@microsoft.com>, 
	Thomas Gleixner <tglx@linutronix.de>, Vitaly Kuznetsov <vkuznets@redhat.com>, 
	Wanpeng Li <wanpengli@tencent.com>, Zahra Tarkhani <ztarkhani@microsoft.com>, kvm@vger.kernel.org, 
	linux-hardening@vger.kernel.org, linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org, 
	linux-security-module@vger.kernel.org, linux-um@lists.infradead.org, x86@kernel.org
Subject: Re: [PATCH v2 6/7] kunit: Print last test location on fault
Message-ID: <20240312.ku4TaiC9uosh@digikod.net>
References: <20240301194037.532117-1-mic@digikod.net>
 <20240301194037.532117-7-mic@digikod.net>
 <CABVgOSk_vea-LrPwJet6hQ4D3PBQOLVg32nZ_gE4c9kgGDEEnQ@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CABVgOSk_vea-LrPwJet6hQ4D3PBQOLVg32nZ_gE4c9kgGDEEnQ@mail.gmail.com>

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@digikod.net> wrote:
> >
> > This helps identify the location of test faults.
> >
> > Cc: Brendan Higgins <brendanhiggins@google.com>
> > Cc: David Gow <davidgow@google.com>
> > Cc: Rae Moar <rmoar@google.com>
> > Cc: Shuah Khan <skhan@linuxfoundation.org>
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Mickaël Salaün <mic@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
> >