Re: [Qemu-devel] [PATCH v5 1/3] qemu-error: introduce {error|warn}_report_once

[Markus Armbruster <armbru@redhat.com>]

Peter Xu <peterx@redhat.com> writes:

> There are many error_report()s that can be used in frequently called
> functions, especially on IO paths.  That can be unideal in that
> malicious guest can try to trigger the error tons of time which might
> use up the log space on the host (e.g., libvirt can capture the stderr
> of QEMU and put it persistently onto disk).  In VT-d emulation code, we
> have trace_vtd_error() tracer.  AFAIU all those places can be replaced
> by something like error_report() but trace points are mostly used to
> avoid the DDOS attack that mentioned above.  However using trace points
> mean that errors are not dumped if trace not enabled.
>
> It's not a big deal in most modern server managements since we have
> things like logrotate to maintain the logs and make sure the quota is
> expected.  However it'll still be nice that we just provide another way
> to restrict message generations.  In most cases, this kind of
> error_report()s will only provide valid information on the first message
> sent, and all the rest of similar messages will be mostly talking about
> the same thing.  This patch introduces *_report_once() helpers to allow
> a message to be dumped only once during one QEMU process's life cycle.
> It will make sure: (1) it's on by deffault, so we can even get something
> without turning the trace on and reproducing, and (2) it won't be
> affected by DDOS attack.
>
> To implement it, I stole the printk_once() macro from Linux.
>
> CC: Eric Blake <eblake@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/qemu/error-report.h | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
> index e1c8ae1a52..c7ec54cb97 100644
> --- a/include/qemu/error-report.h
> +++ b/include/qemu/error-report.h
> @@ -44,6 +44,38 @@ 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);
>  
> +/*
> + * Similar to error_report(), but it only prints the message once.  It
> + * returns true when it prints the first time, otherwise false.
> + */
> +#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_);             \
> +    })
> +
> +/*
> + * Similar to warn_report(), but it only prints the message once.  It
> + * returns true when it prints the first time, otherwise false.
> + */
> +#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_);             \
> +    })
> +
>  const char *error_get_progname(void);
>  extern bool enable_timestamp_msg;

With the touch-ups from my review of v4 squashed in (fixup patch
appended):
Reviewed-by: Markus Armbruster <armbru@redhat.com>


>From f57a2317c75bea5396a589b2d65933312850bb05 Mon Sep 17 00:00:00 2001
From: Markus Armbruster <armbru@redhat.com>
Date: Wed, 15 Aug 2018 13:29:03 +0200
Subject: [PATCH] fixup! intel-iommu: start to use error_report_once

[Whitespace adjusted, comments improved]
---
 include/qemu/error-report.h | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index c7ec54cb97..72fab2b031 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -45,35 +45,35 @@ void warn_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 void info_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 
 /*
- * Similar to error_report(), but it only prints the message once.  It
- * returns true when it prints the first time, otherwise false.
+ * 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_;  \
+        static bool print_once_;                \
+        bool ret_print_once_ = !print_once_;    \
                                                 \
-        if (!print_once_) {                    \
-            print_once_ = true;                \
+        if (!print_once_) {                     \
+            print_once_ = true;                 \
             error_report(fmt, ##__VA_ARGS__);   \
         }                                       \
-        unlikely(ret_print_once_);             \
+        unlikely(ret_print_once_);              \
     })
 
 /*
- * Similar to warn_report(), but it only prints the message once.  It
- * returns true when it prints the first time, otherwise false.
+ * 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_;  \
+        static bool print_once_;                \
+        bool ret_print_once_ = !print_once_;    \
                                                 \
-        if (!print_once_) {                    \
-            print_once_ = true;                \
-            warn_report(fmt, ##__VA_ARGS__);   \
+        if (!print_once_) {                     \
+            print_once_ = true;                 \
+            warn_report(fmt, ##__VA_ARGS__);    \
         }                                       \
-        unlikely(ret_print_once_);             \
+        unlikely(ret_print_once_);              \
     })
 
 const char *error_get_progname(void);
-- 
2.17.1