| Message ID | 20100803110700.75d7c3b0@zephyr |
|---|---|
| State | New |
| Headers | show |
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 > > >
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,
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
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.
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(-)