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

[Peter Xu]

On Wed, Jun 13, 2018 at 10:01:22AM +0200, Markus Armbruster wrote:
> Peter Xu <address@hidden> 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
> 
> default
> 
> > 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 <address@hidden>
> > CC: Markus Armbruster <address@hidden>
> > Signed-off-by: Peter Xu <address@hidden>
> > ---
> >  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.
> 
> I like to start function contracts with a single line stating the
> function's purpose, and I prefer imperative mood, like this:
> 
>     * Similar to error_report(), but it only prints the message 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_);             \
> > +    })
> 
> Please align the backslashes, say with emacs command c-backslash-region,
> bound to C-c C-\.

I am with evil mode so mostly I'm using evil-indent.  It's strange why
the patches were not indented correctly.  Now indent will be fine
locally if I redo the evil-indent.  I must have done something wrong
before. :(

> 
> > +
> > +/*
> > + * 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_);             \
> > +    })
> 
> Likewise.
> 
> > +
> >  const char *error_get_progname(void);
> >  extern bool enable_timestamp_msg;
> 
> With these nits addressed:
> Reviewed-by: Markus Armbruster <address@hidden>
> 
> I can touch them up when I apply.

Thanks, Markus.

-- 
Peter Xu