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

[Benoît Canet]

The Thursday 27 Mar 2014 à 19:07:48 (+0100), Markus Armbruster wrote :
> 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 :)

Don't kid me you'll need it for the next QEMU logo :)

Best regards

Benoît

> 
> [...]