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

[Prerna Saxena <prerna@linux.vnet.ibm.com>]

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