Re: [Qemu-devel] [PATCH] qemu-log: default to stderr for logging output

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

> Switch the default for qemu_log logging output from "/tmp/qemu.log"
> to stderr. This is an incompatible change in some sense, but logging
> is mostly used for debugging purposes so it shouldn't affect production
> use. The previous behaviour can be obtained by adding "-D /tmp/qemu.log"
> to the command line.
>
> This change requires us to:
>  * update all the documentation/help text
>  * make linux-user and bsd-user defer to qemu-log for the default
>    logging destination rather than overriding it themselves
>  * ensure that all logfile closing is done via qemu_log_close()
>    and that that function doesn't close stderr
> as well as the obvious change to the behaviour of do_qemu_set_log()
> when no logfile name has been specified.
>
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> Stefan suggested that qemu_log should default to stderr, and I
> agree that it makes more sense than a random file in /tmp/.
> As noted in the commit message, this is technically an incompatible
> change.
>
> This patchset sits on top of my recent 6 patch qemu_log
> cleanup series.

Related: [PATCH] qemu-log: Remove qemu_log_try_set_file() and its users

>  bsd-user/main.c    |   15 +++++++--------
>  hmp-commands.hx    |    4 ++--
>  include/qemu/log.h |    8 ++++++--
>  linux-user/main.c  |   14 ++++----------
>  qemu-doc.texi      |    8 ++++----
>  qemu-log.c         |   29 +++++++++++------------------
>  qemu-options.hx    |   10 +++++-----
>  tcg/tci/README     |    2 +-
>  8 files changed, 40 insertions(+), 50 deletions(-)
>
> diff --git a/bsd-user/main.c b/bsd-user/main.c
> index 097fbfe..1ec8636 100644
> --- a/bsd-user/main.c
> +++ b/bsd-user/main.c
> @@ -34,8 +34,6 @@
>  #include "qemu/timer.h"
>  #include "qemu/envlist.h"
>  
> -#define DEBUG_LOGFILE "/tmp/qemu.log"
> -
>  int singlestep;
>  #if defined(CONFIG_USE_GUEST_BASE)
>  unsigned long mmap_min_addr;
> @@ -691,8 +689,8 @@ static void usage(void)
>             "-bsd type         select emulated BSD type 
> FreeBSD/NetBSD/OpenBSD (default)\n"
>             "\n"
>             "Debug options:\n"
> -           "-d options   activate log (default logfile=%s)\n"
> -           "-D logfile   override default logfile location\n"
> +           "-d options   activate log (default is to log to stderr)\n"
> +           "-D logfile   write logs to 'logfile' rather than stderr\n"
>             "-p pagesize  set the host page size to 'pagesize'\n"
>             "-singlestep  always run in singlestep mode\n"
>             "-strace      log system calls\n"

No need to mention the default twice.

Pointing to -d help would be nice.

