Add options to specify trace file name at startup and runtime.

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,

[temp@system]$ 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 <[email protected]>

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

monitor.c +1 -0

index 1e35a6b..8e2a3a6 100644
@@ -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);

monitor.c +0 -0

     }
  }
  #endif

qemu-monitor.hx +3 -3

index 25887bd..adfaf2b 100644
@@ -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,
     },
 

qemu-options.hx +0 -0

index d1d2272..aea9675 100644
@@ -2223,6 +2223,17 @@ Normally QEMU loads a configuration file from @var{sysconfdir}/qemu.conf and
  @var{sysconfdir}/target-@var{ARCH}.conf on startup.  The @code{-nodefconfig}

 option will prevent QEMU from loading these configuration files at startup. ETEXI

qemu-options.hx +11 -0

+#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
+@item -trace
+@findex -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

simpletrace.c +2 -3

index 71110b3..5812fe9 100644
@@ -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");

simpletrace.c +3 -0

+
+    st_set_trace_file_enabled(true);
+    return true;
  }
 
  static void flush_trace_file(void)

tracetool +1 -0

index ac832af..5b979f5 100755
@@ -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

vl.c +7 -0

index 920717a..6d68e38 100644
@@ -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]);
 

            case QEMU_OPTION_readconfig:
                {
                    int ret = qemu_read_config_file(optarg);

vl.c +9 -1

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

Quoted 15 lines · Prerna Saxena

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,

[temp@system]$ 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<[email protected]>

Quoted 8 lines

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

Quoted 10 lines

monitor.c +1 -0

index 1e35a6b..8e2a3a6 100644
@@ -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");
 

monitor.c static void do_trace_file(Monitor *mon, const QDict *qdict) · line 563

Quoted 13 lines

monitor.c +5 -0

     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?

monitor.c static void do_trace_file(Monitor *mon, const QDict *qdict)

Quoted 11 lines

monitor.c +3 -1

     }
  }
  #endif
...
...
  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.

monitor.c

Quoted 13 lines

monitor.c +9 -2

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

monitor.c

Quoted 7 lines

monitor.c +3 -3

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

monitor.c

Quoted 10 lines

monitor.c +10 -0

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

Quoted 4 lines

...
...

monitor.c int main(int argc, char **argv, char **envp) · line 2605

Quoted 10 lines

monitor.c +6 -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.

...

Quoted 15 lines · Stefan Hajnoczi

vl.c int main(int argc, char **argv, char **envp) · line 2605

Quoted 10 lines · Prerna Saxena

vl.c +6 -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.

It wouldnt be corect to use optarg directly here. If this optional argument is not specified, st_set_file_name() is called with a NULL argument, and the filename defaults to config-specified name.
(This is how gdbstub_dev works too. The optional argument is copied to gdbstub_dev if provided.)

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,

[temp@system]$ 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.

Changelog from v1 :
- Cleanups.

simpletrace.c +6 -0

+        } 
+    } else {
+        if (asprintf(&trace_file_name, "%s", file) < 0) {
+            trace_file_name = NULL;
+            return false;
+        }
     }

Stefanha, Malc,
Thanks for suggestions. Resending the patch after clean-up.

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,

[temp@system]$ 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.

Changelog from v2 :
- Cleanups.

Signed-off-by Prerna Saxena <[email protected]>

---
 monitor.c       |    6 ++++++
 qemu-monitor.hx |    6 +++---
 qemu-options.hx |   11 +++++++++++
 simpletrace.c   |   41 +++++++++++++++++++++++++++++++---------- tracetool       |    1 +
 vl.c            |   18 ++++++++++++++++++

 6 files changed, 70 insertions(+), 13 deletions(-)

A build warning arising out of 'incompatible pointer assignment' shows up at times. This will take care of it.

Signed-off-by Prerna Saxena <[email protected]>

---
 vl.c |    2 +-

 1 files changed, 1 insertions(+), 1 deletions(-)