Re: [Qemu-devel] [PATCH V2 3/3] qapi: Create an include directive for use in the JSON description files.

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3608 bytes --]

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').

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]