Re: [Qemu-devel] [PATCH v9 3/4] qapi: Use an explicit input file

[Lluís Vilanova]

Markus Armbruster writes:

> Lluís Vilanova <address@hidden> writes:
>> Use an explicit input file on the command-line instead of reading from 
>> standard input

> Please limit commit message line length to 70 characters.

> Worth mentioning that this commit improves error messages!

>     <stdin>:123: Borked

> becomes

>     qapi-schema.json:123: Borked

> which enables Emacs to jump to the error.

>> 
>> Signed-off-by: Lluís Vilanova <address@hidden>
>> ---
>> Makefile                                           |   12 ++++++------
>> docs/qapi-code-gen.txt                             |    4 ++--
>> scripts/qapi-commands.py                           |    9 ++++++---
>> scripts/qapi-types.py                              |    9 ++++++---
>> scripts/qapi-visit.py                              |    9 ++++++---
>> scripts/qapi.py                                    |    5 +++--
>> tests/Makefile                                     |   11 ++++++-----
>> 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                     |    3 ++-
>> 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 +-
>> 25 files changed, 54 insertions(+), 42 deletions(-)
>> 
>> diff --git a/Makefile b/Makefile
>> index 84345ee..ac6a047 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -238,33 +238,33 @@ 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-out-type) -o qga/qapi-generated -p "qga-" -i $<, \
>> "  GEN   $@")
>> 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-out-type) -o qga/qapi-generated -p "qga-" -i $<, \
>> "  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-out-type) -o qga/qapi-generated -p "qga-" -i $<, \
>> "  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-out-type) -o "." -b -i $<, \
>> "  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-out-type) -o "." -b -i $<, \
>> "  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) -o "." -m < $<, \
>> +            $(gen-out-type) -o "." -m -i $<, \
>> "  GEN   $@")
>> 
>> QGALIB_GEN=$(addprefix qga/qapi-generated/, qga-qapi-types.h 
>> qga-qapi-visit.h qga-qmp-commands.h)
>> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
>> index d78921f..63b03cf 100644
>> --- a/docs/qapi-code-gen.txt
>> +++ b/docs/qapi-code-gen.txt
>> @@ -221,7 +221,7 @@ created code.
>> Example:
>> 
>> address@hidden:~/w/qemu2.git$ python scripts/qapi-types.py \
>> -      --output-dir="qapi-generated" --prefix="example-" < 
>> example-schema.json
>> +      --output-dir="qapi-generated" --prefix="example-" 
>> --input-file=example-schema.json
>> address@hidden:~/w/qemu2.git$ cat qapi-generated/example-qapi-types.c
>> /* AUTOMATICALLY GENERATED, DO NOT MODIFY */
>> 
>> @@ -291,7 +291,7 @@ $(prefix)qapi-visit.h: declarations for previously 
>> mentioned visitor
>> Example:
>> 
>> address@hidden:~/w/qemu2.git$ python scripts/qapi-visit.py \
>> -        --output-dir="qapi-generated" --prefix="example-" < 
>> example-schema.json
>> +        --output-dir="qapi-generated" --prefix="example-" 
>> --input-file=example-schema.json
>> address@hidden:~/w/qemu2.git$ cat qapi-generated/example-qapi-visit.c
>> /* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */
>> 
>> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
>> index 9734ab0..8d9096f 100644
>> --- a/scripts/qapi-commands.py
>> +++ b/scripts/qapi-commands.py
>> @@ -369,9 +369,10 @@ 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[1:], "chp:i:o:m",
>> ["source", "header", "prefix=",
>> -                                    "output-dir=", "type=", "middle"])
>> +                                    "input-file=", "output-dir=",
>> +                                    "type=", "middle"])
>> except getopt.GetoptError, err:
>> print str(err)
>> sys.exit(1)
>> @@ -389,6 +390,8 @@ do_h = False
>> for o, a in opts:
>> if o in ("-p", "--prefix"):
>> prefix = a
>> +    elif o in ("-i", "--input-file"):
>> +        input_file = a

> Here, you don't initialize input_file.  Missing -i is "diagnosed" as
> "NameError: name 'input_file' is not defined".  Since this fits right in
> with the existing, disgusting command line error reporting, I'm not
> asking you to do better.

>> elif o in ("-o", "--output-dir"):
>> output_dir = a + "/"
>> elif o in ("-t", "--type"):

