Peter Maydell <peter.mayd...@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.mayd...@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 = ®s1;
> 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 = ®s1;
> 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 <arm...@redhat.com>
