Re: [kvm-unit-tests PATCH 1/2] Use a status enum for reporting pass/fail

From: Bill Wendling <morbo@google.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: kvm list <kvm@vger.kernel.org>, Jim Mattson <jmattson@google.com>
Subject: Re: [kvm-unit-tests PATCH 1/2] Use a status enum for reporting pass/fail
Date: Tue, 29 Oct 2019 11:51:01 -0700	[thread overview]
Message-ID: <CAGG=3QVxXGuuhc4DphovQEECb2OkB8OwCmbhKi6jn5ERbCo8VA@mail.gmail.com> (raw)
In-Reply-To: <81877082-d6c9-9573-4b44-184695386f4f@redhat.com>

On Mon, Oct 21, 2019 at 8:33 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 12/10/19 09:44, Bill Wendling wrote:
> > Some values passed into "report" as "pass/fail" are larger than the
> > size of the parameter. Instead use a status enum so that the size of the
> > argument no longer matters.
> >
> > Signed-off-by: Bill Wendling <morbo@google.com>
>
> I'm going to apply Thomas's argument reversal patch.

Okay.

> Note that the
> commit message doesn't describe very well what is it that you're fixing
> (or rather, why it needs fixing).
>
My apologies for that. I'll keep that in mind for future patches.

> Paolo
>
> > ---
> >  lib/libcflat.h | 13 +++++++++++--
> >  lib/report.c   | 24 ++++++++++++------------
> >  2 files changed, 23 insertions(+), 14 deletions(-)
> >
> > diff --git a/lib/libcflat.h b/lib/libcflat.h
> > index b6635d9..8f80a1c 100644
> > --- a/lib/libcflat.h
> > +++ b/lib/libcflat.h
> > @@ -95,13 +95,22 @@ extern int vsnprintf(char *buf, int size, const char *fmt, va_list va)
> >  extern int vprintf(const char *fmt, va_list va)
> >                                       __attribute__((format(printf, 1, 0)));
> >
> > +enum status { PASSED, FAILED };
> > +
> > +#define STATUS(x) ((x) != 0 ? PASSED : FAILED)
> > +
> > +#define report(msg_fmt, status, ...) \
> > +     report_status(msg_fmt, STATUS(status) __VA_OPT__(,) __VA_ARGS__)
> > +#define report_xfail(msg_fmt, xfail, status, ...) \
> > +     report_xfail_status(msg_fmt, xfail, STATUS(status) __VA_OPT__(,) __VA_ARGS__)
> > +
> >  void report_prefix_pushf(const char *prefix_fmt, ...)
> >                                       __attribute__((format(printf, 1, 2)));
> >  extern void report_prefix_push(const char *prefix);
> >  extern void report_prefix_pop(void);
> > -extern void report(const char *msg_fmt, unsigned pass, ...)
> > +extern void report_status(const char *msg_fmt, unsigned pass, ...)
> >                                       __attribute__((format(printf, 1, 3)));
> > -extern void report_xfail(const char *msg_fmt, bool xfail, unsigned pass, ...)
> > +extern void report_xfail_status(const char *msg_fmt, bool xfail, enum status status, ...)
> >                                       __attribute__((format(printf, 1, 4)));
> >  extern void report_abort(const char *msg_fmt, ...)
> >                                       __attribute__((format(printf, 1, 2)))
> > diff --git a/lib/report.c b/lib/report.c
> > index 2a5f549..4ba2ac0 100644
> > --- a/lib/report.c
> > +++ b/lib/report.c
> > @@ -80,12 +80,12 @@ void report_prefix_pop(void)
> >       spin_unlock(&lock);
> >  }
> >
> > -static void va_report(const char *msg_fmt,
> > -             bool pass, bool xfail, bool skip, va_list va)
> > +static void va_report(const char *msg_fmt, enum status status, bool xfail,
> > +               bool skip, va_list va)
> >  {
> >       const char *prefix = skip ? "SKIP"
> > -                               : xfail ? (pass ? "XPASS" : "XFAIL")
> > -                                       : (pass ? "PASS"  : "FAIL");
> > +                               : xfail ? (status == PASSED ? "XPASS" : "XFAIL")
> > +                                       : (status == PASSED ? "PASS"  : "FAIL");
> >
> >       spin_lock(&lock);
> >
> > @@ -96,27 +96,27 @@ static void va_report(const char *msg_fmt,
> >       puts("\n");
> >       if (skip)
> >               skipped++;
> > -     else if (xfail && !pass)
> > +     else if (xfail && status == FAILED)
> >               xfailures++;
> > -     else if (xfail || !pass)
> > +     else if (xfail || status == FAILED)
> >               failures++;
> >
> >       spin_unlock(&lock);
> >  }
> >
> > -void report(const char *msg_fmt, unsigned pass, ...)
> > +void report_status(const char *msg_fmt, enum status status, ...)
> >  {
> >       va_list va;
> > -     va_start(va, pass);
> > -     va_report(msg_fmt, pass, false, false, va);
> > +     va_start(va, status);
> > +     va_report(msg_fmt, status, false, false, va);
> >       va_end(va);
> >  }
> >
> > -void report_xfail(const char *msg_fmt, bool xfail, unsigned pass, ...)
> > +void report_xfail_status(const char *msg_fmt, bool xfail, enum status status, ...)
> >  {
> >       va_list va;
> > -     va_start(va, pass);
> > -     va_report(msg_fmt, pass, xfail, false, va);
> > +     va_start(va, status);
> > +     va_report(msg_fmt, status, xfail, false, va);
> >       va_end(va);
> >  }
> >
> >
>