[Qemu-devel] [PATCH v2 0/2] qemu-error: advanced report_once handling

[Qemu-devel] [PATCH v2 0/2] qemu-error: advanced report_once handling

[Cornelia Huck]

[Markus: I've decided to not include your R-b, as I did too many changes
to feel comfortable with that.]

[Also note that I'm about to disappear on vacation, so don't expect
quick responses. I just want to get it out before I forget about it.]

Based on previous discussions, I wanted to enhance the recently
merged report_once infrastucture with a way to print a message
once based on a variable instead of globally-once, similar to
what vfio-ccw uses today.

Not really tested, mainly wanted to get this out before my vacation
to get the ball rolling.

v1->v2:
- merged patch 3 (use the new function in vfio-ccw) into patch 1
- require a non-NULL 'printed' parameter
- have the functions return whether something was printed and slightly
  simplify the existing macros
- adapt macro formatting to suit my aesthetic sensibilities

Cornelia Huck (2):
  qemu-error: add {error,warn}_report_once_cond
  qemu-error: make use of {error,warn}_report_once_cond

 hw/vfio/ccw.c               | 18 +++--------------
 include/qemu/error-report.h | 39 ++++++++++++++++++------------------
 util/qemu-error.c           | 48 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+), 35 deletions(-)

-- 
2.14.4

[Qemu-devel] [PATCH v2 1/2] qemu-error: add {error, warn}_report_once_cond

[Cornelia Huck]

Add two functions to print an error/warning report once depending
on a passed-in condition variable and flip it if printed. This is
useful if you want to print a message not once-globally, but e.g.
once-per-device.

Inspired by warn_once() in hw/vfio/ccw.c, which has been replaced
with warn_report_once_cond().

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/vfio/ccw.c               | 18 +++--------------
 include/qemu/error-report.h |  5 +++++
 util/qemu-error.c           | 48 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 15 deletions(-)

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index e96bbdc78b..9246729a75 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -37,24 +37,12 @@ typedef struct VFIOCCWDevice {
     bool warned_orb_pfch;
 } VFIOCCWDevice;
 
-static inline void warn_once(bool *warned, const char *fmt, ...)
-{
-    va_list ap;
-
-    if (!warned || *warned) {
-        return;
-    }
-    *warned = true;
-    va_start(ap, fmt);
-    warn_vreport(fmt, ap);
-    va_end(ap);
-}
-
 static inline void warn_once_pfch(VFIOCCWDevice *vcdev, SubchDev *sch,
                                   const char *msg)
 {
-    warn_once(&vcdev->warned_orb_pfch, "vfio-ccw (devno %x.%x.%04x): %s",
-              sch->cssid, sch->ssid, sch->devno, msg);
+    warn_report_once_cond(&vcdev->warned_orb_pfch,
+                          "vfio-ccw (devno %x.%x.%04x): %s",
+                          sch->cssid, sch->ssid, sch->devno, msg);
 }
 
 static void vfio_ccw_compute_needs_reset(VFIODevice *vdev)
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index 72fab2b031..e415128ac4 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -44,6 +44,11 @@ void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 void warn_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 void info_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 
+bool error_report_once_cond(bool *printed, const char *fmt, ...)
+    GCC_FMT_ATTR(2, 3);
+bool warn_report_once_cond(bool *printed, const char *fmt, ...)
+    GCC_FMT_ATTR(2, 3);
+
 /*
  * Similar to error_report(), except it prints the message just once.
  * Return true when it prints, false otherwise.
diff --git a/util/qemu-error.c b/util/qemu-error.c
index a25d3b94c6..b77e0bac4c 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -310,3 +310,51 @@ void info_report(const char *fmt, ...)
     vreport(REPORT_TYPE_INFO, fmt, ap);
     va_end(ap);
 }
+
+/*
+ * If *printed is false, print an error message to current monitor if we
+ * have one, else to stderr, and flip *printed to true.
+ * Returns false if message was not printed, else true.
+ * Format arguments like sprintf().  The resulting message should be
+ * a single phrase, with no newline or trailing punctuation.
+ * Prepend the current location and append a newline.
+ * It's wrong to call this in a QMP monitor.  Use error_setg() there.
+ */
+bool error_report_once_cond(bool *printed, const char *fmt, ...)
+{
+    va_list ap;
+
+    assert(printed);
+    if (*printed) {
+        return false;
+    }
+    *printed = true;
+    va_start(ap, fmt);
+    vreport(REPORT_TYPE_ERROR, fmt, ap);
+    va_end(ap);
+    return true;
+}
+
+/*
+ * If *printed is false, print a warning message to current monitor if we
+ * have one, else to stderr, and flip *printed to true.
+ * Returns false if message was not printed, else true.
+ * Format arguments like sprintf().  The resulting message should be
+ * a single phrase, with no newline or trailing punctuation.
+ * Prepend the current location and append a newline.
+ * It's wrong to call this in a QMP monitor.  Use error_setg() there.
+ */
+bool warn_report_once_cond(bool *printed, const char *fmt, ...)
+{
+    va_list ap;
+
+    assert(printed);
+    if (*printed) {
+        return false;
+    }
+    *printed = true;
+    va_start(ap, fmt);
+    vreport(REPORT_TYPE_WARNING, fmt, ap);
+    va_end(ap);
+    return true;
+}
-- 
2.14.4

