From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:46524)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@redhat.com>) id 1Wjpba-0000i9-Oh
	for qemu-devel@nongnu.org; Mon, 12 May 2014 08:45:23 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@redhat.com>) id 1WjpbV-0004ga-Q6
	for qemu-devel@nongnu.org; Mon, 12 May 2014 08:45:18 -0400
Received: from mx1.redhat.com ([209.132.183.28]:62439)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@redhat.com>) id 1WjpbV-0004gS-IE
	for qemu-devel@nongnu.org; Mon, 12 May 2014 08:45:13 -0400
Date: Mon, 12 May 2014 14:45:03 +0200
From: Stefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20140512124503.GA5373@stefanha-thinkpad.redhat.com>
References: <20140509143850.30603.28508.stgit@fimbulvetr.bsc.es>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
In-Reply-To: <20140509143850.30603.28508.stgit@fimbulvetr.bsc.es>
Content-Transfer-Encoding: quoted-printable
Subject: Re: [Qemu-devel] [PATCH v3 ] trace: Multi-backend tracing
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: =?iso-8859-1?Q?Llu=EDs?= Vilanova <vilanova@ac.upc.edu>
Cc: Kevin Wolf <kwolf@redhat.com>, qemu-devel@nongnu.org, Anthony Liguori <aliguori@amazon.com>, Kazuya Saito <saito.kazuya@jp.fujitsu.com>

On Fri, May 09, 2014 at 04:38:51PM +0200, Llu=EDs 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 =3D format.replace("-", "_")
> =20
> -    backend =3D 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 =3D backend.replace("-", "_")
> -    backend =3D 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 =3D [backend.replace("-", "_") for backend in backends]

This seems to be duplicated down...

> +    backend =3D tracetool.backend.Wrapper(backends, format)
> =20
>      import tracetool.backend.dtrace
>      tracetool.backend.dtrace.BINARY =3D 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):
> =20
> =20
>  class Wrapper:
> -    def __init__(self, backend, format):
> -        self._backend =3D backend.replace("-", "_")
> +    def __init__(self, backends, format):
> +        self._backends =3D [backend.replace("-", "_") for backend in b=
ackends]

...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 backe=
nd.\n");
> +            return false;
> +    }
> +#else
> +    if (file) {
> +        fprintf(stderr, "error: -trace file=3D...: "
> +                "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
> =20
> +#include <stdint.h>

Why do you need this include?

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

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