Re: [Qemu-devel] [PATCH 09/11] qapi: add qapi-errors.py

[Markus Armbruster]

Paolo, Eric, I got a make puzzle for you below.

Luiz Capitulino <address@hidden> writes:

> This script generates two files from qapi-schema-errors.json:
>
>  o qapi-errors.h: contains error macro definitions, eg. QERR_BASE_NOT_FOUND,
>                   corresponds to most of today's qerror.h
>
>  o qapi-errors.c: contains the error table that currently exists in qerror.c
>
> Signed-off-by: Luiz Capitulino <address@hidden>
> ---
>  Makefile               |   8 ++-
>  scripts/qapi-errors.py | 177 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 183 insertions(+), 2 deletions(-)
>  create mode 100644 scripts/qapi-errors.py
>
> diff --git a/Makefile b/Makefile
> index ab82ef3..2cdc732 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -22,8 +22,9 @@ GENERATED_HEADERS = config-host.h trace.h qemu-options.def
>  ifeq ($(TRACE_BACKEND),dtrace)
>  GENERATED_HEADERS += trace-dtrace.h
>  endif
> -GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h
> -GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c trace.c
> +GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h qapi-errors.h
> +GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c qapi-errors.c \
> +                     trace.c
>  
>  # Don't try to regenerate Makefile or configure
>  # We don't generate any of them
> @@ -200,6 +201,9 @@ $(SRC_PATH)/qapi-schema.json 
> $(SRC_PATH)/scripts/qapi-visit.py
>  qmp-commands.h qmp-marshal.c :\
>  $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py
>       $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py 
> $(gen-out-type) -m -o "." < $<, "  GEN   $@")
> +qapi-errors.h qapi-errors.c :\
> +$(SRC_PATH)/qapi-schema-errors.json $(SRC_PATH)/scripts/qapi-errors.py
> +     $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-errors.py -o 
> "." < $<, "  GEN   $@")

I'm afraid this isn't quite what you want.  It's shorthand for two
separate rules with the same recipe[*].  Therefore, it's prone to run
the recipe twice, with make blissfully unaware that each of the two runs
clobbers the other file, too.  Could conceivably lead to trouble with
parallel execution.

Paolo, Eric, maybe you can provide advice on how to best tell make that
a recipe generates multiple files.

>  QGALIB_OBJ=$(addprefix qapi-generated/, qga-qapi-types.o qga-qapi-visit.o 
> qga-qmp-marshal.o)
>  QGALIB_GEN=$(addprefix qapi-generated/, qga-qapi-types.h qga-qapi-visit.h 
> qga-qmp-commands.h)
> diff --git a/scripts/qapi-errors.py b/scripts/qapi-errors.py
> new file mode 100644
> index 0000000..59cf426
> --- /dev/null
> +++ b/scripts/qapi-errors.py
> @@ -0,0 +1,177 @@
> +#
> +# QAPI errors generator
> +#
> +# Copyright (C) 2012 Red Hat, Inc.
> +#
> +# This work is licensed under the terms of the GNU GPLv2.

Sure you want v2 and not v2+?

> +# See the COPYING.LIB file in the top-level directory.

COPYING.LIB is for LGPL, use COPYING with GPL.

> +from ordereddict import OrderedDict
> +import getopt, sys, os, errno
> +from qapi import *
> +
> +def gen_error_decl_prologue(header, guard, prefix=""):
> +    ret = mcgen('''
> +/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */
> +
> +/*
> + * schema-defined QAPI Errors
> + *
> + * Copyright (C) 2012 Red Hat, Inc.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or 
> later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#ifndef %(guard)s
> +#define %(guard)s
> +
> +''',
> +                 header=basename(header), guard=guardname(header), 
> prefix=prefix)
> +    return ret
> +
> +def gen_error_def_prologue(error_header, prefix=""):
> +    ret = mcgen('''
> +/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */
> +
> +/*
> + * schema-defined QMP Error table
> + *
> + * Copyright (C) 2012 Red Hat, Inc.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or 
> later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#include "%(prefix)s%(error_header)s"
> +
> +''',
> +                prefix=prefix, error_header=error_header)
> +    return ret
> +
> +def gen_error_macro(err_domain):
> +    string = ''
> +    cur = err_domain[0]
> +    for nxt in err_domain[1:]:

"err_domain" is just a fancy name for the error symbol, i.e. what
qerror.h calls 'class', isn't it?

