Re: [Qemu-devel] [Tracing][PATCH] Add options to specify trace file name at startup and runtime.

[Stefan Hajnoczi]

On Tue, Aug 3, 2010 at 6:37 AM, Prerna Saxena <address@hidden> wrote:
> This patch adds an optional command line switch '-trace' to specify the
> filename to write traces to, when qemu starts.
> Eg, If compiled with the 'simple' trace backend,
> address@hidden qemu -trace FILENAME IMAGE
> Allows the binary traces to be written to FILENAME instead of the option
> set at config-time.
>
> Also, this adds monitor sub-command 'set' to trace-file commands to
> dynamically change trace log file at runtime.
> Eg,
> (qemu)trace-file set FILENAME
> This allows one to set trace outputs to FILENAME from the default
> specified at startup.
>
> Signed-off-by: Prerna Saxena <address@hidden>
> ---
>  monitor.c       |    6 ++++++
>  qemu-monitor.hx |    6 +++---
>  qemu-options.hx |   11 +++++++++++
>  simpletrace.c   |   41 ++++++++++++++++++++++++++++++++---------
>  tracetool       |    1 +
>  vl.c            |   22 ++++++++++++++++++++++
>  6 files changed, 75 insertions(+), 12 deletions(-)

Looks like a good approach.  I checked that this also handles the case
where trace events fire before the command-line option is handled and
the trace filename is set.

