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

[Thomas Huth <thuth@redhat.com>]

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() ?

Another idea would be to swap the parameters of report() and
report_xfail_status() :

diff --git a/lib/libcflat.h b/lib/libcflat.h
index b94d0ac..d6d1323 100644
--- a/lib/libcflat.h
+++ b/lib/libcflat.h
@@ -99,10 +99,10 @@ 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, bool pass, ...)
-                                       __attribute__((format(printf, 1,
3)));
-extern void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...)
-                                       __attribute__((format(printf, 1,
4)));
+extern void report(bool pass, const char *msg_fmt, ...)
+                                       __attribute__((format(printf, 2,
3)));
+extern void report_xfail(bool xfail, bool pass, const char *msg_fmt, ...)
+                                       __attribute__((format(printf, 3,
4)));
 extern void report_abort(const char *msg_fmt, ...)
                                        __attribute__((format(printf, 1,
2)))
                                        __attribute__((noreturn));
diff --git a/lib/report.c b/lib/report.c
index ca9b4fd..2255dc3 100644
--- a/lib/report.c
+++ b/lib/report.c
@@ -104,18 +104,18 @@ static void va_report(const char *msg_fmt,
        spin_unlock(&lock);
 }

-void report(const char *msg_fmt, bool pass, ...)
+void report(bool pass, const char *msg_fmt, ...)
 {
        va_list va;
-       va_start(va, pass);
+       va_start(va, msg_fmt);
        va_report(msg_fmt, pass, false, false, va);
        va_end(va);
 }

-void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...)
+void report_xfail(bool xfail, bool pass, const char *msg_fmt, ...)
 {
        va_list va;
-       va_start(va, pass);
+       va_start(va, msg_fmt);
        va_report(msg_fmt, pass, xfail, false, va);
        va_end(va);
 }

... then we can keep the "bool" - but we have to fix all calling sites, too.

Paolo, any preferences?

 Thomas