> Not your fault: -t is missing in getopt.gnu_getopt()'s second argument
> above.

>> @@ -420,7 +423,7 @@ except os.error, e:
>> if e.errno != errno.EEXIST:
>> raise
>> 
>> -exprs = parse_schema(sys.stdin)
>> +exprs = parse_schema(input_file)
>> 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..b463232 100644
>> --- a/scripts/qapi-types.py
>> +++ b/scripts/qapi-types.py
>> @@ -279,14 +279,15 @@ 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[1:], "chbp:i:o:",
>> ["source", "header", "builtins",
>> -                                    "prefix=", "output-dir="])
>> +                                    "prefix=", "input-file=", 
>> "output-dir="])
>> except getopt.GetoptError, err:
>> print str(err)
>> sys.exit(1)
>> 
>> output_dir = ""
>> +input_file = ""
>> prefix = ""
>> c_file = 'qapi-types.c'
>> h_file = 'qapi-types.h'

> Here, you do initialize input_file.  Missing -i is "diagnosed" as
> "IOError: [Errno 2] No such file or directory: ''".  Again, I'm not
> asking you to do better.  The inconsistency annoys me, though.  If it
> still annoys me next time I touch this piece of crap, I'll clean it up.

>> @@ -298,6 +299,8 @@ do_builtins = False
>> for o, a in opts:
>> if o in ("-p", "--prefix"):
>> prefix = a
>> +    elif o in ("-i", "--input-file"):
>> +        input_file = a
>> elif o in ("-o", "--output-dir"):
>> output_dir = a + "/"
>> elif o in ("-c", "--source"):
>> @@ -378,7 +381,7 @@ fdecl.write(mcgen('''
>> ''',
>> guard=guardname(h_file)))
>> 
>> -exprs = parse_schema(sys.stdin)
>> +exprs = parse_schema(input_file)
>> 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..c6579be 100644
>> --- a/scripts/qapi-visit.py
>> +++ b/scripts/qapi-visit.py
>> @@ -397,13 +397,14 @@ 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[1:], "chbp:i:o:",
>> ["source", "header", "builtins", "prefix=",
>> -                                    "output-dir="])
>> +                                    "input-file=", "output-dir="])
>> except getopt.GetoptError, err:
>> print str(err)
>> sys.exit(1)
>> 
>> +input_file = ""
>> output_dir = ""
>> prefix = ""
>> c_file = 'qapi-visit.c'
>> @@ -416,6 +417,8 @@ do_builtins = False
>> for o, a in opts:
>> if o in ("-p", "--prefix"):
>> prefix = a
>> +    elif o in ("-i", "--input-file"):
>> +        input_file = a
>> elif o in ("-o", "--output-dir"):
>> output_dir = a + "/"
>> elif o in ("-c", "--source"):
>> @@ -494,7 +497,7 @@ fdecl.write(mcgen('''
>> ''',
>> prefix=prefix, guard=guardname(h_file)))
>> 
>> -exprs = parse_schema(sys.stdin)
>> +exprs = parse_schema(input_file)
>> 
>> # 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..3a38e27 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 = [
>> @@ -263,9 +264,9 @@ def check_exprs(schema):
>> if expr.has_key('union'):
>> check_union(expr, expr_elem['info'])
>> 
>> -def parse_schema(fp):
>> +def parse_schema(input_path):

> The parameter is not a path.  I'd call it fname, or maybe input_file.