> @@ -709,8 +707,7 @@ static void usage(void)
>             ,
>             TARGET_ARCH,
>             interp_prefix,
> -           x86_stack_size,
> -           DEBUG_LOGFILE);
> +           x86_stack_size);
>      exit(1);
>  }
>  
> @@ -733,7 +730,7 @@ int main(int argc, char **argv)
>  {
>      const char *filename;
>      const char *cpu_model;
> -    const char *log_file = DEBUG_LOGFILE;
> +    const char *log_file = NULL;
>      const char *log_mask = NULL;
>      struct target_pt_regs regs1, *regs = &regs1;
>      struct image_info info1, *info = &info1;
> @@ -861,7 +858,9 @@ int main(int argc, char **argv)
>      }
>  
>      /* init debug */
> -    qemu_set_log_filename(log_file);
> +    if (log_file) {
> +        qemu_set_log_filename(log_file);
> +    }
>      if (log_mask) {
>          int mask;
>  

Doesn't qemu_set_log_filename(NULL) do the right thing?

> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 64008a9..cef7708 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -295,14 +295,14 @@ ETEXI
>          .name       = "log",
>          .args_type  = "items:s",
>          .params     = "item1[,...]",
> -        .help       = "activate logging of the specified items to 
> '/tmp/qemu.log'",
> +        .help       = "activate logging of the specified items",
>          .mhandler.cmd = do_log,
>      },
>  
>  STEXI
>  @item log @var{item1}[,...]
>  @findex log
> -Activate logging of the specified items to @file{/tmp/qemu.log}.
> +Activate logging of the specified items.
>  ETEXI
>  
>      {
> diff --git a/include/qemu/log.h b/include/qemu/log.h
> index 4527003..6b0db02 100644
> --- a/include/qemu/log.h
> +++ b/include/qemu/log.h
> @@ -116,8 +116,12 @@ static inline void qemu_log_flush(void)
>  /* Close the log file */
>  static inline void qemu_log_close(void)
>  {
> -    fclose(qemu_logfile);
> -    qemu_logfile = NULL;
> +    if (qemu_logfile) {
> +        if (qemu_logfile != stderr) {
> +            fclose(qemu_logfile);
> +        }
> +        qemu_logfile = NULL;
> +    }
>  }
>  
>  /* Set up a new log file */
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 8a61ea4..55e8326 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -35,8 +35,6 @@
>  #include "qemu/envlist.h"
>  #include "elf.h"
>  
> -#define DEBUG_LOGFILE "/tmp/qemu.log"
> -
>  char *exec_path;
>  
>  int singlestep;
> @@ -3289,9 +3287,9 @@ static const struct qemu_argument arg_table[] = {
>       "size",       "reserve 'size' bytes for guest virtual address space"},
>  #endif
>      {"d",          "QEMU_LOG",         true,  handle_arg_log,
> -     "options",    "activate log"},
> +     "options",    "activate log (default is to log to stderr)"},
>      {"D",          "QEMU_LOG_FILENAME", true, handle_arg_log_filename,
> -     "logfile",     "override default logfile location"},
> +     "logfile",     "log to specified file rather than stderr"},
>      {"p",          "QEMU_PAGESIZE",    true,  handle_arg_pagesize,
>       "pagesize",   "set the host page size to 'pagesize'"},
>      {"singlestep", "QEMU_SINGLESTEP",  false, handle_arg_singlestep,

No need to mention the default twice.

Pointing to -d help would be nice.

> @@ -3344,11 +3342,9 @@ static void usage(void)
>      printf("\n"
>             "Defaults:\n"
>             "QEMU_LD_PREFIX  = %s\n"
> -           "QEMU_STACK_SIZE = %ld byte\n"
> -           "QEMU_LOG        = %s\n",
> +           "QEMU_STACK_SIZE = %ld byte\n",
>             interp_prefix,
> -           guest_stack_size,
> -           DEBUG_LOGFILE);
> +           guest_stack_size);
>  
>      printf("\n"
>             "You can use -E and -U options or the QEMU_SET_ENV and\n"
> @@ -3432,7 +3428,6 @@ static int parse_args(int argc, char **argv)
>  
>  int main(int argc, char **argv, char **envp)
>  {
> -    const char *log_file = DEBUG_LOGFILE;
>      struct target_pt_regs regs1, *regs = &regs1;
>      struct image_info info1, *info = &info1;
>      struct linux_binprm bprm;
> @@ -3476,7 +3471,6 @@ int main(int argc, char **argv, char **envp)
>  #endif
>  
>      /* init debug */
> -    qemu_set_log_filename(log_file);
>      optind = parse_args(argc, argv);
>  
>      /* Zero out regs */

bsd-user: if there's more than one -D, all but the last get silently
ignored.  We create that log file when logging is enabled.

linux-user: we create all the log files when logging is enabled.

No biggie, but consistency would be nice.  I prefer bsd-user's behavior.

> diff --git a/qemu-doc.texi b/qemu-doc.texi
> index 6d7f50d..747e052 100644
> --- a/qemu-doc.texi
> +++ b/qemu-doc.texi
> @@ -2642,8 +2642,8 @@ Pre-allocate a guest virtual address space of the given 
> size (in bytes).
>  Debug options:
>  
>  @table @option
> address@hidden -d
> -Activate log (logfile=/tmp/qemu.log)
> address@hidden -d item1,...
> +Activate logging of the specified items (use '-d help' for a list of log 
> items)
>  @item -p pagesize
>  Act as if the host page size was 'pagesize' bytes
>  @item -g port
> @@ -2781,8 +2781,8 @@ FreeBSD, NetBSD and OpenBSD (default).
>  Debug options:
>  
>  @table @option
> address@hidden -d
> -Activate log (logfile=/tmp/qemu.log)
> address@hidden -d item1,...
> +Activate logging of the specified items (use '-d help' for a list of log 
> items)
>  @item -p pagesize
>  Act as if the host page size was 'pagesize' bytes
>  @item -singlestep
> diff --git a/qemu-log.c b/qemu-log.c
> index 2f47aaf..797f2af 100644
> --- a/qemu-log.c
> +++ b/qemu-log.c
> @@ -20,12 +20,6 @@
>  #include "qemu-common.h"
>  #include "qemu/log.h"
>  
> -#ifdef WIN32
> -#define DEFAULT_LOGFILENAME "qemu.log"
> -#else
> -#define DEFAULT_LOGFILENAME "/tmp/qemu.log"
> -#endif
> -
>  static char *logfilename;
>  FILE *qemu_logfile;
>  int qemu_loglevel;
> @@ -56,14 +50,17 @@ void qemu_log_mask(int mask, const char *fmt, ...)
>  /* enable or disable low levels log */
>  void do_qemu_set_log(int log_flags, bool use_own_buffers)
>  {
> -    const char *fname = logfilename ?: DEFAULT_LOGFILENAME;
> -
>      qemu_loglevel = log_flags;
>      if (qemu_loglevel && !qemu_logfile) {
> -        qemu_logfile = fopen(fname, log_append ? "a" : "w");
> -        if (!qemu_logfile) {
> -            perror(fname);
> -            _exit(1);
> +        if (logfilename) {
> +            qemu_logfile = fopen(logfilename, log_append ? "a" : "w");
> +            if (!qemu_logfile) {
> +                perror(logfilename);
> +                _exit(1);
> +            }
> +        } else {
> +            /* Default to stderr if no log file specified */
> +            qemu_logfile = stderr;
>          }
>          /* must avoid mmap() usage of glibc by setting a buffer "by hand" */
>          if (use_own_buffers) {
> @@ -81,8 +78,7 @@ void do_qemu_set_log(int log_flags, bool use_own_buffers)
>          }
>      }
>      if (!qemu_loglevel && qemu_logfile) {
> -        fclose(qemu_logfile);
> -        qemu_logfile = NULL;
> +        qemu_log_close();
>      }
>  }
>  
> @@ -90,10 +86,7 @@ void qemu_set_log_filename(const char *filename)
>  {
>      g_free(logfilename);
>      logfilename = g_strdup(filename);
> -    if (qemu_logfile) {
> -        fclose(qemu_logfile);
> -        qemu_logfile = NULL;
> -    }
> +    qemu_log_close();
>      qemu_set_log(qemu_loglevel);
>  }
>  
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 046bdc0..d957565 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2489,21 +2489,21 @@ Shorthand for -gdb tcp::1234, i.e. open a gdbserver 
> on TCP port 1234
>  ETEXI
>  
>  DEF("d", HAS_ARG, QEMU_OPTION_d, \
> -    "-d item1,...    output log to /tmp/qemu.log (use '-d help' for a list 
> of log items)\n",
> +    "-d item1,...    enable logging of specified items (use '-d help' for a 
> list of log items)\n",
>      QEMU_ARCH_ALL)
>  STEXI
> address@hidden -d
> address@hidden -d @var{item1}[,...]
>  @findex -d
> -Output log in /tmp/qemu.log
> +Log specified items

Pointing to -d help would be nice.

>  ETEXI
>  
>  DEF("D", HAS_ARG, QEMU_OPTION_D, \
> -    "-D logfile      output log to logfile (instead of the default 
> /tmp/qemu.log)\n",
> +    "-D logfile      output log to logfile (instead of the default 
> stderr)\n",

    "-D logfile      output log to logfile (default stderr)\n",

