Luiz Capitulino <lcapitul...@redhat.com> writes:
> 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).
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
