| Message ID | 1343235256-26310-10-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. > +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
Il 26/07/2012 13:50, Markus Armbruster ha scritto: >> > +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. Hmm, I would just do qapi-errors.h: qapi-errors.c 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 think that's what I usually saw for bison (which creates both .h and .c). A perhaps cleaner alternative is to add a stamp file, and make both files depend on it. Paolo
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 [...] > 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 [...] > +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", > + }, > +''', Trailing whitespace. [...]
On 07/26/2012 05:55 AM, Paolo Bonzini wrote: > Il 26/07/2012 13:50, Markus Armbruster ha scritto: >>>> +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. > > Hmm, I would just do > > qapi-errors.h: qapi-errors.c > 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 think that's what I usually saw for bison (which creates both .h and .c). Indeed, per https://www.gnu.org/software/automake/manual/automake.html#Multiple-Outputs, that is an appropriate solution for a 2-file generation. It is only when you have more than two files where... > > A perhaps cleaner alternative is to add a stamp file, and make both files > depend on it. a stamp file makes more sense.
On 07/26/2012 06:38 AM, Eric Blake wrote: >>> Paolo, Eric, maybe you can provide advice on how to best tell make that >>> a recipe generates multiple files. >> >> Hmm, I would just do >> >> qapi-errors.h: qapi-errors.c >> 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 think that's what I usually saw for bison (which creates both .h and .c). > > Indeed, per > https://www.gnu.org/software/automake/manual/automake.html#Multiple-Outputs, > that is an appropriate solution for a 2-file generation. Or, since we depend on GNU make, we can use pattern rules instead (untested): %-errors.h %-errors.c: \ %(SRC_PATH)/%-schema-errors.json $(SRC_PATH)/srcipts/%-errors.py $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-errors.py -o "." < $<, " GEN $@") https://www.gnu.org/software/automake/manual/make.html#Pattern-Examples
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 >
On Thu, 26 Jul 2012 13:55:29 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 26/07/2012 13:50, Markus Armbruster ha scritto: > >> > +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. > > Hmm, I would just do > > qapi-errors.h: qapi-errors.c > 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've done this for v3, thanks for the suggestion. > > I think that's what I usually saw for bison (which creates both .h and .c). > > A perhaps cleaner alternative is to add a stamp file, and make both files > depend on it. > > Paolo >
On Thu, 26 Jul 2012 14:12:50 +0200 Markus Armbruster <armbru@redhat.com> wrote: > 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 > [...] > > 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 > [...] > > +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", > > + }, > > +''', > > Trailing whitespace. Fixed for v3.
Luiz Capitulino <lcapitulino@redhat.com> writes: > 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). 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
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 $@") 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. +# See the COPYING.LIB file in the top-level directory. + +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:]: + 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"' + else: + sys.exit("unknown data type '%s' for error '%s'" % (v, name)) + colon = ', ' + 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']) + 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 + 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() + + 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()
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