would be more consistent with the rest of the help text.

>      QEMU_ARCH_ALL)
>  STEXI
>  @item -D @var{logfile}
>  @findex -D
> -Output log in @var{logfile} instead of /tmp/qemu.log
> +Output log in @var{logfile} instead of to stderr
>  ETEXI
>  
>  DEF("hdachs", HAS_ARG, QEMU_OPTION_hdachs, \
> diff --git a/tcg/tci/README b/tcg/tci/README
> index 6ac1ac9..dc57f07 100644
> --- a/tcg/tci/README
> +++ b/tcg/tci/README
> @@ -52,7 +52,7 @@ The only difference from running QEMU with TCI to running 
> without TCI
>  should be speed. Especially during development of TCI, it was very
>  useful to compare runs with and without TCI. Create /tmp/qemu.log by
>  
> -        qemu-system-i386 -d in_asm,op_opt,cpu -singlestep
> +        qemu-system-i386 -d in_asm,op_opt,cpu -D /tmp/qemu.log -singlestep
>  
>  once with interpreter and once without interpreter and compare the resulting
>  qemu.log files. This is also useful to see the effects of additional

Possible future work: reconsider the liberal use of inline in log.h.

Extra points for tracking down obscure occurences of -d and
/tmp/qemu.log.

Since all my comments can be addressed on top:

Reviewed-by: Markus Armbruster <address@hidden>