From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=57607 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OgaMJ-0007OC-L1 for qemu-devel@nongnu.org; Wed, 04 Aug 2010 05:34:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OgaMD-0001Bx-V7 for qemu-devel@nongnu.org; Wed, 04 Aug 2010 05:33:59 -0400 Received: from e23smtp01.au.ibm.com ([202.81.31.143]:54745) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OgaMD-0001BY-Et for qemu-devel@nongnu.org; Wed, 04 Aug 2010 05:33:53 -0400 Received: from d23relay03.au.ibm.com (d23relay03.au.ibm.com [202.81.31.245]) by e23smtp01.au.ibm.com (8.14.4/8.13.1) with ESMTP id o749V52J015641 for ; Wed, 4 Aug 2010 19:31:05 +1000 Received: from d23av02.au.ibm.com (d23av02.au.ibm.com [9.190.235.138]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id o749XoCO1597660 for ; Wed, 4 Aug 2010 19:33:50 +1000 Received: from d23av02.au.ibm.com (loopback [127.0.0.1]) by d23av02.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id o749XoYr027081 for ; Wed, 4 Aug 2010 19:33:50 +1000 Message-ID: <4C5933FC.2090902@linux.vnet.ibm.com> Date: Wed, 04 Aug 2010 15:03:48 +0530 From: Prerna Saxena MIME-Version: 1.0 Subject: Re: [Qemu-devel] [Tracing][PATCH] Add options to specify trace file name at startup and runtime. References: <20100803110700.75d7c3b0@zephyr> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Ananth Narayan , qemu-devel , Stefan Hajnoczi On 08/03/2010 07:45 PM, Stefan Hajnoczi wrote: > On Tue, Aug 3, 2010 at 6:37 AM, 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. >> >> Signed-off-by: Prerna Saxena >> --- >> 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- 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