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

["Alex Bennée" <alex.bennee@linaro.org>]

Josh Kunz <jkz@google.com> writes:

> 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 <jkz@google.com>
> ---
>  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)
>
>  /* 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);
> +}
> +
> +/* 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));
> +}
> +
>  /* Print a usage message listing all the valid logging categories
>   * to the specified FILE*.
>   */
> diff --git a/linux-user/main.c b/linux-user/main.c
> index c4f3de77db..0bf40c4d27 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -63,6 +63,12 @@ int have_guest_base;
>  /* Used to implement backwards-compatibility for user-mode logging. */
>  static bool force_user_mode_logging = true;
>
> +/*
> + * Used to implement backwards-compatibility for the `-strace`, and
> + * QEMU_STRACE options.
> + */
> +static bool enable_strace;
> +
>  /*
>   * When running 32-on-64 we should make sure we can fit all of the possible
>   * guest address space into a contiguous chunk of virtual host memory.
> @@ -378,7 +384,7 @@ static void handle_arg_singlestep(const char *arg)
>
>  static void handle_arg_strace(const char *arg)
>  {
> -    do_strace = 1;
> +    enable_strace = true;

Could we cut out the middle-man and just qemu_add_log(LOG_STRACE) here
and drop the enable_strace variable.

>  }
>
>  static void handle_arg_version(const char *arg)
> @@ -672,6 +678,15 @@ int main(int argc, char **argv, char **envp)
>      qemu_plugin_add_opts();
>
>      optind = parse_args(argc, argv);
> +    /*
> +     * Backwards Compatability: If handle_arg_strace just enabled strace
> +     * logging directly, then it could be accidentally turned off by a
> +     * QEMU_LOG/-d option. To make sure that strace logging is always enabled
> +     * when QEMU_STRACE/-strace is set, re-enable LOG_STRACE here.
> +     */
> +    if (enable_strace) {
> +        qemu_add_log(LOG_STRACE);
> +    }
>
>      if (force_user_mode_logging) {
>          /*
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index f6f5fe5fbb..02c6890c8a 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -385,7 +385,6 @@ void print_syscall_ret(int num, abi_long arg1);
>   * --- SIGSEGV {si_signo=SIGSEGV, si_code=SI_KERNEL, si_addr=0} ---
>   */
>  void print_taken_signal(int target_signum, const target_siginfo_t *tinfo);
> -extern int do_strace;
>
>  /* signal.c */
>  void process_pending_signals(CPUArchState *cpu_env);
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 5ca6d62b15..2ff0065804 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -864,9 +864,8 @@ static void handle_pending_signal(CPUArchState *cpu_env, int sig,
>          handler = sa->_sa_handler;
>      }
>
> -    if (do_strace) {
> +    if (unlikely(qemu_loglevel_mask(LOG_STRACE)))
>          print_taken_signal(sig, &k->info);
> -    }

Please don't drop the brace - c.f. CODING_STYLE.rst

Side note, print_taken_signal might want to consider using the
qemu_log_lock() functionality to keep sequential qemu_log's together. I
suspect you might want to do the same for syscalls.

--
Alex Bennée