| Message ID | 20191012074454.208377-1-morbo@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests,1/2] Use a status enum for reporting pass/fail | expand |
On Sat, Oct 12, 2019 at 12:44:53AM -0700, 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> The threading of these mails has me all kinds of confused. What is the relationship between all these patches? Did you perhaps intend to send some of these as v2? [kvm-unit-tests PATCH 1/2] Use a status enum for reporting pass/fail [kvm-unit-tests PATCH 2/2] Use a status enum for reporting pass/fail [kvm-unit-tests PATCH 1/1] x86: use pointer for end of exception table [kvm-unit-tests PATCH 2/2] x86: use pointer for end of exception table
On Mon, Oct 14, 2019 at 9:27 AM Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Sat, Oct 12, 2019 at 12:44:53AM -0700, 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> > > The threading of these mails has me all kinds of confused. What is the > relationship between all these patches? Did you perhaps intend to send > some of these as v2? > > [kvm-unit-tests PATCH 1/2] Use a status enum for reporting pass/fail > [kvm-unit-tests PATCH 2/2] Use a status enum for reporting pass/fail > [kvm-unit-tests PATCH 1/1] x86: use pointer for end of exception table > [kvm-unit-tests PATCH 2/2] x86: use pointer for end of exception table Yes, the later ones are meant as v2. I'm not familiar with the patch submission policy via emails. :-( -bw
The attached patches have fixes for clang compilation and some failures
seen from the previous "report()" fix.
The first, changing the "report()" function to accept an "enum status"
instead of "unsigned pass" is under discussion, with others making
alternative suggestions. I add the patch here for completeness, but you
may want to wait until the discussion has settled.
The second, using "less than" to compare two global objects' addresses
for inequality, should be less controversial.
This replaces the previous two patches I sent. I apologize for the
spammage.
[kvm-unit-tests PATCH 1/2] Use a status enum for reporting pass/fail
[kvm-unit-tests PATCH 2/2] x86: use pointer for end of exception table
Bill Wendling (2):
lib: use a status enum for reporting pass/fail
x86: don't compare two global objects' addrs for inequality
lib/libcflat.h | 13 +++++++++++--
lib/report.c | 24 ++++++++++++------------
lib/x86/desc.c | 2 +-
3 files changed, 24 insertions(+), 15 deletions(-)
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. Note that the commit message doesn't describe very well what is it that you're fixing (or rather, why it needs fixing). 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); > } > >
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); > > } > > > > >
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); }
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> --- lib/libcflat.h | 13 +++++++++++-- lib/report.c | 24 ++++++++++++------------ 2 files changed, 23 insertions(+), 14 deletions(-)