[Qemu-devel] [PATCH v2 2/2] qemu-error: make use of {error, warn}_report_once_cond

[Cornelia Huck]

{error,warn}_report_once() are a special case of the new functions
and can simply switch to them.

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 include/qemu/error-report.h | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index e415128ac4..918cb936d8 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -53,32 +53,26 @@ bool warn_report_once_cond(bool *printed, const char *fmt, ...)
  * Similar to error_report(), except it prints the message just once.
  * Return true when it prints, false otherwise.
  */
-#define error_report_once(fmt, ...)             \
-    ({                                          \
-        static bool print_once_;                \
-        bool ret_print_once_ = !print_once_;    \
-                                                \
-        if (!print_once_) {                     \
-            print_once_ = true;                 \
-            error_report(fmt, ##__VA_ARGS__);   \
-        }                                       \
-        unlikely(ret_print_once_);              \
+#define error_report_once(fmt, ...)                     \
+    ({                                                  \
+        static bool print_once_;                        \
+        bool ret_print_once_ =                          \
+            error_report_once_cond(&print_once_,        \
+                                   fmt, ##__VA_ARGS__); \
+        unlikely(ret_print_once_);                      \
     })
 
 /*
  * Similar to warn_report(), except it prints the message just once.
  * Return true when it prints, false otherwise.
  */
-#define warn_report_once(fmt, ...)              \
-    ({                                          \
-        static bool print_once_;                \
-        bool ret_print_once_ = !print_once_;    \
-                                                \
-        if (!print_once_) {                     \
-            print_once_ = true;                 \
-            warn_report(fmt, ##__VA_ARGS__);    \
-        }                                       \
-        unlikely(ret_print_once_);              \
+#define warn_report_once(fmt, ...)                      \
+    ({                                                  \
+        static bool print_once_;                        \
+        bool ret_print_once_ =                          \
+            warn_report_once_cond(&print_once_,         \
+                                  fmt, ##__VA_ARGS__);  \
+        unlikely(ret_print_once_);                      \
     })
 
 const char *error_get_progname(void);
-- 
2.14.4

Re: [Qemu-devel] [PATCH v2 1/2] qemu-error: add {error, warn}_report_once_cond

[Markus Armbruster]

Cornelia Huck <cohuck@redhat.com> writes:

> Add two functions to print an error/warning report once depending
> on a passed-in condition variable and flip it if printed. This is
> useful if you want to print a message not once-globally, but e.g.
> once-per-device.
>
> Inspired by warn_once() in hw/vfio/ccw.c, which has been replaced
> with warn_report_once_cond().
>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  hw/vfio/ccw.c               | 18 +++--------------
>  include/qemu/error-report.h |  5 +++++
>  util/qemu-error.c           | 48 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 56 insertions(+), 15 deletions(-)
>
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index e96bbdc78b..9246729a75 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -37,24 +37,12 @@ typedef struct VFIOCCWDevice {
>      bool warned_orb_pfch;
>  } VFIOCCWDevice;
>  
> -static inline void warn_once(bool *warned, const char *fmt, ...)
> -{
> -    va_list ap;
> -
> -    if (!warned || *warned) {
> -        return;
> -    }
> -    *warned = true;
> -    va_start(ap, fmt);
> -    warn_vreport(fmt, ap);
> -    va_end(ap);
> -}
> -
>  static inline void warn_once_pfch(VFIOCCWDevice *vcdev, SubchDev *sch,
>                                    const char *msg)
>  {
> -    warn_once(&vcdev->warned_orb_pfch, "vfio-ccw (devno %x.%x.%04x): %s",
> -              sch->cssid, sch->ssid, sch->devno, msg);
> +    warn_report_once_cond(&vcdev->warned_orb_pfch,
> +                          "vfio-ccw (devno %x.%x.%04x): %s",
> +                          sch->cssid, sch->ssid, sch->devno, msg);
>  }
>  
>  static void vfio_ccw_compute_needs_reset(VFIODevice *vdev)
> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
> index 72fab2b031..e415128ac4 100644
> --- a/include/qemu/error-report.h
> +++ b/include/qemu/error-report.h
> @@ -44,6 +44,11 @@ void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>  void warn_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>  void info_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>  
> +bool error_report_once_cond(bool *printed, const char *fmt, ...)
> +    GCC_FMT_ATTR(2, 3);
> +bool warn_report_once_cond(bool *printed, const char *fmt, ...)
> +    GCC_FMT_ATTR(2, 3);
> +
>  /*
>   * Similar to error_report(), except it prints the message just once.
>   * Return true when it prints, false otherwise.
> diff --git a/util/qemu-error.c b/util/qemu-error.c
> index a25d3b94c6..b77e0bac4c 100644
> --- a/util/qemu-error.c
> +++ b/util/qemu-error.c
> @@ -310,3 +310,51 @@ void info_report(const char *fmt, ...)
>      vreport(REPORT_TYPE_INFO, fmt, ap);
>      va_end(ap);
>  }
> +
> +/*
> + * If *printed is false, print an error message to current monitor if we
> + * have one, else to stderr, and flip *printed to true.

I like function contracts to state the function's purpose in one line if
at all possible.  Perhaps:

    * Like error_report(), except print just once.
    * If *printed is false, print the message, and flip *printed to true.

> + * Returns false if message was not printed, else true.

Uh, confusing negative.  What about

    * Return whether the message was printed.

Happy to make these tweaks in my tree.

> + * Format arguments like sprintf().  The resulting message should be
> + * a single phrase, with no newline or trailing punctuation.
> + * Prepend the current location and append a newline.
> + * It's wrong to call this in a QMP monitor.  Use error_setg() there.
> + */
> +bool error_report_once_cond(bool *printed, const char *fmt, ...)
> +{
> +    va_list ap;
> +
> +    assert(printed);
> +    if (*printed) {
> +        return false;
> +    }
> +    *printed = true;
> +    va_start(ap, fmt);
> +    vreport(REPORT_TYPE_ERROR, fmt, ap);
> +    va_end(ap);
> +    return true;
> +}
> +
> +/*
> + * If *printed is false, print a warning message to current monitor if we
> + * have one, else to stderr, and flip *printed to true.
> + * Returns false if message was not printed, else true.
> + * Format arguments like sprintf().  The resulting message should be
> + * a single phrase, with no newline or trailing punctuation.
> + * Prepend the current location and append a newline.
> + * It's wrong to call this in a QMP monitor.  Use error_setg() there.
> + */
> +bool warn_report_once_cond(bool *printed, const char *fmt, ...)
> +{
> +    va_list ap;
> +
> +    assert(printed);
> +    if (*printed) {
> +        return false;
> +    }
> +    *printed = true;
> +    va_start(ap, fmt);
> +    vreport(REPORT_TYPE_WARNING, fmt, ap);
> +    va_end(ap);
> +    return true;
> +}

Preferably with improved comments:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2 2/2] qemu-error: make use of {error, warn}_report_once_cond

[Markus Armbruster]

Cornelia Huck <cohuck@redhat.com> writes:

> {error,warn}_report_once() are a special case of the new functions
> and can simply switch to them.
>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  include/qemu/error-report.h | 34 ++++++++++++++--------------------
>  1 file changed, 14 insertions(+), 20 deletions(-)
>
> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
> index e415128ac4..918cb936d8 100644
> --- a/include/qemu/error-report.h
> +++ b/include/qemu/error-report.h
> @@ -53,32 +53,26 @@ bool warn_report_once_cond(bool *printed, const char *fmt, ...)
>   * Similar to error_report(), except it prints the message just once.
>   * Return true when it prints, false otherwise.
>   */
> -#define error_report_once(fmt, ...)             \
> -    ({                                          \
> -        static bool print_once_;                \
> -        bool ret_print_once_ = !print_once_;    \
> -                                                \
> -        if (!print_once_) {                     \
> -            print_once_ = true;                 \
> -            error_report(fmt, ##__VA_ARGS__);   \
> -        }                                       \
> -        unlikely(ret_print_once_);              \
> +#define error_report_once(fmt, ...)                     \
> +    ({                                                  \
> +        static bool print_once_;                        \
> +        bool ret_print_once_ =                          \
> +            error_report_once_cond(&print_once_,        \
> +                                   fmt, ##__VA_ARGS__); \
> +        unlikely(ret_print_once_);                      \
>      })

Do we still need @ret_print_once_?

   #define error_report_once(fmt, ...)                     \
       ({                                                  \
           static bool print_once_;                        \
           unlikely(error_report_once_cond(&print_once_,   \
                                   fmt, ##__VA_ARGS__));   \
       })

Or dispense with the unlikely:

   #define error_report_once(fmt, ...)                     \
       ({                                                  \
           static bool print_once_;                        \
           error_report_once_cond(&print_once_,            \
                                  fmt, ##__VA_ARGS__);     \
       })

Got a preference?

>  
>  /*
>   * Similar to warn_report(), except it prints the message just once.
>   * Return true when it prints, false otherwise.
>   */
> -#define warn_report_once(fmt, ...)              \
> -    ({                                          \
> -        static bool print_once_;                \
> -        bool ret_print_once_ = !print_once_;    \
> -                                                \
> -        if (!print_once_) {                     \
> -            print_once_ = true;                 \
> -            warn_report(fmt, ##__VA_ARGS__);    \
> -        }                                       \
> -        unlikely(ret_print_once_);              \
> +#define warn_report_once(fmt, ...)                      \
> +    ({                                                  \
> +        static bool print_once_;                        \
> +        bool ret_print_once_ =                          \
> +            warn_report_once_cond(&print_once_,         \
> +                                  fmt, ##__VA_ARGS__);  \
> +        unlikely(ret_print_once_);                      \
>      })
>  
>  const char *error_get_progname(void);