Re: [Qemu-devel] [PATCH v7 05/13] trace: avoid conditional code compilation during option parsing

[Stefan Hajnoczi <stefanha@gmail.com>]

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