Re: [PATCH 2/4] linux-user: Use `qemu_log' for strace

[Laurent Vivier]

Le 14/01/2020 à 04:01, Josh Kunz a écrit :
> This change switches linux-user strace logging to use the newer `qemu_log`
> logging subsystem rather than the older `gemu_log` (notice the "g")
> logger. `qemu_log` has several advantages, namely that it allows logging
> to a file, and provides a more unified interface for configuration
> of logging (via the QEMU_LOG environment variable or options).
> 
> Signed-off-by: Josh Kunz <address@hidden>
> ---
>  include/qemu/log.h   |  13 ++
>  linux-user/main.c    |  17 +-
>  linux-user/qemu.h    |   1 -
>  linux-user/signal.c  |   3 +-
>  linux-user/strace.c  | 479 ++++++++++++++++++++++---------------------
>  linux-user/syscall.c |  13 +-
>  util/log.c           |   2 +
>  7 files changed, 282 insertions(+), 246 deletions(-)
> 
> diff --git a/include/qemu/log.h b/include/qemu/log.h
> index 503e4f88d5..8f044c1716 100644
> --- a/include/qemu/log.h
> +++ b/include/qemu/log.h
> @@ -64,6 +64,7 @@ static inline bool qemu_log_separate(void)
>  #define CPU_LOG_PLUGIN     (1 << 18)
>  /* LOG_USER is used for some informational user-mode logging. */
>  #define LOG_USER           (1 << 19)
> +#define LOG_STRACE         (1 << 20)

Perhaps we can use flag LOG_TRACE? (cc Paolo)

>  
>  /* Lock output for a series of related logs.  Since this is not needed
>   * for a single qemu_log / qemu_log_mask / qemu_log_mask_and_addr, we
> @@ -154,6 +155,18 @@ void qemu_set_dfilter_ranges(const char *ranges, Error 
> **errp);
>  bool qemu_log_in_addr_range(uint64_t addr);
>  int qemu_str_to_log_mask(const char *str);
>  
> +/* Add (union) the given "log_flags" to the current log mask. */
> +static inline void qemu_add_log(int log_flags)
> +{
> +    qemu_set_log(qemu_loglevel | log_flags);
> +}

This is really a special case as the flags are all given at the same
time, could you use directly qemu_set_log() in main()?

> +/* Remove (subtract) the given log flags from the current log mask. */
> +static inline void qemu_del_log(int log_flags)
> +{
> +    qemu_set_log(qemu_loglevel & ~(log_flags));
> +}

Unused and unneeded.

Except that, the patch seems good.

Thanks,
Laurent