diff mbox

[v7,05/13] trace: avoid conditional code compilation during option parsing

Message ID 20110825191804.1413.26801.stgit@ginnungagap.bsc.es
State New
Headers show

Commit Message

=?utf-8?Q?Llu=C3=ADs?= Aug. 25, 2011, 7:18 p.m. UTC 
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 Makefile.objs   |    8 ++++++++
 qemu-config.c   |    4 ----
 qemu-options.hx |    6 ++++--
 trace/control.h |   23 +++++++++++++++++++++++
 trace/dtrace.c  |   12 ++++++++++++
 trace/nop.c     |   12 ++++++++++++
 trace/simple.c  |   15 ++++++++++++---
 trace/simple.h  |    8 --------
 trace/stderr.c  |   12 ++++++++++++
 trace/ust.c     |   12 ++++++++++++
 vl.c            |   28 ++++++++++++++++++----------
 11 files changed, 113 insertions(+), 27 deletions(-)
 create mode 100644 trace/control.h
 create mode 100644 trace/dtrace.c
 create mode 100644 trace/nop.c
 create mode 100644 trace/stderr.c
 create mode 100644 trace/ust.c

Comments

Stefan Hajnoczi Aug. 31, 2011, 9:53 a.m. UTC | #1 
On Thu, Aug 25, 2011 at 09:18:04PM +0200, Lluís wrote:
> diff --git a/trace/control.h b/trace/control.h
> new file mode 100644
> index 0000000..80526f7
> --- /dev/null
> +++ b/trace/control.h
> @@ -0,0 +1,23 @@
> +/*
> + * Interface for configuring and controlling the state of tracing events.
> + *
> + * Copyright (C) 2011 Lluís Vilanova <vilanova@ac.upc.edu>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#ifndef TRACE_CONTROL_H
> +#define TRACE_CONTROL_H
> +
> +#include <stdbool.h>
> +
> +/** Whether any cmdline trace option is avilable. */

s/avilable/available/

> +bool trace_config_init (void);
> +/** Configure output trace file.
> + *
> + * @return Whether cmdline option is available.
> + */
> +bool trace_config_init_file (const char *file);
> +
> +#endif  /* TRACE_CONTROL_H */
> diff --git a/trace/dtrace.c b/trace/dtrace.c
> new file mode 100644
> index 0000000..e0121ca
> --- /dev/null
> +++ b/trace/dtrace.c
> @@ -0,0 +1,12 @@
> +#inclued "trace/control.h"

#include

Instead of duplicating all of this I suggest creating
trace/default-backend.c and linking that in for all external trace
backends which do not have qemu -trace ... support.

