On 08/03/2010 07:45 PM, Stefan Hajnoczi wrote:
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?
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