Is it the same as error_class in gen_error_decl_macros() below?

> +        if string and cur.isupper() and nxt.islower():
> +            string += '_'
> +        string += cur
> +        cur = nxt
> +    string += cur
> +    return 'QERR_' + string.upper()
> +
> +def gen_error_def_table(exprs):
> +    ret = mcgen('''
> +static const QErrorStringTable qerror_table[] = {
> +''')
> +
> +    for err in exprs:
> +        macro = gen_error_macro(err['error'])
> +        desc = err['description']
> +        ret += mcgen('''
> +        {
> +            .error_fmt = %(error_macro)s,
> +            .desc      = "%(error_desc)s",
> +        },
> +''',    
> +                    error_macro=macro, error_desc=desc)
> +
> +    ret += mcgen('''
> +    {}
> +};
> +''')
> +
> +    return ret
> +
> +def gen_error_macro_data_str(data):
> +    colon = ''
> +    data_str = ''
> +    for k, v in data.items():
> +        data_str += colon + "'%s': " % k
> +        if v == 'str':
> +            data_str += "%s"
> +        elif v == 'int':
> +            data_str += '%"PRId64"'

PRId64 matches existing qerror.h practice.  Requires the macro's users
to pass suitable argument, which can be bothersome, but at least the
compiler helps with it.

> +        else:
> +            sys.exit("unknown data type '%s' for error '%s'" % (v, name))
> +        colon = ', '

colon is either empty or ', ', but never a colon.  What about calling it
sep, for separator?

> +    return data_str
> +
> +def gen_error_decl_macros(exprs):
> +    ret = ''
> +    for err in exprs:
> +        data = gen_error_macro_data_str({})
> +        if err.has_key('data'):
> +            data = gen_error_macro_data_str(err['data'])

Wouldn't 
           if err.has_key('data'):
               data = gen_error_macro_data_str(err['data'])
           else:
               data = gen_error_macro_data_str({})

be clearer?

> +        macro = gen_error_macro(err['error'])
> +        name = err['error']
> +
> +        ret += mcgen('''
> +#define %(error_macro)s \\
> +    "{ 'class': '%(error_class)s', 'data': { %(error_data)s } }"
> +
> +''',
> +                error_macro=macro, error_class=name, error_data=data)
> +    return ret
> +
> +def maybe_open(really, name, opt):
> +    if really:
> +        return open(name, opt)
> +    else:
> +        import StringIO
> +        return StringIO.StringIO()
> +
> +if __name__ == '__main__':
> +    try:
> +        opts, args = getopt.gnu_getopt(sys.argv[1:], "chp:o:",
> +                                       ["prefix=", "output-dir="])
> +    except getopt.GetoptError, err:
> +        print str(err)
> +        sys.exit(1)
> +
> +    prefix = ""
> +    output_dir = ""
> +    do_c = True
> +    do_h = True

Both are never false.  Purpose?

> +    c_file = 'qapi-errors.c'
> +    h_file = 'qapi-errors.h'
> +
> +    for o, a in opts:
> +        if o in ("-p", "--prefix"):
> +            prefix = a
> +        elif o in ("-o", "--output-dir"):
> +            output_dir = a + "/"
> +
> +    c_file = output_dir + prefix + c_file
> +    h_file = output_dir + prefix + h_file
> +
> +    try:
> +        os.makedirs(output_dir)
> +    except os.error, e:
> +        if e.errno != errno.EEXIST:
> +            raise
> +
> +    exprs = parse_schema(sys.stdin)
> +
> +    fdecl = maybe_open(do_h, h_file, 'w')
> +    ret = gen_error_decl_prologue(header=basename(h_file), 
> guard=guardname(h_file), prefix=prefix)
> +    fdecl.write(ret)
> +
> +    ret = gen_error_decl_macros(exprs)
> +    fdecl.write(ret)
> +
> +    fdecl.write("#endif\n")
> +    fdecl.flush()
> +    fdecl.close()

Err, is flush before close really necessary?

> +
> +    fdef = maybe_open(do_c, c_file, 'w')
> +    ret = gen_error_def_prologue(error_header=h_file, prefix=prefix)
> +    fdef.write(ret)
> +
> +    ret = gen_error_def_table(exprs)
> +    fdef.write(ret)
> +
> +    fdef.flush()
> +    fdef.close()


[*] http://www.gnu.org/software/make/manual/make.html#Multiple-Targets