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

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

Josh Kunz <jkz@google.com> writes:

> This change introduces a new logging mask "LOG_USER", which is used for
> masking general user-mode logging. This change also switches all non-strace
> uses of `gemu_log' in linux-user/ to use `qemu_log_mask(LOG_USER, ...)'
> instead. This allows the user to easily log to a file, and to mask out
> these log messages if they desire.
>
> Signed-off-by: Josh Kunz <jkz@google.com>
> ---
>  include/qemu/log.h        |  2 ++
>  linux-user/arm/cpu_loop.c |  5 ++--
>  linux-user/fd-trans.c     | 55 +++++++++++++++++++++++++--------------
>  linux-user/main.c         | 24 +++++++++++++++++
>  linux-user/syscall.c      | 30 ++++++++++++---------
>  linux-user/vm86.c         |  3 ++-
>  util/log.c                |  3 +++
>  7 files changed, 86 insertions(+), 36 deletions(-)
>
> diff --git a/include/qemu/log.h b/include/qemu/log.h
> index e0f4e40628..503e4f88d5 100644
> --- a/include/qemu/log.h
> +++ b/include/qemu/log.h
> @@ -62,6 +62,8 @@ static inline bool qemu_log_separate(void)
>  #define CPU_LOG_TB_OP_IND  (1 << 16)
>  #define CPU_LOG_TB_FPU     (1 << 17)
>  #define CPU_LOG_PLUGIN     (1 << 18)
> +/* LOG_USER is used for some informational user-mode logging. */
> +#define LOG_USER           (1 << 19)

As Laurent said I think LOG_UNIMP is perfectly fine for stuff we haven't
done. I don't think any of the cases warrant LOG_GUEST_ERROR.
>  
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 8718d03ee2..c4f3de77db 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -60,6 +60,9 @@ unsigned long mmap_min_addr;
>  unsigned long guest_base;
>  int have_guest_base;
>  
> +/* Used to implement backwards-compatibility for user-mode logging. */
> +static bool force_user_mode_logging = true;
> +

I'm not sure want to bother with this. I know we like to avoid
regression but isn't this all debug log stuff? If we must keep it can we
invert the variable to save the initialisation.

>  /*
>   * 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.
> @@ -399,6 +402,11 @@ static void handle_arg_abi_call0(const char *arg)
>  }
>  #endif
>  
> +static void handle_arg_no_force_user_mode_logging(const char *arg)
> +{
> +    force_user_mode_logging = false;
> +}
> +
>  static QemuPluginList plugins = QTAILQ_HEAD_INITIALIZER(plugins);
>  
>  #ifdef CONFIG_PLUGIN
> @@ -469,6 +477,10 @@ static const struct qemu_argument arg_table[] = {
>      {"xtensa-abi-call0", "QEMU_XTENSA_ABI_CALL0", false, handle_arg_abi_call0,
>       "",           "assume CALL0 Xtensa ABI"},
>  #endif
> +    {"no-force-user-mode-logging", "", false,
> +      handle_arg_no_force_user_mode_logging,
> +      "",          "disable forced user-mode logging, other logging options "
> +                   "will be used exactly as provided" },
>      {NULL, NULL, false, NULL, NULL, NULL}
>  };
>  
> @@ -661,6 +673,18 @@ int main(int argc, char **argv, char **envp)
>  
>      optind = parse_args(argc, argv);
>  
> +    if (force_user_mode_logging) {
> +        /*
> +         * Backwards Compatibility: gemu_log for non-strace messages was not
> +         * configurable, and was always on. Unless the user explicitly disables
> +         * forced LOG_USER, force LOG_USER into the mask.
> +         */
> +        qemu_add_log(LOG_USER);
> +    }
> +
> +    qemu_log_mask(LOG_USER, "--> from user\n");
> +    qemu_log_mask(LOG_STRACE, "--> from strace\n");
> +

I mean we jumped through hoops to maintain backwards compatibility and
then added new output? Also LOG_STRACE doesn't exist yet.

>      if (!trace_init_backends()) {
>          exit(1);
>      }
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 171c0caef3..7e23dd6327 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1555,7 +1555,7 @@ static inline abi_long target_to_host_cmsg(struct msghdr *msgh,
>               * something more intelligent than "twice the size of the
>               * target buffer we're reading from".
>               */
> -            gemu_log("Host cmsg overflow\n");
> +            qemu_log_mask(LOG_USER, "Host cmsg overflow\n");

I'm not sure we shouldn't just be asserting this case above. The
comments imply it is a bug on our part. The rest look like good cases
for LOG_UNIMP.

-- 
Alex Bennée