[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V2 2/3] qapi: Change the qapi scripts to take th
|
From: |
Markus Armbruster |
|
Subject: |
Re: [Qemu-devel] [PATCH V2 2/3] qapi: Change the qapi scripts to take their input as first argument. |
|
Date: |
Thu, 27 Mar 2014 18:56:35 +0100 |
|
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) |
Benoît Canet <address@hidden> writes:
> This patch is here to pave the way for the JSON include directive which
> will need to do include loop detection.
>
> Signed-off-by: Benoit Canet <address@hidden>
> ---
> Makefile | 24 ++++++++--------
> scripts/qapi-commands.py | 8 ++++--
> scripts/qapi-types.py | 8 ++++--
> scripts/qapi-visit.py | 8 ++++--
> scripts/qapi.py | 29
> ++++++++++++++------
> tests/Makefile | 14 +++++-----
> tests/qapi-schema/duplicate-key.err | 2 +-
> .../qapi-schema/flat-union-invalid-branch-key.err | 2 +-
> .../flat-union-invalid-discriminator.err | 2 +-
> tests/qapi-schema/flat-union-no-base.err | 2 +-
> .../flat-union-string-discriminator.err | 2 +-
> tests/qapi-schema/funny-char.err | 2 +-
> tests/qapi-schema/missing-colon.err | 2 +-
> tests/qapi-schema/missing-comma-list.err | 2 +-
> tests/qapi-schema/missing-comma-object.err | 2 +-
> tests/qapi-schema/non-objects.err | 2 +-
> tests/qapi-schema/quoted-structural-chars.err | 2 +-
> tests/qapi-schema/test-qapi.py | 2 +-
> tests/qapi-schema/trailing-comma-list.err | 2 +-
> tests/qapi-schema/trailing-comma-object.err | 2 +-
> tests/qapi-schema/unclosed-list.err | 2 +-
> tests/qapi-schema/unclosed-object.err | 2 +-
> tests/qapi-schema/unclosed-string.err | 2 +-
> tests/qapi-schema/union-invalid-base.err | 2 +-
> 24 files changed, 76 insertions(+), 51 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index ec74039..9bec4ff 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -236,24 +236,24 @@ gen-out-type = $(subst .,-,$(suffix $@))
> qapi-py = $(SRC_PATH)/scripts/qapi.py $(SRC_PATH)/scripts/ordereddict.py
>
> qga/qapi-generated/qga-qapi-types.c qga/qapi-generated/qga-qapi-types.h :\
> -$(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
> - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py
> $(gen-out-type) -o qga/qapi-generated -p "qga-" < $<, " GEN $@")
> +$(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py
> $(SRC_PATH)/qga/qapi-schema.json $(gen-out-type) -o qga/qapi-generated -p
> "qga-" < $<, " GEN $@")
You remove the prerequisite $(SRC_PATH)/qga/qapi-schema.json. Breaks
remake after schema change.
Why do you redirect input from $(SRC_PATH)/scripts/qapi-types.py?
I suspect what we really want is the exact old rule with '< $<' replaced
by just '$<'.
More of the same below.
> qga/qapi-generated/qga-qapi-visit.c qga/qapi-generated/qga-qapi-visit.h :\
> -$(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py)
> - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py
> $(gen-out-type) -o qga/qapi-generated -p "qga-" < $<, " GEN $@")
> +$(SRC_PATH)/scripts/qapi-visit.py $(qapi-py)
> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py
> $(SRC_PATH)/qga/qapi-schema.json $(gen-out-type) -o qga/qapi-generated -p
> "qga-" < $<, " GEN $@")
> qga/qapi-generated/qga-qmp-commands.h qga/qapi-generated/qga-qmp-marshal.c :\
> -$(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py
> $(qapi-py)
> - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py
> $(gen-out-type) -o qga/qapi-generated -p "qga-" < $<, " GEN $@")
> +$(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py
> $(SRC_PATH)/qga/qapi-schema.json $(gen-out-type) -o qga/qapi-generated -p
> "qga-" < $<, " GEN $@")
>
> qapi-types.c qapi-types.h :\
> -$(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
> - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py
> $(gen-out-type) -o "." -b < $<, " GEN $@")
> +$(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py
> $(SRC_PATH)/qapi-schema.json $(gen-out-type) -o "." -b < $<, " GEN $@")
> qapi-visit.c qapi-visit.h :\
> -$(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py $(qapi-py)
> - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py
> $(gen-out-type) -o "." -b < $<, " GEN $@")
> +$(SRC_PATH)/scripts/qapi-visit.py $(qapi-py)
> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py
> $(SRC_PATH)/qapi-schema.json $(gen-out-type) -o "." -b < $<, " GEN $@")
> qmp-commands.h qmp-marshal.c :\
> -$(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
> - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py
> $(gen-out-type) -m -o "." < $<, " GEN $@")
> +$(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py
> $(SRC_PATH)/qapi-schema.json $(gen-out-type) -m -o "." < $<, " GEN $@")
>
> QGALIB_GEN=$(addprefix qga/qapi-generated/, qga-qapi-types.h
> qga-qapi-visit.h qga-qmp-commands.h)
> $(qga-obj-y) qemu-ga.o: $(QGALIB_GEN)
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 9734ab0..2995eab 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -369,13 +369,17 @@ def gen_command_def_prologue(prefix="", proxy=False):
>
>
> try:
> - opts, args = getopt.gnu_getopt(sys.argv[1:], "chp:o:m",
> + opts, args = getopt.gnu_getopt(sys.argv[2:], "chp:o:m",
This looks wrong. You should continue to pass all arguments to
gnu_getopt(). You get back two values, the options (assigned to opts
here), and the non-option arguments (assigned to args).
> ["source", "header", "prefix=",
> "output-dir=", "type=", "middle"])
> except getopt.GetoptError, err:
> print str(err)
> sys.exit(1)
>
> +if len(sys.argv) < 2:
> + print "The first argument must be the file name to parse"
> + sys.exit(1)
> +
You should test len(args).
Are arguments beyond the first used? If no, better reject length other
than 1.
> output_dir = ""
> prefix = ""
> dispatch_type = "sync"
> @@ -420,7 +424,7 @@ except os.error, e:
> if e.errno != errno.EEXIST:
> raise
>
> -exprs = parse_schema(sys.stdin)
> +exprs = parse_schema(sys.argv[1])
You should use args[0].
Same for the other utilities.
> commands = filter(lambda expr: expr.has_key('command'), exprs)
> commands = filter(lambda expr: not expr.has_key('gen'), commands)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 10864ef..07df991 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -279,13 +279,17 @@ void qapi_free_%(type)s(%(c_type)s obj)
>
>
> try:
> - opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:o:",
> + opts, args = getopt.gnu_getopt(sys.argv[2:], "chbp:o:",
> ["source", "header", "builtins",
> "prefix=", "output-dir="])
> except getopt.GetoptError, err:
> print str(err)
> sys.exit(1)
>
> +if len(sys.argv) < 2:
> + print "The first argument must be the file name to parse"
> + sys.exit(1)
> +
> output_dir = ""
> prefix = ""
> c_file = 'qapi-types.c'
> @@ -378,7 +382,7 @@ fdecl.write(mcgen('''
> ''',
> guard=guardname(h_file)))
>
> -exprs = parse_schema(sys.stdin)
> +exprs = parse_schema(sys.argv[1])
> exprs = filter(lambda expr: not expr.has_key('gen'), exprs)
>
> fdecl.write(guardstart("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 45ce3a9..cd53c97 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -397,13 +397,17 @@ void visit_type_%(name)s(Visitor *m, %(name)s * obj,
> const char *name, Error **e
> name=name)
>
> try:
> - opts, args = getopt.gnu_getopt(sys.argv[1:], "chbp:o:",
> + opts, args = getopt.gnu_getopt(sys.argv[2:], "chbp:o:",
> ["source", "header", "builtins",
> "prefix=",
> "output-dir="])
> except getopt.GetoptError, err:
> print str(err)
> sys.exit(1)
>
> +if len(sys.argv) < 2:
> + print "The first argument must be the file name to parse"
> + sys.exit(1)
> +
> output_dir = ""
> prefix = ""
> c_file = 'qapi-visit.c'
> @@ -494,7 +498,7 @@ fdecl.write(mcgen('''
> ''',
> prefix=prefix, guard=guardname(h_file)))
>
> -exprs = parse_schema(sys.stdin)
> +exprs = parse_schema(sys.argv[1])
>
> # to avoid header dependency hell, we always generate declarations
> # for built-in types in our header files and simply guard them
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index b474c39..597042a 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -12,6 +12,7 @@
> # See the COPYING file in the top-level directory.
>
> from ordereddict import OrderedDict
> +import os
> import sys
>
> builtin_types = [
> @@ -35,6 +36,9 @@ builtin_type_qtypes = {
> 'uint64': 'QTYPE_QINT',
> }
>
> +def get_filename(path):
> + return os.path.split(path)[1]
> +
> class QAPISchemaError(Exception):
> def __init__(self, schema, msg):
> self.fp = schema.fp
> @@ -48,7 +52,8 @@ class QAPISchemaError(Exception):
> self.col += 1
>
> def __str__(self):
> - return "%s:%s:%s: %s" % (self.fp.name, self.line, self.col, self.msg)
> + return "%s:%s:%s: %s" % (get_filename(self.fp.name),
> + self.line, self.col, self.msg)
>
This makes the error message refer to the input file by its
non-directory part. As mentioned in review of Lluís's patch, plase
don't do that. I understand the directory part gets in the way of tests
because $(SRC_PATH), but the way to cope with that is having the tests
normalize output with a suitable filter, not making the error messages
less informational and potentially ambiguous.
> class QAPIExprError(Exception):
> def __init__(self, expr_info, msg):
> @@ -57,7 +62,8 @@ class QAPIExprError(Exception):
> self.msg = msg
>
> def __str__(self):
> - return "%s:%s: %s" % (self.fp.name, self.line, self.msg)
> + return "%s:%s: %s" % (get_filename(self.fp.name),
> + self.line, self.msg)
>
Likewise.
> class QAPISchema:
>
> @@ -263,12 +269,19 @@ def check_exprs(schema):
> if expr.has_key('union'):
> check_union(expr, expr_elem['info'])
>
> -def parse_schema(fp):
> - try:
> - schema = QAPISchema(fp)
> - except QAPISchemaError, e:
> - print >>sys.stderr, e
> - exit(1)
> +def build_schema(path):
This isn't a path, it's a file name. Please call it fname, filename, or
something like that.
> + with open(path, "r") as fp:
> + try:
> + schema = QAPISchema(fp)
> + except QAPISchemaError, e:
> + print >>sys.stderr, e
> + exit(1)
> + return schema
> +
> +def parse_schema(path):
> + path = os.path.abspath(path)
This turns path into an normalized absolute file name. The result ends
up in self.fp.name. Therefore, your change makes error messages refer
to input files by their absolute filename, currently less the directory
part. Not nice. Note that abspath() can change the non-directory part!
Symbolic links are so much fun...
I figure you want normalization to detect include cycles. If that's the
case, consider doing it in the patch that needs it.
Independently, please use normalized file names *only* for include cycle
detection, *not* for error messages.
> +
> + schema = build_schema(path)
>
> exprs = []
>
> diff --git a/tests/Makefile b/tests/Makefile
> index 2d021fb..c4ed5c2 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -215,14 +215,14 @@ tests/test-vmstate$(EXESUF): tests/test-vmstate.o \
> libqemuutil.a
>
> tests/test-qapi-types.c tests/test-qapi-types.h :\
> -$(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json
> $(SRC_PATH)/scripts/qapi-types.py
> - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py
> $(gen-out-type) -o tests -p "test-" < $<, " GEN $@")
> +$(SRC_PATH)/scripts/qapi-types.py
> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py
> $(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json $(gen-out-type) -o tests
> -p "test-" < $<, " GEN $@")
> tests/test-qapi-visit.c tests/test-qapi-visit.h :\
> -$(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json
> $(SRC_PATH)/scripts/qapi-visit.py
> - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py
> $(gen-out-type) -o tests -p "test-" < $<, " GEN $@")
> +$(SRC_PATH)/scripts/qapi-visit.py
> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py
> $(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json $(gen-out-type) -o tests
> -p "test-" < $<, " GEN $@")
> tests/test-qmp-commands.h tests/test-qmp-marshal.c :\
> -$(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json
> $(SRC_PATH)/scripts/qapi-commands.py
> - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py
> $(gen-out-type) -o tests -p "test-" < $<, " GEN $@")
> +$(SRC_PATH)/scripts/qapi-commands.py
> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py
> $(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json $(gen-out-type) -o tests
> -p "test-" < $<, " GEN $@")
>
> tests/test-string-output-visitor$(EXESUF):
> tests/test-string-output-visitor.o $(test-qapi-obj-y) libqemuutil.a
> libqemustub.a
> tests/test-string-input-visitor$(EXESUF): tests/test-string-input-visitor.o
> $(test-qapi-obj-y) libqemuutil.a libqemustub.a
> @@ -362,7 +362,7 @@ check-tests/test-qapi.py: tests/test-qapi.py
>
> .PHONY: $(patsubst %, check-%, $(check-qapi-schema-y))
> $(patsubst %, check-%, $(check-qapi-schema-y)): check-%.json:
> $(SRC_PATH)/%.json
> - $(call quiet-command, PYTHONPATH=$(SRC_PATH)/scripts $(PYTHON)
> $(SRC_PATH)/tests/qapi-schema/test-qapi.py <$^ >$*.test.out 2>$*.test.err;
> echo $$? >$*.test.exit, " TEST $*.out")
> + $(call quiet-command, PYTHONPATH=$(SRC_PATH)/scripts $(PYTHON)
> $(SRC_PATH)/tests/qapi-schema/test-qapi.py $^ >$*.test.out 2>$*.test.err;
> echo $$? >$*.test.exit, " TEST $*.out")
> @diff -q $(SRC_PATH)/$*.out $*.test.out
> @diff -q $(SRC_PATH)/$*.err $*.test.err
> @diff -q $(SRC_PATH)/$*.exit $*.test.exit
> diff --git a/tests/qapi-schema/duplicate-key.err
> b/tests/qapi-schema/duplicate-key.err
> index 0801c6a..21303ef 100644
> --- a/tests/qapi-schema/duplicate-key.err
> +++ b/tests/qapi-schema/duplicate-key.err
> @@ -1 +1 @@
> -<stdin>:2:10: Duplicate key "key"
> +duplicate-key.json:2:10: Duplicate key "key"
> diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.err
> b/tests/qapi-schema/flat-union-invalid-branch-key.err
> index 1125caf..34b4584 100644
> --- a/tests/qapi-schema/flat-union-invalid-branch-key.err
> +++ b/tests/qapi-schema/flat-union-invalid-branch-key.err
> @@ -1 +1 @@
> -<stdin>:13: Discriminator value 'value_wrong' is not found in enum 'TestEnum'
> +flat-union-invalid-branch-key.json:13: Discriminator value 'value_wrong' is
> not found in enum 'TestEnum'
> diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.err
> b/tests/qapi-schema/flat-union-invalid-discriminator.err
> index cad9dbf..b6ce3b7 100644
> --- a/tests/qapi-schema/flat-union-invalid-discriminator.err
> +++ b/tests/qapi-schema/flat-union-invalid-discriminator.err
> @@ -1 +1 @@
> -<stdin>:13: Discriminator 'enum_wrong' is not a member of base type
> 'TestBase'
> +flat-union-invalid-discriminator.json:13: Discriminator 'enum_wrong' is not
> a member of base type 'TestBase'
> diff --git a/tests/qapi-schema/flat-union-no-base.err
> b/tests/qapi-schema/flat-union-no-base.err
> index e2d7443..7929455 100644
> --- a/tests/qapi-schema/flat-union-no-base.err
> +++ b/tests/qapi-schema/flat-union-no-base.err
> @@ -1 +1 @@
> -<stdin>:7: Flat union 'TestUnion' must have a base field
> +flat-union-no-base.json:7: Flat union 'TestUnion' must have a base field
> diff --git a/tests/qapi-schema/flat-union-string-discriminator.err
> b/tests/qapi-schema/flat-union-string-discriminator.err
> index 8748270..fc1e2d5 100644
> --- a/tests/qapi-schema/flat-union-string-discriminator.err
> +++ b/tests/qapi-schema/flat-union-string-discriminator.err
> @@ -1 +1 @@
> -<stdin>:13: Discriminator 'kind' must be of enumeration type
> +flat-union-string-discriminator.json:13: Discriminator 'kind' must be of
> enumeration type
> diff --git a/tests/qapi-schema/funny-char.err
> b/tests/qapi-schema/funny-char.err
> index d3dd293..ee65869 100644
> --- a/tests/qapi-schema/funny-char.err
> +++ b/tests/qapi-schema/funny-char.err
> @@ -1 +1 @@
> -<stdin>:2:36: Stray ";"
> +funny-char.json:2:36: Stray ";"
> diff --git a/tests/qapi-schema/missing-colon.err
> b/tests/qapi-schema/missing-colon.err
> index 9f2a355..676cce5 100644
> --- a/tests/qapi-schema/missing-colon.err
> +++ b/tests/qapi-schema/missing-colon.err
> @@ -1 +1 @@
> -<stdin>:1:10: Expected ":"
> +missing-colon.json:1:10: Expected ":"
> diff --git a/tests/qapi-schema/missing-comma-list.err
> b/tests/qapi-schema/missing-comma-list.err
> index 4fe0700..d0ed8c3 100644
> --- a/tests/qapi-schema/missing-comma-list.err
> +++ b/tests/qapi-schema/missing-comma-list.err
> @@ -1 +1 @@
> -<stdin>:2:20: Expected "," or "]"
> +missing-comma-list.json:2:20: Expected "," or "]"
> diff --git a/tests/qapi-schema/missing-comma-object.err
> b/tests/qapi-schema/missing-comma-object.err
> index b0121b5..ad9b457 100644
> --- a/tests/qapi-schema/missing-comma-object.err
> +++ b/tests/qapi-schema/missing-comma-object.err
> @@ -1 +1 @@
> -<stdin>:2:3: Expected "," or "}"
> +missing-comma-object.json:2:3: Expected "," or "}"
> diff --git a/tests/qapi-schema/non-objects.err
> b/tests/qapi-schema/non-objects.err
> index a6c2dc2..e958cf0 100644
> --- a/tests/qapi-schema/non-objects.err
> +++ b/tests/qapi-schema/non-objects.err
> @@ -1 +1 @@
> -<stdin>:1:1: Expected "{"
> +non-objects.json:1:1: Expected "{"
> diff --git a/tests/qapi-schema/quoted-structural-chars.err
> b/tests/qapi-schema/quoted-structural-chars.err
> index a6c2dc2..77732d0 100644
> --- a/tests/qapi-schema/quoted-structural-chars.err
> +++ b/tests/qapi-schema/quoted-structural-chars.err
> @@ -1 +1 @@
> -<stdin>:1:1: Expected "{"
> +quoted-structural-chars.json:1:1: Expected "{"
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index ac6da13..4f217d2 100644
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -15,7 +15,7 @@ from pprint import pprint
> import sys
>
> try:
> - exprs = parse_schema(sys.stdin)
> + exprs = parse_schema(sys.argv[1])
> except SystemExit:
> raise
> except Exception, e:
What if argv[1] is None?
Suggest to error out unless len(sys.argv) == 1.
[...]