Re: [kvm-unit-tests PATCH] lib: use an argument which doesn't require default argument promotion

[Thomas Huth <thuth@redhat.com>]

On 14/10/2019 10.14, Bill Wendling wrote:
> On Mon, Oct 14, 2019 at 12:57 AM Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 11/10/2019 20.36, Bill Wendling wrote:
>>> I apologize for the breakage. I'm not sure how this escaped me. Here's
>>> a proposed fix. Thoughts?
>>>
>>> commit 5fa1940140fd75a97f3ac5ae2e4de9e1bef645d0
>>> Author: Bill Wendling <morbo@google.com>
>>> Date:   Fri Oct 11 11:26:03 2019 -0700
>>>
>>>     Use a status enum for reporting pass/fail
>>>
>>>     Some values passed into "report" as "pass/fail" are larger than the
>>>     size of the parameter. Use instead a status enum so that the size of the
>>>     argument no longer matters.
>>>
>>> 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);
>>>  }
>>
>> That's certainly a solution... but I wonder whether it might be easier
>> to simply fix the failing tests instead, to make sure that they do not
>> pass a value > sizeof(int) to report() and report_xfail_status() ?
>>
> It may be easier, but it won't stop future changes from encountering
> the same issue.

True.

>> Another idea would be to swap the parameters of report() and
>> report_xfail_status() :
>>
> It's a bit non-standard, but I don't have much of a preference. It
> would take changing tons of places in the code base though.

Yes, it's a bigger change now ... but with your approach, I'm a little
bit afraid that we'll forget the reason over the years, so one day in
the future somebody might wonder what's this "enum status" magic all
about and more or less revert your patch again... so if we take your
patch, I think there should either be some comments in the code as
explanation, or we might want to add builds with clang to the
.travis.yml and gitlab-ci.yml files to make sure that we keep building
the kvm-unit-tests with clang, too.

 Thomas