Re: [Qemu-devel] [PATCH 1/3] log: improve performance of qemu_log and qemu_log_mask if disabled

[Markus Armbruster]

"Denis V. Lunev" <address@hidden> writes:

> The patch is intended to avoid to perform any operation including
> calculation of log function arguments when the log is not enabled due to
> various reasons.
>
> Functions qemu_log and qemu_log_mask are replaced with variadic macros.
> Unfortunately the code is not C99 compatible and we can not use
> portable __VA_ARGS__ way. There are a lot of warnings in the other
> places with --std=c99. Thus the only way to achive the result is to use
> args.. GCC extension.

Really?  We use __VA_ARGS__ all over the place, why won't it work here?

> Format checking performed by compiler will not suffer by this patch. It
> will be done inside in fprintf arguments checking.
>
> Signed-off-by: Denis V. Lunev <address@hidden>
> Signed-off-by: Pavel Butsykin <address@hidden>
> CC: Markus Armbruster <address@hidden>
> CC: Luiz Capitulino <address@hidden>
> CC: Eric Blake <address@hidden>
> CC: Peter Maydell <address@hidden>
> ---
>  include/qemu/log.h | 17 ++++++++++++++---
>  qemu-log.c         | 21 ---------------------
>  2 files changed, 14 insertions(+), 24 deletions(-)
>
> diff --git a/include/qemu/log.h b/include/qemu/log.h
> index f880e66..57b8c96 100644
> --- a/include/qemu/log.h
> +++ b/include/qemu/log.h
> @@ -53,7 +53,13 @@ static inline bool qemu_loglevel_mask(int mask)
>  
>  /* main logging function
>   */
> -void GCC_FMT_ATTR(1, 2) qemu_log(const char *fmt, ...);
> +#define qemu_log(args...)                   \
> +    do {                                    \
> +        if (!qemu_log_enabled()) {          \
> +            break;                          \
> +        }                                   \
> +        fprintf(qemu_logfile, args);        \
> +    } while (0)

Feels stilted.  Like Alex's, I'd prefer something like

    #define qemu_log(fmt, ...)                          \
        do {                                                \
            if (unlikely(qemu_log_enabled())) {         \
                fprintf(qemu_logfile, fmt, ## __VA_ARGS__);     \
            }                                           \
        } while (0)

I'm no fan of hiding qemu_logfile in qemu_log_enabled(), then using it
directly to print to it, but that's a different conversation.

However, we already have

    static inline void GCC_FMT_ATTR(1, 0)
    qemu_log_vprintf(const char *fmt, va_list va)
    {
        if (qemu_logfile) {
            vfprintf(qemu_logfile, fmt, va);
        }
    }

Wouldn't static inline work for qemu_log(), too?

>  /* vfprintf-like logging function
>   */
> @@ -67,8 +73,13 @@ qemu_log_vprintf(const char *fmt, va_list va)
>  
>  /* log only if a bit is set on the current loglevel mask
>   */
> -void GCC_FMT_ATTR(2, 3) qemu_log_mask(int mask, const char *fmt, ...);
> -
> +#define qemu_log_mask(mask, args...)        \
> +    do {                                    \
> +        if (!qemu_loglevel_mask(mask)) {    \
> +            break;                          \
> +        }                                   \
> +        qemu_log(args);                     \
> +    } while (0)
>  
>  /* Special cases: */
>  
> diff --git a/qemu-log.c b/qemu-log.c
> index 13f3813..e6d2b3f 100644
> --- a/qemu-log.c
> +++ b/qemu-log.c
> @@ -25,27 +25,6 @@ FILE *qemu_logfile;
>  int qemu_loglevel;
>  static int log_append = 0;
>  
> -void qemu_log(const char *fmt, ...)
> -{
> -    va_list ap;
> -
> -    va_start(ap, fmt);
> -    if (qemu_logfile) {
> -        vfprintf(qemu_logfile, fmt, ap);
> -    }
> -    va_end(ap);
> -}
> -
> -void qemu_log_mask(int mask, const char *fmt, ...)
> -{
> -    va_list ap;
> -
> -    va_start(ap, fmt);
> -    if ((qemu_loglevel & mask) && qemu_logfile) {
> -        vfprintf(qemu_logfile, fmt, ap);
> -    }
> -    va_end(ap);
> -}
>  
>  /* enable or disable low levels log */
>  void do_qemu_set_log(int log_flags, bool use_own_buffers)

I support the general approach to inline the "is logging on" test
somehow.