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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 26 Jul 2012 13:50:24 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Paolo, Eric, I got a make puzzle for you below.
> 
> Luiz Capitulino <lcapitulino@redhat.com> 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 <lcapitulino@redhat.com>
> > ---
> >  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).

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

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

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