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

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

[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 <prerna@linux.vnet.ibm.com>
---
 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(-)

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]");
     }
 }
 #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
 @var{sysconfdir}/target-@var{ARCH}.conf 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
+@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
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;
+}
 
-    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);
     }
 
-    trace_fp = fopen(filename, "w");
-    free(filename);
-    return trace_fp != NULL;
+    if (trace_file_name) {
+        free(trace_file_name);
+    }
+
+    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;
+        }
+    }
+
+    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
             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

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 <prerna@linux.vnet.ibm.com> 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,
> [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 <prerna@linux.vnet.ibm.com>
> ---
>  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
>  @var{sysconfdir}/target-@var{ARCH}.conf 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
> +@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
> 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
>
>
>

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

[Prerna Saxena]

On 08/03/2010 07:45 PM, Stefan Hajnoczi wrote:
> On Tue, Aug 3, 2010 at 6:37 AM, Prerna Saxena<prerna@linux.vnet.ibm.com>  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,
>> [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<prerna@linux.vnet.ibm.com>
>> ---
>>   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?

Agree, changed in v2.

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

Done in v2.

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

Agree this is unnecessary. Changed in v2.

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

Changed in v2.

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

Done.

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

>
>...
>

Thanks,
-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India

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

[Stefan Hajnoczi]

On Wed, Aug 4, 2010 at 10:33 AM, Prerna Saxena
<prerna@linux.vnet.ibm.com> wrote:
> On 08/03/2010 07:45 PM, Stefan Hajnoczi wrote:
>> On Tue, Aug 3, 2010 at 6:37 AM, Prerna Saxena<prerna@linux.vnet.ibm.com>
>>  wrote:
>>> @@ -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.)

const char *trace_file = NULL;
...
switch (...) {
    case QEMU_OPTION_trace:
        trace_file = optarg;
        break;
}
...
st_set_trace_file(trace_file);

There is no need to malloc/strcpy/free.

Stefan

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

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

Changelog from v1 :
- Cleanups.

Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
---
 monitor.c       |    6 ++++++
 qemu-monitor.hx |    6 +++---
 qemu-options.hx |   11 +++++++++++
 simpletrace.c   |   41 +++++++++++++++++++++++++++++++----------
 tracetool       |    1 +
 vl.c            |   20 ++++++++++++++++++++
 6 files changed, 72 insertions(+), 13 deletions(-)

diff --git a/monitor.c b/monitor.c
index 1e35a6b..1d6c4c0 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);
+        help_cmd(mon, "trace-file");
     }
 }
 #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
 @var{sysconfdir}/target-@var{ARCH}.conf 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