Stefan
Stefan Hajnoczi Aug. 31, 2011, 11:43 a.m. UTC | #2 
On Thu, Aug 25, 2011 at 8:18 PM, Lluís <xscript@gmx.net> wrote:
> diff --git a/vl.c b/vl.c
> index 145d738..ed2db9a 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -156,7 +156,7 @@ int main(int argc, char **argv)
>  #include "slirp/libslirp.h"
>
>  #include "trace.h"
> -#include "trace/simple.h"
> +#include "trace/control.h"
>  #include "qemu-queue.h"
>  #include "cpus.h"
>  #include "arch_init.h"
> @@ -2130,7 +2130,6 @@ int main(int argc, char **argv, char **envp)
>     int show_vnc_port = 0;
>  #endif
>     int defconfig = 1;
> -    const char *trace_file = NULL;
>     const char *log_mask = NULL;
>     const char *log_file = NULL;
>     GMemVTable mem_trace = {
> @@ -2928,14 +2927,27 @@ int main(int argc, char **argv, char **envp)
>                 }
>                 xen_mode = XEN_ATTACH;
>                 break;
> -#ifdef CONFIG_TRACE_SIMPLE
>             case QEMU_OPTION_trace:
> +            {
>                 opts = qemu_opts_parse(qemu_find_opts("trace"), optarg, 0);
> -                if (opts) {
> -                    trace_file = qemu_opt_get(opts, "file");
> +                if (!opts) {
> +                    exit(1);
> +                }
> +                if (!trace_config_init()) {
> +                    fprintf(stderr, "qemu: error: option \"-%s\" is not "
> +                            "supported by the selected tracing backend\n",
> +                            popt->name);
> +                    exit(1);
> +                }
> +                const char *trace_file = qemu_opt_get(opts, "file");
> +                if (trace_file && !trace_config_init_file(trace_file)) {
> +                    fprintf(stderr, "error: suboption \"-%s file\" is not "
> +                            "supported by the selected tracing backend\n",
> +                            popt->name);
> +                    exit(1);
>                 }
>                 break;
> -#endif
> +            }
>             case QEMU_OPTION_readconfig:
>                 {
>                     int ret = qemu_read_config_file(optarg);
> @@ -2993,10 +3005,6 @@ int main(int argc, char **argv, char **envp)
>         set_cpu_log(log_mask);
>     }
>
> -    if (!st_init(trace_file)) {
> -        fprintf(stderr, "warning: unable to initialize simple trace backend\n");
> -    }
> -

This changes the semantics of simpletrace.  If you start without
-trace ... then simpletrace will not be initialized and will not be
functional (no write-out thread).  That means you cannot start without
an events file and interactively enable the events that you want.  The
stderr backend handles this case fine because it has no initialization
code which this hunk replaces with trace_config_init() (only called
when -trace ... is given).

I suggest changing the trace backend interface to a single function:

/**
 * Set up the trace backend with the -trace events=...,file=... arguments
 *
 * @events file with events to be enabled on startup, may be NULL
 * @file   name of trace output file, may be NULL
 * @return true on success, false on error
 */
bool trace_backend_init(const char *events, const char *file);

In the vl.c:main() args parsing switch statement, just collect const
char *events and *file.  Then after the switch statement
unconditionally call trace_backend_init(events, file) (even if -trace
was not given).  Default behavior for backends that do not support
-trace becomes:

bool trace_backend_init(const char *events, const char *file)
{
    if (events || file) {
        fprintf(stderr, "The -trace option is not supported by this
trace backend\n");
        return false;
    }
    return true;
}

This guarantees that trace backends will receive the
trace_backend_init() call even when -trace is not used.  This is where
they can create threads or do any other setup.

Stefan
diff mbox

Patch

diff --git a/Makefile.objs b/Makefile.objs
index f1dfeda..4f5392a 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -379,9 +379,17 @@  ifneq ($(TRACE_BACKEND),dtrace)
 trace-obj-y = trace.o
 endif
 
+trace-nested-$(CONFIG_TRACE_NOP) += nop.o
+
 trace-nested-$(CONFIG_TRACE_SIMPLE) += simple.o
 trace-obj-$(CONFIG_TRACE_SIMPLE) += qemu-timer-common.o
 
+trace-nested-$(CONFIG_TRACE_STDERR) += stderr.o
+
+trace-nested-$(CONFIG_TRACE_UST) += ust.o
+
+trace-nested-$(CONFIG_TRACE_DTRACE) += dtrace.o
+
 trace-obj-y += $(addprefix trace/, $(trace-nested-y))
 
 ######################################################################
diff --git a/qemu-config.c b/qemu-config.c
index 4b79103..f67d3a5 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -302,7 +302,6 @@  static QemuOptsList qemu_mon_opts = {
     },
 };
 
-#ifdef CONFIG_TRACE_SIMPLE
 static QemuOptsList qemu_trace_opts = {
     .name = "trace",
     .implied_opt_name = "trace",
@@ -315,7 +314,6 @@  static QemuOptsList qemu_trace_opts = {
         { /* end of list */ }
     },
 };
-#endif
 
 static QemuOptsList qemu_cpudef_opts = {
     .name = "cpudef",
@@ -516,9 +514,7 @@  static QemuOptsList *vm_config_groups[32] = {
     &qemu_global_opts,
     &qemu_mon_opts,
     &qemu_cpudef_opts,
-#ifdef CONFIG_TRACE_SIMPLE
     &qemu_trace_opts,
-#endif
     &qemu_option_rom_opts,
     &qemu_machine_opts,
     &qemu_boot_opts,
diff --git a/qemu-options.hx b/qemu-options.hx
index 8a31b4f..d6421b9 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2432,17 +2432,19 @@  Normally QEMU loads a configuration file from @var{sysconfdir}/qemu.conf and
 @var{sysconfdir}/target-@var{ARCH}.conf on startup.  The @code{-nodefconfig}
 option will prevent QEMU from loading these configuration files at startup.
 ETEXI
-#ifdef CONFIG_TRACE_SIMPLE
 DEF("trace", HAS_ARG, QEMU_OPTION_trace,
     "-trace\n"
     "                Specify a trace file to log traces to\n",
     QEMU_ARCH_ALL)
 STEXI
+HXCOMM This line is not accurate, as the option is backend-specific but HX does
+HXCOMM not support conditional compilation of text.
 @item -trace
 @findex -trace
 Specify a trace file to log output traces to.
+
+This option is available only when using the @var{simple} tracing backend.
 ETEXI
-#endif
 
 HXCOMM This is the last statement. Insert new options before this line!
 STEXI
diff --git a/trace/control.h b/trace/control.h
new file mode 100644
index 0000000..80526f7
--- /dev/null
+++ b/trace/control.h
@@ -0,0 +1,23 @@ 
+/*
+ * Interface for configuring and controlling the state of tracing events.
+ *
+ * Copyright (C) 2011 Lluís Vilanova <vilanova@ac.upc.edu>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#ifndef TRACE_CONTROL_H
+#define TRACE_CONTROL_H
+
+#include <stdbool.h>
+
+/** Whether any cmdline trace option is avilable. */
+bool trace_config_init (void);
+/** Configure output trace file.
+ *
+ * @return Whether cmdline option is available.
+ */
+bool trace_config_init_file (const char *file);
+
+#endif  /* TRACE_CONTROL_H */
diff --git a/trace/dtrace.c b/trace/dtrace.c
new file mode 100644
index 0000000..e0121ca
--- /dev/null
+++ b/trace/dtrace.c
@@ -0,0 +1,12 @@ 
+#inclued "trace/control.h"
+
+
+bool trace_config_init (void)
+{
+    return false;
+}
+
+bool trace_config_init_file (const char *file)
+{
+    return false;
+}
diff --git a/trace/nop.c b/trace/nop.c
new file mode 100644
index 0000000..e0121ca
--- /dev/null
+++ b/trace/nop.c
@@ -0,0 +1,12 @@ 
+#inclued "trace/control.h"
+
+
+bool trace_config_init (void)
+{
+    return false;
+}
+
+bool trace_config_init_file (const char *file)
+{
+    return false;
+}
diff --git a/trace/simple.c b/trace/simple.c
index de355e9..f3891c1 100644
--- a/trace/simple.c
+++ b/trace/simple.c
@@ -16,6 +16,7 @@ 
 #include <pthread.h>
 #include "qemu-timer.h"
 #include "trace.h"
+#include "trace/control.h"
 
 /** Trace file header event ID */
 #define HEADER_EVENT_ID (~(uint64_t)0) /* avoids conflicting with TraceEventIDs */
@@ -330,7 +331,7 @@  void st_flush_trace_buffer(void)
     flush_trace_file(true);
 }
 
-bool st_init(const char *file)
+bool trace_config_init (void)
 {
     pthread_t thread;
     pthread_attr_t attr;
@@ -346,10 +347,18 @@  bool st_init(const char *file)
     pthread_sigmask(SIG_SETMASK, &oldset, NULL);
 
     if (ret != 0) {
-        return false;
+        fprintf(stderr, "warning: unable to initialize simple trace backend\n");
+    }
+    else {
+        atexit(st_flush_trace_buffer);
+        st_set_trace_file(NULL);
     }
+    return true;
+}
 
-    atexit(st_flush_trace_buffer);
+bool trace_config_init_file (const char *file)
+{
     st_set_trace_file(file);
+
     return true;
 }
diff --git a/trace/simple.h b/trace/simple.h
index 77633ab..08b9a52 100644
--- a/trace/simple.h
+++ b/trace/simple.h
@@ -15,7 +15,6 @@ 
 #include <stdbool.h>
 #include <stdio.h>
 
-#ifdef CONFIG_TRACE_SIMPLE
 typedef uint64_t TraceEventID;
 
 typedef struct {
@@ -37,12 +36,5 @@  void st_print_trace_file_status(FILE *stream, fprintf_function stream_printf);
 void st_set_trace_file_enabled(bool enable);
 bool st_set_trace_file(const char *file);
 void st_flush_trace_buffer(void);
-bool st_init(const char *file);
-#else
-static inline bool st_init(const char *file)
-{
-    return true;
-}
-#endif /* !CONFIG_TRACE_SIMPLE */
 
 #endif /* TRACE_SIMPLE_H */
diff --git a/trace/stderr.c b/trace/stderr.c
new file mode 100644
index 0000000..e0121ca
--- /dev/null
+++ b/trace/stderr.c
@@ -0,0 +1,12 @@ 
+#inclued "trace/control.h"
+
+
+bool trace_config_init (void)
+{
+    return false;
+}
+
+bool trace_config_init_file (const char *file)
+{
+    return false;
+}
diff --git a/trace/ust.c b/trace/ust.c
new file mode 100644
index 0000000..e0121ca
--- /dev/null
+++ b/trace/ust.c
@@ -0,0 +1,12 @@ 
+#inclued "trace/control.h"
+
+
+bool trace_config_init (void)
+{
+    return false;
+}
+
+bool trace_config_init_file (const char *file)
+{
+    return false;
+}
diff --git a/vl.c b/vl.c
index 145d738..ed2db9a 100644
--- a/vl.c
+++ b/vl.c
@@ -156,7 +156,7 @@  int main(int argc, char **argv)
 #include "slirp/libslirp.h"
 
 #include "trace.h"
-#include "trace/simple.h"
+#include "trace/control.h"
 #include "qemu-queue.h"
 #include "cpus.h"
 #include "arch_init.h"
@@ -2130,7 +2130,6 @@  int main(int argc, char **argv, char **envp)
     int show_vnc_port = 0;
 #endif
     int defconfig = 1;
-    const char *trace_file = NULL;
     const char *log_mask = NULL;
     const char *log_file = NULL;
     GMemVTable mem_trace = {
@@ -2928,14 +2927,27 @@  int main(int argc, char **argv, char **envp)
                 }
                 xen_mode = XEN_ATTACH;
                 break;
-#ifdef CONFIG_TRACE_SIMPLE
             case QEMU_OPTION_trace:
+            {
                 opts = qemu_opts_parse(qemu_find_opts("trace"), optarg, 0);
-                if (opts) {
-                    trace_file = qemu_opt_get(opts, "file");
+                if (!opts) {
+                    exit(1);
+                }
+                if (!trace_config_init()) {
+                    fprintf(stderr, "qemu: error: option \"-%s\" is not "
+                            "supported by the selected tracing backend\n",
+                            popt->name);
+                    exit(1);
+                }
+                const char *trace_file = qemu_opt_get(opts, "file");
+                if (trace_file && !trace_config_init_file(trace_file)) {
+                    fprintf(stderr, "error: suboption \"-%s file\" is not "
+                            "supported by the selected tracing backend\n",
+                            popt->name);
+                    exit(1);
                 }
                 break;
-#endif
+            }
             case QEMU_OPTION_readconfig:
                 {
                     int ret = qemu_read_config_file(optarg);
@@ -2993,10 +3005,6 @@  int main(int argc, char **argv, char **envp)
         set_cpu_log(log_mask);
     }
 
-    if (!st_init(trace_file)) {
-        fprintf(stderr, "warning: unable to initialize simple trace backend\n");
-    }
-
     /* If no data_dir is specified then try to find it relative to the
        executable path.  */
     if (!data_dir) {