>> try:
>> -        schema = QAPISchema(fp)
>> +        schema = QAPISchema(open(input_path, "r"))
>> except QAPISchemaError, e:
>> print >>sys.stderr, e
>> exit(1)
>> diff --git a/tests/Makefile b/tests/Makefile
>> index 42ed652..6803e7b 100644
>> --- a/tests/Makefile
>> +++ b/tests/Makefile
>> @@ -217,17 +217,17 @@ tests/test-vmstate$(EXESUF): tests/test-vmstate.o \
>> 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-out-type) -o tests -p "test-" -i $<, \
>> "  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-out-type) -o tests -p "test-" -i $<, \
>> "  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-out-type) -o tests -p "test-" -i $<, \
>> "  GEN   $@")
>> 
>> tests/test-string-output-visitor$(EXESUF): 
>> tests/test-string-output-visitor.o $(test-qapi-obj-y) libqemuutil.a 
>> libqemustub.a
>> @@ -370,13 +370,14 @@ check-tests/test-qapi.py: tests/test-qapi.py
>> $(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")
>> @diff -q $(SRC_PATH)/$*.out $*.test.out
>> -    @diff -q $(SRC_PATH)/$*.err $*.test.err
>> +    @# Sanitize error messages (make them independent of build directory)
>> +    @sed 's|$(SRC_PATH)/||g' $*.test.err | diff -q $(SRC_PATH)/$*.err -
>> @diff -q $(SRC_PATH)/$*.exit $*.test.exit
>> 

> Breaks when $(SRC_PATH) interpreted as regexp matches anything but a
> prefix of the source file part.  In particular, it breaks when SRC_PATH
> is ".." (which is common) and the error message contains "/".

> Here's a safer version:

>     @perl -p -e 's|^\Q$(SRC_PATH)\E/||' $*.test.err | diff -q 
> $(SRC_PATH)/$*.err -

Not a perl fan, so I'll assume your line as correct. Also, can perl be assumed
as present?


>> # Consolidated targets
>> diff --git a/tests/qapi-schema/duplicate-key.err 
>> b/tests/qapi-schema/duplicate-key.err
>> index 0801c6a..768b276 100644
>> --- a/tests/qapi-schema/duplicate-key.err
>> +++ b/tests/qapi-schema/duplicate-key.err
>> @@ -1 +1 @@
>> -<stdin>:2:10: Duplicate key "key"
>> +tests/qapi-schema/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..ccf72d2 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'
>> +tests/qapi-schema/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..790b675 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'
>> +tests/qapi-schema/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..a59749e 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
>> +tests/qapi-schema/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..200016b 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
>> +tests/qapi-schema/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..bfc890c 100644
>> --- a/tests/qapi-schema/funny-char.err
>> +++ b/tests/qapi-schema/funny-char.err
>> @@ -1 +1 @@
>> -<stdin>:2:36: Stray ";"
>> +tests/qapi-schema/funny-char.json:2:36: Stray ";"
>> diff --git a/tests/qapi-schema/missing-colon.err 
>> b/tests/qapi-schema/missing-colon.err
>> index 9f2a355..d9d66b3 100644
>> --- a/tests/qapi-schema/missing-colon.err
>> +++ b/tests/qapi-schema/missing-colon.err
>> @@ -1 +1 @@
>> -<stdin>:1:10: Expected ":"
>> +tests/qapi-schema/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..e73d277 100644
>> --- a/tests/qapi-schema/missing-comma-list.err
>> +++ b/tests/qapi-schema/missing-comma-list.err
>> @@ -1 +1 @@
>> -<stdin>:2:20: Expected "," or "]"
>> +tests/qapi-schema/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..52b3a8a 100644
>> --- a/tests/qapi-schema/missing-comma-object.err
>> +++ b/tests/qapi-schema/missing-comma-object.err
>> @@ -1 +1 @@
>> -<stdin>:2:3: Expected "," or "}"
>> +tests/qapi-schema/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..334f0c9 100644
>> --- a/tests/qapi-schema/non-objects.err
>> +++ b/tests/qapi-schema/non-objects.err
>> @@ -1 +1 @@
>> -<stdin>:1:1: Expected "{"
>> +tests/qapi-schema/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..9b18384 100644
>> --- a/tests/qapi-schema/quoted-structural-chars.err
>> +++ b/tests/qapi-schema/quoted-structural-chars.err
>> @@ -1 +1 @@
>> -<stdin>:1:1: Expected "{"
>> +tests/qapi-schema/quoted-structural-chars.json:1:1: Expected "{"
>> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
>> index 5a26ef3..634ef2d 100644
>> --- a/tests/qapi-schema/test-qapi.py
>> +++ b/tests/qapi-schema/test-qapi.py
>> @@ -12,10 +12,11 @@
>> 
>> from qapi import *
>> from pprint import pprint
>> +import os
>> import sys
>> 
>> try:
>> -    exprs = parse_schema(sys.stdin)
>> +    exprs = parse_schema(sys.argv[1])
>> except SystemExit:
>> raise
>> 

> This one reports missing input file name argument as "IndexError: list
> index out of range".  Again, fits right in.

> [...]

AFAIR, Eric said that it'd be better to leave it as simple as possible, since no
one else uses this script.

Same applies to the argument parsing of the other scripts (since no one is
sufficiently annoyed to rewrite it with argparse or similar).


Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth