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

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Thu, 26 Jul 2012 13:50:24 +0200
> Markus Armbruster <address@hidden> wrote:
>
>> 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.
>
> I started this script as a copy from the others qapi scripts and didn't
> notice that, thanks for spotting it (fixed in v3).

The other qapi scripts need fixing then --- self-contradictory copyright
statements are not a good idea.

>> > +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?
>
> Do you want to suggest a better name or are you just pointing out the lack of
> consistence?

No, I'm just trying to understand what's going on, and asking you to
confirm my hypothesis.

But yes, same name for same thing would be nice.

>> > +        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?
>
> Done.
>
>> 
>> > +    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?
>
> Makes no difference for me, but I've changed to your suggestion.
>
>> 
>> > +        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?
>
> This was also copied from others qapi scripts, but qapi-errors.py always
> creates both files so I've dropped this.
>
>> 
>> > +    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?
>
> I don't think so, but doesn't cause any harm either (and I'll maintain it as
> the others qapi scripts do this too).

Looks like cargo cult programming to me :)

>> > +
>> > +    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