> diff --git a/monitor.c b/monitor.c
> index 1e35a6b..8e2a3a6 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -544,6 +544,7 @@ static void do_change_trace_event_state(Monitor *mon, 
> const QDict *qdict)
>  static void do_trace_file(Monitor *mon, const QDict *qdict)
>  {
>     const char *op = qdict_get_try_str(qdict, "op");
> +    const char *arg = qdict_get_try_str(qdict, "arg");
>
>     if (!op) {
>         st_print_trace_file_status((FILE *)mon, &monitor_fprintf);
> @@ -553,8 +554,13 @@ static void do_trace_file(Monitor *mon, const QDict 
> *qdict)
>         st_set_trace_file_enabled(false);
>     } else if (!strcmp(op, "flush")) {
>         st_flush_trace_buffer();
> +    } else if (!strcmp(op, "set")) {
> +        if (arg) {
> +            st_set_trace_file(arg);
> +        }
>     } else {
>         monitor_printf(mon, "unexpected argument \"%s\"\n", op);
> +        monitor_printf(mon, "Options are: [on | off| flush| set FILENAME]");

Can we use help_cmd() here to print the help text and avoid
duplicating the options?

>     }
>  }
>  #endif
> diff --git a/qemu-monitor.hx b/qemu-monitor.hx
> index 25887bd..adfaf2b 100644
> --- a/qemu-monitor.hx
> +++ b/qemu-monitor.hx
> @@ -276,9 +276,9 @@ ETEXI
>
>     {
>         .name       = "trace-file",
> -        .args_type  = "op:s?",
> -        .params     = "op [on|off|flush]",
> -        .help       = "open, close, or flush trace file",
> +        .args_type  = "op:s?,arg:F?",
> +        .params     = "on|off|flush|set [arg]",
> +        .help       = "open, close, or flush trace file, or set a new file 
> name",
>         .mhandler.cmd = do_trace_file,
>     },
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index d1d2272..aea9675 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2223,6 +2223,17 @@ Normally QEMU loads a configuration file from 
> @var{sysconfdir}/qemu.conf and
> address@hidden/address@hidden on startup.  The @code{-nodefconfig}
>  option will prevent QEMU from loading these configuration files at startup.
>  ETEXI
> +#ifdef CONFIG_SIMPLE_TRACE
> +DEF("trace", HAS_ARG, QEMU_OPTION_trace,
> +    "-trace\n"
> +    "                Specify a trace file to log traces to\n",
> +    QEMU_ARCH_ALL)
> +STEXI
> address@hidden -trace
> address@hidden -trace
> +Specify a trace file to log output traces to.
> +ETEXI
> +#endif
>
>  HXCOMM This is the last statement. Insert new options before this line!
>  STEXI
> diff --git a/simpletrace.c b/simpletrace.c
> index 71110b3..5812fe9 100644
> --- a/simpletrace.c
> +++ b/simpletrace.c
> @@ -20,25 +20,48 @@ enum {
>  static TraceRecord trace_buf[TRACE_BUF_LEN];
>  static unsigned int trace_idx;
>  static FILE *trace_fp;
> -static bool trace_file_enabled = true;
> +static char *trace_file_name = NULL;
> +static bool trace_file_enabled = false;
>
>  void st_print_trace_file_status(FILE *stream, int (*stream_printf)(FILE 
> *stream, const char *fmt, ...))
>  {
> -    stream_printf(stream, "Trace file \"" CONFIG_TRACE_FILE "\" %s.\n",
> -                  getpid(), trace_file_enabled ? "on" : "off");
> +    stream_printf(stream, "Trace file \"%s\" %s.\n",
> +                  trace_file_name, trace_file_enabled ? "on" : "off");
>  }
>
>  static bool open_trace_file(void)
>  {
> -    char *filename;
> +    trace_fp = fopen(trace_file_name, "w");
> +    return trace_fp != NULL;
> +}

This could be inlined now.  The function is only used by one caller.

>
> -    if (asprintf(&filename, CONFIG_TRACE_FILE, getpid()) < 0) {
> -        return false;
> +/**
> + * set_trace_file : To set the name of a trace file.
> + * @file : pointer to the name to be set.
> + *         If NULL, set to the default name-<pid> set at config time.
> + */
> +bool st_set_trace_file(const char *file)
> +{
> +    if (trace_file_enabled) {
> +        st_set_trace_file_enabled(false);
>     }

No need for an if statement.  If trace_file_enabled is already false,
then st_set_trace_file_enabled() is a nop.

>
> -    trace_fp = fopen(filename, "w");
> -    free(filename);
> -    return trace_fp != NULL;
> +    if (trace_file_name) {
> +        free(trace_file_name);
> +    }

No need for an if statement.  free(NULL) is a nop.

> +
> +    if (!file) {
> +        if (asprintf(&trace_file_name, CONFIG_TRACE_FILE, getpid()) < 0) {
> +           return false;
> +        }
> +    } else {
> +        if (asprintf(&trace_file_name, "%s", file) < 0) {
> +            return false;
> +        }
> +    }

When asprintf() fails, the value of the string pointer is undefined
according to the man page.  That can result in double frees.  It would
be safest to set trace_file_name = NULL on failure.

> +
> +    st_set_trace_file_enabled(true);
> +    return true;
>  }
>
>  static void flush_trace_file(void)
> diff --git a/tracetool b/tracetool
> index ac832af..5b979f5 100755
> --- a/tracetool
> +++ b/tracetool
> @@ -158,6 +158,7 @@ void st_print_trace_events(FILE *stream, int 
> (*stream_printf)(FILE *stream, cons
>  void st_print_trace_file_status(FILE *stream, int (*stream_printf)(FILE 
> *stream, const char *fmt, ...));
>  void st_flush_trace_buffer(void);
>  void st_set_trace_file_enabled(bool enable);
> +bool st_set_trace_file(const char *file);
>  void change_trace_event_state(const char *tname, bool tstate);
>  EOF
>
> diff --git a/vl.c b/vl.c
> index 920717a..6d68e38 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -47,6 +47,10 @@
>  #include <dirent.h>
>  #include <netdb.h>
>  #include <sys/select.h>
> +#ifdef CONFIG_SIMPLE_TRACE
> +#include "trace.h"
> +#endif
> +
>  #ifdef CONFIG_BSD
>  #include <sys/stat.h>
>  #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || 
> defined(__DragonFly__)
> @@ -1822,6 +1826,9 @@ int main(int argc, char **argv, char **envp)
>     int show_vnc_port = 0;
>     int defconfig = 1;
>
> +#ifdef CONFIG_SIMPLE_TRACE
> +    char *trace_file = NULL;
> +#endif
>     atexit(qemu_run_exit_notifiers);
>     error_set_progname(argv[0]);
>
> @@ -2590,6 +2597,12 @@ int main(int argc, char **argv, char **envp)
>                 }
>                 xen_mode = XEN_ATTACH;
>                 break;
> +#ifdef CONFIG_SIMPLE_TRACE
> +            case QEMU_OPTION_trace:
> +                trace_file = (char *) qemu_malloc(strlen(optarg) + 1);
> +                strcpy(trace_file, optarg);
> +                break;
> +#endif

Malloc isn't necessary, just hold the optarg pointer like gdbstub_dev
and other string options do.

>             case QEMU_OPTION_readconfig:
>                 {
>                     int ret = qemu_read_config_file(optarg);
> @@ -2633,6 +2646,15 @@ int main(int argc, char **argv, char **envp)
>         data_dir = CONFIG_QEMU_DATADIR;
>     }
>
> +#ifdef CONFIG_SIMPLE_TRACE
> +    /*
> +     * Set the trace file name, if specified.
> +     */
> +    st_set_trace_file(trace_file);
> +    if (trace_file) {
> +        qemu_free(trace_file);
> +    }
> +#endif
>     /*
>      * Default to max_cpus = smp_cpus, in case the user doesn't
>      * specify a max_cpus value.
> --
> 1.6.2.5
>
>
>
> --
> Prerna Saxena
>
> Linux Technology Centre,
> IBM Systems and Technology Lab,
> Bangalore, India
>
>
>