+@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
diff --git a/simpletrace.c b/simpletrace.c
index 71110b3..19855f4 100644
--- a/simpletrace.c
+++ b/simpletrace.c
@@ -20,25 +20,46 @@ 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)
+static inline bool open_trace_file(void)
 {
-    char *filename;
+    trace_fp = fopen(trace_file_name, "w");
+    return trace_fp != NULL;
+}
+
+/**
+ * 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)
+{
+    st_set_trace_file_enabled(false);
 
-    if (asprintf(&filename, CONFIG_TRACE_FILE, getpid()) < 0) {
-        return false;
+    free(trace_file_name);
+
+    if (!file) {
+        if (asprintf(&trace_file_name, CONFIG_TRACE_FILE, getpid()) < 0) {
+            trace_file_name = NULL;
+	    return false;
+        } 
+    } else {
+        if (asprintf(&trace_file_name, "%s", file) < 0) {
+            trace_file_name = NULL;
+            return false;
+        }
     }
 
-    trace_fp = fopen(filename, "w");
-    free(filename);
-    return trace_fp != NULL;
+    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..df1257c 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
             case QEMU_OPTION_readconfig:
                 {
                     int ret = qemu_read_config_file(optarg);
@@ -2633,6 +2646,13 @@ 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);
+    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

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

[malc]

On Wed, 4 Aug 2010, Prerna Saxena 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,
> [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.

[..snip..]

> +    st_set_trace_file_enabled(false);
>  
> -    if (asprintf(&filename, CONFIG_TRACE_FILE, getpid()) < 0) {
> -        return false;
> +    free(trace_file_name);
> +
> +    if (!file) {
> +        if (asprintf(&trace_file_name, CONFIG_TRACE_FILE, getpid()) < 0) {
> +            trace_file_name = NULL;
> +	    return false;

Tab here.

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

-- 
mailto:av1474@comtv.ru

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

[Prerna Saxena]

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 <prerna@linux.vnet.ibm.com>
---
 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(-)

diff --git a/monitor.c b/monitor.c
index 1e35a6b..1d6c4c0 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);
+        help_cmd(mon, "trace-file");
     }
 }
 #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
 @var{sysconfdir}/target-@var{ARCH}.conf 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
+@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
diff --git a/simpletrace.c b/simpletrace.c
index 71110b3..860bcf1 100644
--- a/simpletrace.c
+++ b/simpletrace.c
@@ -20,25 +20,46 @@ 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)
+static inline bool open_trace_file(void)
 {
-    char *filename;
+    trace_fp = fopen(trace_file_name, "w");
+    return trace_fp != NULL;
+}
+
+/**
+ * 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)
+{
+    st_set_trace_file_enabled(false);
 
-    if (asprintf(&filename, CONFIG_TRACE_FILE, getpid()) < 0) {
-        return false;
+    free(trace_file_name);
+
+    if (!file) {
+        if (asprintf(&trace_file_name, CONFIG_TRACE_FILE, getpid()) < 0) {
+            trace_file_name = NULL;
+            return false;
+        } 
+    } else {
+        if (asprintf(&trace_file_name, "%s", file) < 0) {
+            trace_file_name = NULL;
+            return false;
+        }
     }
 
-    trace_fp = fopen(filename, "w");
-    free(filename);
-    return trace_fp != NULL;
+    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..46314d4 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,11 @@ int main(int argc, char **argv, char **envp)
                 }
                 xen_mode = XEN_ATTACH;
                 break;
+#ifdef CONFIG_SIMPLE_TRACE
+            case QEMU_OPTION_trace:
+                trace_file = optarg;
+                break;
+#endif
             case QEMU_OPTION_readconfig:
                 {
                     int ret = qemu_read_config_file(optarg);
@@ -2633,6 +2645,12 @@ 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);
+#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

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

[Stefan Hajnoczi]

On Wed, Aug 4, 2010 at 11:53 AM, Prerna Saxena
<prerna@linux.vnet.ibm.com> wrote:
> 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 <prerna@linux.vnet.ibm.com>
> ---
>  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(-)

Thanks, applied to the tracing branch:
http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/tracing

Stefan

[Qemu-devel] [Tracing][PATCH] Fix a build warning

[Prerna Saxena]

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

Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
---
 vl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/vl.c b/vl.c
index 46314d4..989c35b 100644
--- a/vl.c
+++ b/vl.c
@@ -1827,7 +1827,7 @@ int main(int argc, char **argv, char **envp)
     int defconfig = 1;
 
 #ifdef CONFIG_SIMPLE_TRACE
-    char *trace_file = NULL;
+    const char *trace_file = NULL;
 #endif
     atexit(qemu_run_exit_notifiers);
     error_set_progname(argv[0]);
-- 
1.6.2.5



-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India

[Qemu-devel] Re: [Tracing][PATCH] Fix a build warning

[Stefan Hajnoczi]

On Thu, Aug 5, 2010 at 1:08 PM, Prerna Saxena <prerna@linux.vnet.ibm.com> wrote:
> A build warning arising out of 'incompatible pointer assignment' shows
> up at times. This will take care of it.
>
> Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>
> ---
>  vl.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

Thanks Prerna.  I noticed the warning and added const when merging
your trace file name patch:

http://repo.or.cz/w/qemu/stefanha.git/commitdiff/ea06bd2dd2e168059bde1e8c4d1003bb7b07dfe9

Stefan