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

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