Re: [Qemu-devel] [PATCH v3 ] trace: Multi-backend tracing

[Stefan Hajnoczi]

On Fri, May 09, 2014 at 04:38:51PM +0200, Lluís Vilanova wrote:

Nice.  A few minor points but overall it looks close.

> @@ -260,13 +260,13 @@ def generate(fevents, format, backend,
>          raise TracetoolError("unknown format: %s" % format)
>      format = format.replace("-", "_")
>  
> -    backend = str(backend)
> -    if len(backend) is 0:
> -        raise TracetoolError("backend not set")
> -    if not tracetool.backend.exists(backend):
> -        raise TracetoolError("unknown backend: %s" % backend)
> -    backend = backend.replace("-", "_")
> -    backend = tracetool.backend.Wrapper(backend, format)
> +    if len(backends) is 0:
> +        raise TracetoolError("no backends specified")
> +    for backend in backends:
> +        if not tracetool.backend.exists(backend):
> +            raise TracetoolError("unknown backend: %s" % backend)
> +    backends = [backend.replace("-", "_") for backend in backends]

This seems to be duplicated down...

> +    backend = tracetool.backend.Wrapper(backends, format)
>  
>      import tracetool.backend.dtrace
>      tracetool.backend.dtrace.BINARY = binary
> diff --git a/scripts/tracetool/backend/__init__.py 
> b/scripts/tracetool/backend/__init__.py
> index 5e36f04..5bfa1ef 100644
> --- a/scripts/tracetool/backend/__init__.py
> +++ b/scripts/tracetool/backend/__init__.py
> @@ -99,17 +99,18 @@ def exists(name):
>  
>  
>  class Wrapper:
> -    def __init__(self, backend, format):
> -        self._backend = backend.replace("-", "_")
> +    def __init__(self, backends, format):
> +        self._backends = [backend.replace("-", "_") for backend in backends]

...here.

> @@ -130,3 +149,29 @@ void trace_backend_init_events(const char *fname)
>          exit(1);
>      }
>  }
> +
> +bool trace_init_backends(const char *events, const char *file)
> +{
> +#ifdef CONFIG_TRACE_SIMPLE
> +    if (!st_init(file)) {
> +            fprintf(stderr, "failed to initialize simple tracing 
> backend.\n");
> +            return false;
> +    }
> +#else
> +    if (file) {
> +        fprintf(stderr, "error: -trace file=...: "
> +                "option not supported by the selected tracing backends\n");
> +        return false;
> +    }
> +#endif
> +
> +#ifdef CONFIG_TRACE_FTRACE
> +    if (!ftrace_init()) {
> +            fprintf(stderr, "failed to initialize ftrace backend.\n");
> +            return false;
> +    }
> +#endif

Please use scripts/checkpatch.pl to scan patches for coding style
issues.  QEMU uses 4-space indentation.

> diff --git a/trace/ftrace.h b/trace/ftrace.h
> index 94cb8d5..ad18ccb 100644
> --- a/trace/ftrace.h
> +++ b/trace/ftrace.h
> @@ -1,10 +1,15 @@
>  #ifndef TRACE_FTRACE_H
>  #define TRACE_FTRACE_H
>  
> +#include <stdint.h>

Why do you need this include?

> +
> +
>  #define MAX_TRACE_STRLEN 512
>  #define _STR(x) #x
>  #define STR(x) _STR(x)
>  
>  extern int trace_marker_fd;
>  
> +bool ftrace_init(void);

Missing #include <stdbool.h> for the bool type.