On Tue, Aug 3, 2010 at 6:37 AM, Prerna Saxena <pre...@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,
> [t...@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 <pre...@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}/targ...@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
>
>
>
