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

[Markus Armbruster]

Benoît Canet <address@hidden> 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 <address@hidden>
> ---
>  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 :)

[...]