| Message ID | 1395934399-18769-4-git-send-email-benoit.canet@irqsave.net |
|---|---|
| State | New |
| Headers | show |
On 03/27/2014 09:33 AM, Benoît Canet wrote: > The new directive in the form { 'include': 'path/to/file.json' } will trigger the > parsing of path/to/file.json. > The directive will be replaced by the result of the parsing. > > This will allow for easy modularisation of qapi JSON descriptions files. > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > --- > docs/qapi-code-gen.txt | 14 ++++++++++++++ > scripts/qapi.py | 17 ++++++++++++++++- > tests/Makefile | 2 +- > tests/qapi-schema/include.exit | 1 + > tests/qapi-schema/include.json | 4 ++++ > tests/qapi-schema/include.out | 8 ++++++++ > tests/qapi-schema/include/include.json | 7 +++++++ > tests/qapi-schema/include_loop.exit | 1 + > tests/qapi-schema/include_loop.json | 1 + > tests/qapi-schema/include_loop.out | 1 + > 10 files changed, 54 insertions(+), 2 deletions(-) > create mode 100644 tests/qapi-schema/include.err > create mode 100644 tests/qapi-schema/include.exit > create mode 100644 tests/qapi-schema/include.json > create mode 100644 tests/qapi-schema/include.out > create mode 100644 tests/qapi-schema/include/include.json > create mode 100644 tests/qapi-schema/include_loop.err > create mode 100644 tests/qapi-schema/include_loop.exit > create mode 100644 tests/qapi-schema/include_loop.json > create mode 100644 tests/qapi-schema/include_loop.out > > for expr_elem in schema.exprs: > expr = expr_elem['expr'] > - if expr.has_key('enum'): > + if expr.has_key('include'): > + prefix = os.path.split(path)[0] > + sub_path = os.path.join(prefix, expr['include']) Should you validate that expr['include'] is a string, so we know the user didn't do something stupid like { 'include': true } or { 'include': { 'hello':'world' } }. And if you add validation, you also need to add tests. > + elif expr.has_key('enum'): > add_enum(expr['enum'], expr['data']) For that matter (pre-existing, but you suffer from the same problem) - there's no validation against unexpected keys, which means { 'enum':'foo', 'garbage':'blah' } and { 'include':'path', 'garbage':'blah' } both manage to silently ignore the 'garbage' key. If you fix it for 'enum', do that first as a separate commit. > +++ b/tests/qapi-schema/include.json > @@ -0,0 +1,4 @@ > +{ 'enum': 'Status', > + 'data': [ 'good', 'bad', 'ugly' ] } > +{ 'include': './include/include.json' } This tests successful inclusion relative to directory names. > +++ b/tests/qapi-schema/include_loop.json > @@ -0,0 +1 @@ > +{ 'include': 'include_loop.json' } And this tests that self-inclusion is rejected. But still missing tests for: error message when the error occurs in an included file (ideally, the error message should be the location within the included file, not the outer file) error occurring after a successful include (ideally, the lines injected by the included file do NOT upset the line number tracking of the original file) multi-file loops being rejected (a includes b includes a) each include name is relative to the current file even across nested inclusion that changes directory (such as 'a' includes 'include/b' includes '../c', should work when 'a' and 'c' are in the same directory, and NOT try to pick up 'c' in the parent directory of 'a').
Benoît Canet <benoit.canet@irqsave.net> writes: > The new directive in the form { 'include': 'path/to/file.json' } will trigger the > parsing of path/to/file.json. > The directive will be replaced by the result of the parsing. > > This will allow for easy modularisation of qapi JSON descriptions files. > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > --- > docs/qapi-code-gen.txt | 14 ++++++++++++++ > scripts/qapi.py | 17 ++++++++++++++++- > tests/Makefile | 2 +- > tests/qapi-schema/include.exit | 1 + > tests/qapi-schema/include.json | 4 ++++ > tests/qapi-schema/include.out | 8 ++++++++ > tests/qapi-schema/include/include.json | 7 +++++++ > tests/qapi-schema/include_loop.exit | 1 + > tests/qapi-schema/include_loop.json | 1 + > tests/qapi-schema/include_loop.out | 1 + > 10 files changed, 54 insertions(+), 2 deletions(-) > create mode 100644 tests/qapi-schema/include.err > create mode 100644 tests/qapi-schema/include.exit > create mode 100644 tests/qapi-schema/include.json > create mode 100644 tests/qapi-schema/include.out > create mode 100644 tests/qapi-schema/include/include.json > create mode 100644 tests/qapi-schema/include_loop.err > create mode 100644 tests/qapi-schema/include_loop.exit > create mode 100644 tests/qapi-schema/include_loop.json > create mode 100644 tests/qapi-schema/include_loop.out > > diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt > index d78921f..a16aa47 100644 > --- a/docs/qapi-code-gen.txt > +++ b/docs/qapi-code-gen.txt > @@ -180,6 +180,20 @@ An example command is: > 'data': { 'arg1': 'str', '*arg2': 'str' }, > 'returns': 'str' } > > +=== Includes === > + > +A schema file can include other sub schema files with the include > +directive. > + > +An example of include directive is: > + > +{ 'include': 'path/to/sub_schema.json' } > + > +The include path is relative to the current schema file. > +The include parsing method is recursive. > +The expressions resulting from the parsing of the sub schema are injected > +in place of the include directive like a C #include would do. > + > > == Code generation == > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 597042a..0b0c8e4 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -269,6 +269,8 @@ def check_exprs(schema): > if expr.has_key('union'): > check_union(expr, expr_elem['info']) > > +modules = [] > + > def build_schema(path): > with open(path, "r") as fp: > try: > @@ -281,13 +283,26 @@ def build_schema(path): > def parse_schema(path): > path = os.path.abspath(path) > > + if path in modules: > + print "Module inclusion loop detected with module: %s" %\ > + get_filename(path) > + sys.exit(1) > + > + modules.append(path) > + I'm afraid this will detect a loop when you include the same file twice. Maybe that's not useful, but it's definitely not a loop. Easy to avoid: make modules a stack, push on include, pop on EOF. Bonus: you can easily print the actual cycle. Suggest to drop "Module" from the error message. I don't like the identifier modules, either. > schema = build_schema(path) > > exprs = [] > > for expr_elem in schema.exprs: > expr = expr_elem['expr'] > - if expr.has_key('enum'): > + if expr.has_key('include'): > + prefix = os.path.split(path)[0] > + sub_path = os.path.join(prefix, expr['include']) > + sub_exprs = parse_schema(sub_path) > + exprs += sub_exprs > + continue > + elif expr.has_key('enum'): > add_enum(expr['enum'], expr['data']) > elif expr.has_key('union'): > add_union(expr) > diff --git a/tests/Makefile b/tests/Makefile > index c4ed5c2..b5e4cf0 100644 > --- a/tests/Makefile > +++ b/tests/Makefile > @@ -164,7 +164,7 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \ > duplicate-key.json union-invalid-base.json flat-union-no-base.json \ > flat-union-invalid-discriminator.json \ > flat-union-invalid-branch-key.json flat-union-reverse-define.json \ > - flat-union-string-discriminator.json) > + flat-union-string-discriminator.json include.json include_loop.json) > > GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h > > diff --git a/tests/qapi-schema/include.err b/tests/qapi-schema/include.err > new file mode 100644 > index 0000000..e69de29 > diff --git a/tests/qapi-schema/include.exit b/tests/qapi-schema/include.exit > new file mode 100644 > index 0000000..573541a > --- /dev/null > +++ b/tests/qapi-schema/include.exit > @@ -0,0 +1 @@ > +0 > diff --git a/tests/qapi-schema/include.json b/tests/qapi-schema/include.json > new file mode 100644 > index 0000000..ffece21 > --- /dev/null > +++ b/tests/qapi-schema/include.json > @@ -0,0 +1,4 @@ > +{ 'enum': 'Status', > + 'data': [ 'good', 'bad', 'ugly' ] } > +{ 'include': './include/include.json' } > +{ 'foo': '42' } Doubt I'd bother with a sub-directory. Certainly covered by your artistic license, though :) [...]
The Thursday 27 Mar 2014 à 19:07:48 (+0100), Markus Armbruster wrote : > Benoît Canet <benoit.canet@irqsave.net> writes: > > > The new directive in the form { 'include': 'path/to/file.json' } will trigger the > > parsing of path/to/file.json. > > The directive will be replaced by the result of the parsing. > > > > This will allow for easy modularisation of qapi JSON descriptions files. > > > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > > --- > > docs/qapi-code-gen.txt | 14 ++++++++++++++ > > scripts/qapi.py | 17 ++++++++++++++++- > > tests/Makefile | 2 +- > > tests/qapi-schema/include.exit | 1 + > > tests/qapi-schema/include.json | 4 ++++ > > tests/qapi-schema/include.out | 8 ++++++++ > > tests/qapi-schema/include/include.json | 7 +++++++ > > tests/qapi-schema/include_loop.exit | 1 + > > tests/qapi-schema/include_loop.json | 1 + > > tests/qapi-schema/include_loop.out | 1 + > > 10 files changed, 54 insertions(+), 2 deletions(-) > > create mode 100644 tests/qapi-schema/include.err > > create mode 100644 tests/qapi-schema/include.exit > > create mode 100644 tests/qapi-schema/include.json > > create mode 100644 tests/qapi-schema/include.out > > create mode 100644 tests/qapi-schema/include/include.json > > create mode 100644 tests/qapi-schema/include_loop.err > > create mode 100644 tests/qapi-schema/include_loop.exit > > create mode 100644 tests/qapi-schema/include_loop.json > > create mode 100644 tests/qapi-schema/include_loop.out > > > > diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt > > index d78921f..a16aa47 100644 > > --- a/docs/qapi-code-gen.txt > > +++ b/docs/qapi-code-gen.txt > > @@ -180,6 +180,20 @@ An example command is: > > 'data': { 'arg1': 'str', '*arg2': 'str' }, > > 'returns': 'str' } > > > > +=== Includes === > > + > > +A schema file can include other sub schema files with the include > > +directive. > > + > > +An example of include directive is: > > + > > +{ 'include': 'path/to/sub_schema.json' } > > + > > +The include path is relative to the current schema file. > > +The include parsing method is recursive. > > +The expressions resulting from the parsing of the sub schema are injected > > +in place of the include directive like a C #include would do. > > + > > > > == Code generation == > > > > diff --git a/scripts/qapi.py b/scripts/qapi.py > > index 597042a..0b0c8e4 100644 > > --- a/scripts/qapi.py > > +++ b/scripts/qapi.py > > @@ -269,6 +269,8 @@ def check_exprs(schema): > > if expr.has_key('union'): > > check_union(expr, expr_elem['info']) > > > > +modules = [] > > + > > def build_schema(path): > > with open(path, "r") as fp: > > try: > > @@ -281,13 +283,26 @@ def build_schema(path): > > def parse_schema(path): > > path = os.path.abspath(path) > > > > + if path in modules: > > + print "Module inclusion loop detected with module: %s" %\ > > + get_filename(path) > > + sys.exit(1) > > + > > + modules.append(path) > > + > > I'm afraid this will detect a loop when you include the same file > twice. Maybe that's not useful, but it's definitely not a loop. > > Easy to avoid: make modules a stack, push on include, pop on EOF. > Bonus: you can easily print the actual cycle. > > Suggest to drop "Module" from the error message. > > I don't like the identifier modules, either. > > > schema = build_schema(path) > > > > exprs = [] > > > > for expr_elem in schema.exprs: > > expr = expr_elem['expr'] > > - if expr.has_key('enum'): > > + if expr.has_key('include'): > > + prefix = os.path.split(path)[0] > > + sub_path = os.path.join(prefix, expr['include']) > > + sub_exprs = parse_schema(sub_path) > > + exprs += sub_exprs > > + continue > > + elif expr.has_key('enum'): > > add_enum(expr['enum'], expr['data']) > > elif expr.has_key('union'): > > add_union(expr) > > diff --git a/tests/Makefile b/tests/Makefile > > index c4ed5c2..b5e4cf0 100644 > > --- a/tests/Makefile > > +++ b/tests/Makefile > > @@ -164,7 +164,7 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \ > > duplicate-key.json union-invalid-base.json flat-union-no-base.json \ > > flat-union-invalid-discriminator.json \ > > flat-union-invalid-branch-key.json flat-union-reverse-define.json \ > > - flat-union-string-discriminator.json) > > + flat-union-string-discriminator.json include.json include_loop.json) > > > > GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h > > > > diff --git a/tests/qapi-schema/include.err b/tests/qapi-schema/include.err > > new file mode 100644 > > index 0000000..e69de29 > > diff --git a/tests/qapi-schema/include.exit b/tests/qapi-schema/include.exit > > new file mode 100644 > > index 0000000..573541a > > --- /dev/null > > +++ b/tests/qapi-schema/include.exit > > @@ -0,0 +1 @@ > > +0 > > diff --git a/tests/qapi-schema/include.json b/tests/qapi-schema/include.json > > new file mode 100644 > > index 0000000..ffece21 > > --- /dev/null > > +++ b/tests/qapi-schema/include.json > > @@ -0,0 +1,4 @@ > > +{ 'enum': 'Status', > > + 'data': [ 'good', 'bad', 'ugly' ] } > > +{ 'include': './include/include.json' } > > +{ 'foo': '42' } > > Doubt I'd bother with a sub-directory. Certainly covered by your > artistic license, though :) Don't kid me you'll need it for the next QEMU logo :) Best regards Benoît > > [...]
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt index d78921f..a16aa47 100644 --- a/docs/qapi-code-gen.txt +++ b/docs/qapi-code-gen.txt @@ -180,6 +180,20 @@ An example command is: 'data': { 'arg1': 'str', '*arg2': 'str' }, 'returns': 'str' } +=== Includes === + +A schema file can include other sub schema files with the include +directive. + +An example of include directive is: + +{ 'include': 'path/to/sub_schema.json' } + +The include path is relative to the current schema file. +The include parsing method is recursive. +The expressions resulting from the parsing of the sub schema are injected +in place of the include directive like a C #include would do. + == Code generation == diff --git a/scripts/qapi.py b/scripts/qapi.py index 597042a..0b0c8e4 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -269,6 +269,8 @@ def check_exprs(schema): if expr.has_key('union'): check_union(expr, expr_elem['info']) +modules = [] + def build_schema(path): with open(path, "r") as fp: try: @@ -281,13 +283,26 @@ def build_schema(path): def parse_schema(path): path = os.path.abspath(path) + if path in modules: + print "Module inclusion loop detected with module: %s" %\ + get_filename(path) + sys.exit(1) + + modules.append(path) + schema = build_schema(path) exprs = [] for expr_elem in schema.exprs: expr = expr_elem['expr'] - if expr.has_key('enum'): + if expr.has_key('include'): + prefix = os.path.split(path)[0] + sub_path = os.path.join(prefix, expr['include']) + sub_exprs = parse_schema(sub_path) + exprs += sub_exprs + continue + elif expr.has_key('enum'): add_enum(expr['enum'], expr['data']) elif expr.has_key('union'): add_union(expr) diff --git a/tests/Makefile b/tests/Makefile index c4ed5c2..b5e4cf0 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -164,7 +164,7 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \ duplicate-key.json union-invalid-base.json flat-union-no-base.json \ flat-union-invalid-discriminator.json \ flat-union-invalid-branch-key.json flat-union-reverse-define.json \ - flat-union-string-discriminator.json) + flat-union-string-discriminator.json include.json include_loop.json) GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h diff --git a/tests/qapi-schema/include.err b/tests/qapi-schema/include.err new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/include.exit b/tests/qapi-schema/include.exit new file mode 100644 index 0000000..573541a --- /dev/null +++ b/tests/qapi-schema/include.exit @@ -0,0 +1 @@ +0 diff --git a/tests/qapi-schema/include.json b/tests/qapi-schema/include.json new file mode 100644 index 0000000..ffece21 --- /dev/null +++ b/tests/qapi-schema/include.json @@ -0,0 +1,4 @@ +{ 'enum': 'Status', + 'data': [ 'good', 'bad', 'ugly' ] } +{ 'include': './include/include.json' } +{ 'foo': '42' } diff --git a/tests/qapi-schema/include.out b/tests/qapi-schema/include.out new file mode 100644 index 0000000..89e43e8 --- /dev/null +++ b/tests/qapi-schema/include.out @@ -0,0 +1,8 @@ +[OrderedDict([('enum', 'Status'), ('data', ['good', 'bad', 'ugly'])]), + OrderedDict([('bar', '33')]), + OrderedDict([('enum', 'Fruits'), ('data', ['orange', 'apple', 'gooseberry'])]), + OrderedDict([('baz', '54')]), + OrderedDict([('foo', '42')])] +[{'enum_name': 'Status', 'enum_values': ['good', 'bad', 'ugly']}, + {'enum_name': 'Fruits', 'enum_values': ['orange', 'apple', 'gooseberry']}] +[] diff --git a/tests/qapi-schema/include/include.json b/tests/qapi-schema/include/include.json new file mode 100644 index 0000000..f445eee --- /dev/null +++ b/tests/qapi-schema/include/include.json @@ -0,0 +1,7 @@ + +{ 'bar': '33' } + +{ 'enum': 'Fruits', + 'data': [ 'orange', 'apple', 'gooseberry' ] } + +{ 'baz': '54' } diff --git a/tests/qapi-schema/include_loop.err b/tests/qapi-schema/include_loop.err new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/include_loop.exit b/tests/qapi-schema/include_loop.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/tests/qapi-schema/include_loop.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/include_loop.json b/tests/qapi-schema/include_loop.json new file mode 100644 index 0000000..cb8ff03 --- /dev/null +++ b/tests/qapi-schema/include_loop.json @@ -0,0 +1 @@ +{ 'include': 'include_loop.json' } diff --git a/tests/qapi-schema/include_loop.out b/tests/qapi-schema/include_loop.out new file mode 100644 index 0000000..35da4dd --- /dev/null +++ b/tests/qapi-schema/include_loop.out @@ -0,0 +1 @@ +Module inclusion loop detected with module: include_loop.json
The new directive in the form { 'include': 'path/to/file.json' } will trigger the parsing of path/to/file.json. The directive will be replaced by the result of the parsing. This will allow for easy modularisation of qapi JSON descriptions files. Signed-off-by: Benoit Canet <benoit@irqsave.net> --- docs/qapi-code-gen.txt | 14 ++++++++++++++ scripts/qapi.py | 17 ++++++++++++++++- tests/Makefile | 2 +- tests/qapi-schema/include.exit | 1 + tests/qapi-schema/include.json | 4 ++++ tests/qapi-schema/include.out | 8 ++++++++ tests/qapi-schema/include/include.json | 7 +++++++ tests/qapi-schema/include_loop.exit | 1 + tests/qapi-schema/include_loop.json | 1 + tests/qapi-schema/include_loop.out | 1 + 10 files changed, 54 insertions(+), 2 deletions(-) create mode 100644 tests/qapi-schema/include.err create mode 100644 tests/qapi-schema/include.exit create mode 100644 tests/qapi-schema/include.json create mode 100644 tests/qapi-schema/include.out create mode 100644 tests/qapi-schema/include/include.json create mode 100644 tests/qapi-schema/include_loop.err create mode 100644 tests/qapi-schema/include_loop.exit create mode 100644 tests/qapi-schema/include_loop.json create mode 100644 tests/qapi-schema/include_loop.out