On Thu, 26 Jul 2012 13:50:24 +0200
Markus Armbruster <arm...@redhat.com> wrote:
> Paolo, Eric, I got a make puzzle for you below.
>
> Luiz Capitulino <lcapitul...@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 <lcapitul...@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
>
