Re: [Qemu-devel] [PATCH 01/12] Converting tracetool.sh to tracetool.py

[Stefan Hajnoczi]

2012/3/13 Lluís Vilanova <address@hidden>:
> diff --git a/configure b/configure
> index d7631ed..629e740 100755
> --- a/configure
> +++ b/configure
> @@ -1097,7 +1097,7 @@ echo "  --disable-docs           disable documentation 
> build"
>  echo "  --disable-vhost-net      disable vhost-net acceleration support"
>  echo "  --enable-vhost-net       enable vhost-net acceleration support"
>  echo "  --enable-trace-backend=B Set trace backend"
> -echo "                           Available backends:" 
> $("$source_path"/scripts/tracetool --list-backends)
> +echo "                           Available backends:" 
> $("$source_path"/scripts/tracetool.py --list-backends)

We need to use $python.

>  echo "  --with-trace-file=NAME   Full PATH,NAME of file to store traces"
>  echo "                           Default:trace-<pid>"
>  echo "  --disable-spice          disable spice"
> @@ -2654,7 +2654,7 @@ fi
>  ##########################################
>  # check if trace backend exists
>
> -sh "$source_path/scripts/tracetool" "--$trace_backend" --check-backend > 
> /dev/null 2> /dev/null
> +$python "$source_path/scripts/tracetool.py" "--backend=$trace_backend" 
> --check-backend  > /dev/null 2> /dev/null
>  if test "$?" -ne 0 ; then
>   echo
>   echo "Error: invalid trace backend"
> @@ -2672,7 +2672,8 @@ if test "$trace_backend" = "ust"; then
>  int main(void) { return 0; }
>  EOF
>   if compile_prog "" "" ; then
> -    LIBS="-lust $LIBS"
> +    LIBS="-lust -lurcu-bp $LIBS"
> +    libs_qga+="-lust -lurcu-bp"

This should be part of a separate commit because it's unrelated to
converting tracetool to Python.  Also, how about "$pkg_config --libs
ust" instead of hardcoding the libraries?

> +def get_argnames(line, sep=','):
> +    nfields = 0
> +    str = []
> +    args = get_args(line)
> +    for field in args.split():
> +      nfields = nfields + 1

2-space indentation.  Please use 4 spaces.

> +def dtrace_h(events):
> +    print '#include "trace-dtrace.h"'
> +    print
> +    for event in events:
> +        print '''static inline void trace_%(name)s(%(args)s) {
> +    if (QEMU_%(uppername)s_ENABLED()) {
> +        QEMU_%(uppername)s(%(argnames)s);
> +    }

This has changed in qemu.git/master - we don't check
QEMU_$name_ENABLED() anymore.  Just calling the trace event is fine.

> +        elif opt == "--check-backend":
> +            if any(backend in s for s in supported_backends):
> +                sys.exit(0)
> +            else:
> +                sys.exit(1)

Why not just:

if backend in supported_backends:
    sys.exit(0)
else:
    sys.exit(1)

Also backend is only assigned from --backend.  So it's possible to
trigger a Python NameError here if you didn't specify --backend first
- that's ugly and not user-friendly.  It would be better to set
backend to None by default.