diff mbox

qemu-log: default to stderr for logging output

Message ID 1360690410-8467-1-git-send-email-peter.maydell@linaro.org
State Superseded
Headers show

Commit Message

Peter Maydell Feb. 12, 2013, 5:33 p.m. UTC
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 <peter.maydell@linaro.org>
---
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.

 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(-)

Comments

Andreas Färber Feb. 12, 2013, 5:50 p.m. UTC | #1
Am 12.02.2013 18:33, schrieb Peter Maydell:
> 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 <peter.maydell@linaro.org>
> ---
> 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.

I don't think this change is okay, it should be limited to the softmmus.
Having random debug output appear on stderr may well break linux-user
test suites or use cases.

Regards,
Andreas
Alexander Graf Feb. 12, 2013, 5:56 p.m. UTC | #2
Am 12.02.2013 um 18:50 schrieb Andreas Färber <afaerber@suse.de>:

> Am 12.02.2013 18:33, schrieb Peter Maydell:
>> 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 <peter.maydell@linaro.org>
>> ---
>> 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.
> 
> I don't think this change is okay, it should be limited to the softmmus.
> Having random debug output appear on stderr may well break linux-user
> test suites or use cases.

But only when you pass in -d, no?

Alex

> 
> Regards,
> Andreas
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Peter Maydell Feb. 12, 2013, 5:58 p.m. UTC | #3
On 12 February 2013 17:50, Andreas Färber <afaerber@suse.de> wrote:
> Am 12.02.2013 18:33, schrieb Peter Maydell:
>> 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.
>
> I don't think this change is okay, it should be limited to the softmmus.
> Having random debug output appear on stderr may well break linux-user
> test suites or use cases.

Debug output will only appear on stderr if you asked for it by
passing -d something. This isn't any different to the softmmu
case (where it is also the case that random debug output might
break things, and also that you won't get it unless you ask).

I agree that it might break weird use cases (where you're running
some existing scripted usecase with debug logging enabled for some
reason); the tradeoff for those people having to tweak their script
to add "-D /tmp/qemu.log" is that we make things better for the
common user who mostly doesn't run with debug but who can now run
with '-d unimp,guest_errors' and get those messages usefully
interspersed with guest output.

-- PMM
Andreas Färber Feb. 12, 2013, 6:16 p.m. UTC | #4
Am 12.02.2013 18:58, schrieb Peter Maydell:
> On 12 February 2013 17:50, Andreas Färber <afaerber@suse.de> wrote:
>> Am 12.02.2013 18:33, schrieb Peter Maydell:
>>> 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.
>>
>> I don't think this change is okay, it should be limited to the softmmus.
>> Having random debug output appear on stderr may well break linux-user
>> test suites or use cases.
> 
> Debug output will only appear on stderr if you asked for it by
> passing -d something. This isn't any different to the softmmu
> case (where it is also the case that random debug output might
> break things, and also that you won't get it unless you ask).
> 
> I agree that it might break weird use cases (where you're running
> some existing scripted usecase with debug logging enabled for some
> reason); the tradeoff for those people having to tweak their script
> to add "-D /tmp/qemu.log" is that we make things better for the
> common user who mostly doesn't run with debug but who can now run
> with '-d unimp,guest_errors' and get those messages usefully
> interspersed with guest output.

I was thinking of default options, so if nothing gets printed by default
that's okay with me.

Andreas
Peter Maydell Feb. 12, 2013, 6:40 p.m. UTC | #5
On 12 February 2013 18:16, Andreas Färber <afaerber@suse.de> wrote:
> Am 12.02.2013 18:58, schrieb Peter Maydell:
>> Debug output will only appear on stderr if you asked for it by
>> passing -d something.

> I was thinking of default options, so if nothing gets printed by default
> that's okay with me.

Yep. If we ever add a -d logcategory which we want to have enabled
by default we'll have to rejig things, because at the moment
there are some users of the API which log things with no category
[relying on the fact that unless the user specifies some -d option
then no logging happens at all and so uncategorised log messages
aren't printed either], and we'd need to move them all to some kind
of "misc rubbish" category so they didn't appear by default. But I
can't actually think of a log category we'd want to have defaulting
to enabled anyway...

-- PMM
Stefan Hajnoczi Feb. 13, 2013, 8:42 a.m. UTC | #6
On Tue, Feb 12, 2013 at 05:33:30PM +0000, Peter Maydell wrote:
> 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 <peter.maydell@linaro.org>
> ---
> 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.
> 
>  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(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Markus Armbruster Feb. 13, 2013, 12:57 p.m. UTC | #7
Peter Maydell <peter.maydell@linaro.org> 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 <peter.maydell@linaro.org>
> ---
> 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
> -@item -d
> -Activate log (logfile=/tmp/qemu.log)
> +@item -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
> -@item -d
> -Activate log (logfile=/tmp/qemu.log)
> +@item -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
> -@item -d
> +@item -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 <armbru@redhat.com>
Peter Maydell Feb. 14, 2013, 6:19 p.m. UTC | #8
On 13 February 2013 12:57, Markus Armbruster <armbru@redhat.com> wrote:
>> -           "-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.

Yeah. I'll standardise the phrasing here bsd-user, linux-user
and system mode all say
           "-d item1[,...]    enable logging of specified items\n"
           "                  (use '-d help' for a list of log items)\n"
           "-D logfile        write logs to 'logfile' (default stderr)\n"

>> @@ -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?

Yes, it does. I'll drop this hunk.

> 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.

I agree, but I think changing the linux-user behaviour to
match bsd-user should be a separate patch.

-- PMM
Markus Armbruster Feb. 18, 2013, 10:46 a.m. UTC | #9
Peter Maydell <peter.maydell@linaro.org> writes:

> On 13 February 2013 12:57, Markus Armbruster <armbru@redhat.com> wrote:
>>> -           "-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.
>
> Yeah. I'll standardise the phrasing here bsd-user, linux-user
> and system mode all say
>            "-d item1[,...]    enable logging of specified items\n"
>            "                  (use '-d help' for a list of log items)\n"
>            "-D logfile        write logs to 'logfile' (default stderr)\n"

Looks good.

>>> @@ -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?
>
> Yes, it does. I'll drop this hunk.
>
>> 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.
>
> I agree, but I think changing the linux-user behaviour to
> match bsd-user should be a separate patch.

Makes sense.
diff mbox

Patch

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"
@@ -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;
 
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,
@@ -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 */
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
-@item -d
-Activate log (logfile=/tmp/qemu.log)
+@item -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
-@item -d
-Activate log (logfile=/tmp/qemu.log)
+@item -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
-@item -d
+@item -d @var{item1}[,...]
 @findex -d
-Output log in /tmp/qemu.log
+Log specified items
 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",
     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