On Fri, Oct 25, 2024 at 04:45:54PM +0100, Alexandru Elisei wrote:
> Hi Drew,
>
> On Wed, Oct 23, 2024 at 06:53:48PM +0200, Andrew Jones wrote:
> > A nice pattern to use in order to try and maintain parsable reports,
> > but also output unexpected values, is
> >
> > if (!report(value == expected_value, "my test")) {
> > report_info("failure due to unexpected value (received %d, expected %d)",
> > value, expected_value);
> > }
>
> This looks like a good idea to me, makes the usage of report() similar to
> the kernel pattern of wrapping an if condition around WARN_ON():
>
> if (WARN_ON(condition)) {
> do_stuff()
> }
>
> Plus, current users are not affected by the change so I see no reason not
> to have the choice.
>
> >
> > Signed-off-by: Andrew Jones <andrew.jones@xxxxxxxxx>
> > ---
> > lib/libcflat.h | 6 +++---
> > lib/report.c | 28 +++++++++++++++++++++-------
> > 2 files changed, 24 insertions(+), 10 deletions(-)
> >
> > diff --git a/lib/libcflat.h b/lib/libcflat.h
> > index eec34c3f2710..b4110b9ec91b 100644
> > --- a/lib/libcflat.h
> > +++ b/lib/libcflat.h
> > @@ -97,11 +97,11 @@ void report_prefix_pushf(const char *prefix_fmt, ...)
> > extern void report_prefix_push(const char *prefix);
> > extern void report_prefix_pop(void);
> > extern void report_prefix_popn(int n);
> > -extern void report(bool pass, const char *msg_fmt, ...)
> > +extern bool report(bool pass, const char *msg_fmt, ...)
> > __attribute__((format(printf, 2, 3), nonnull(2)));
> > -extern void report_xfail(bool xfail, bool pass, const char *msg_fmt, ...)
> > +extern bool report_xfail(bool xfail, bool pass, const char *msg_fmt, ...)
> > __attribute__((format(printf, 3, 4), nonnull(3)));
> > -extern void report_kfail(bool kfail, bool pass, const char *msg_fmt, ...)
> > +extern bool report_kfail(bool kfail, bool pass, const char *msg_fmt, ...)
> > __attribute__((format(printf, 3, 4), nonnull(3)));
> > extern void report_abort(const char *msg_fmt, ...)
> > __attribute__((format(printf, 1, 2)))
> > diff --git a/lib/report.c b/lib/report.c
> > index 0756e64e6f10..43c0102c1b0e 100644
> > --- a/lib/report.c
> > +++ b/lib/report.c
> > @@ -89,7 +89,7 @@ void report_prefix_popn(int n)
> > spin_unlock(&lock);
> > }
> >
> > -static void va_report(const char *msg_fmt,
> > +static bool va_report(const char *msg_fmt,
> > bool pass, bool xfail, bool kfail, bool skip, va_list va)
> > {
> > const char *prefix = skip ? "SKIP"
> > @@ -114,14 +114,20 @@ static void va_report(const char *msg_fmt,
> > failures++;
> >
> > spin_unlock(&lock);
> > +
> > + return pass || xfail;
>
> va_report() has 4 boolean parameters that the callers set. 'kfail' can be
> ignored, because all it does is control which variable serves as the
> accumulator for the failure.
>
> I was thinking about the 'skip' parameter - report_skip() sets pass = xfail
> = false, skip = true. Does it matter that va_report() returns false for
> report_skip()? I don't think so (report_skip() returns void), just wanting
> to make sure we've considered all the cases. Sorry if this looks like
> nitpicking.
I think I considered all the cases, but if you see something missing, then
I'm all ears.
>
> Other than that, the patch looks good to me.
Thanks,
drew
