Re: [Qemu-devel] [PATCH v4 2/3] qapi: Add a primitive to include other files from a QAPI schema file

[Lluís Vilanova]

Markus Armbruster writes:

> Lluís Vilanova <address@hidden> writes:
>> Adds the "include(...)" primitive to the syntax of QAPI schema files.
>> 
>> Signed-off-by: Lluís Vilanova <address@hidden>
>> ---
>> docs/qapi-code-gen.txt |    8 ++++++++
>> scripts/qapi.py        |   36 ++++++++++++++++++++++++++++++++++--
>> 2 files changed, 42 insertions(+), 2 deletions(-)
>> 
>> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
>> index 2e9f036..e007807 100644
>> --- a/docs/qapi-code-gen.txt
>> +++ b/docs/qapi-code-gen.txt
>> @@ -40,6 +40,14 @@ enumeration types and union types.
>> Generally speaking, types definitions should always use CamelCase for the 
>> type
>> names. Command names should be all lower case with words separated by a 
>> hyphen.
>> 
>> +The QAPI schema definitions can be modularized using the 'include' 
>> directive:
>> +
>> + include("sub-system/qapi.json")

> And now it isn't JSON anymore.

> To keep it JSON, use syntax like

>     { "include": "sub-system/qapi.json" }

> If you absolutely must make it non-JSON, you better rename the .json
> files.

> Hmm, we already are non-JSON, because we use ' instead of " for no sane
> reason.

> Our JSON parser accepts ' as an extension, to save us quoting in C
> strings.  That reason doesn't apply to .json files.

Is it a problem if they are not pure JSON? In the end, they are parsed by
qapi.py (which already knows about file syntax), and having a separate syntax
for includes makes it somewhat easier to spot when that happens.


>> +
>> +All paths are interpreted as relative to the initial input file passed to 
>> the
>> +QAPI parsing scripts.

> Really?

> Consider foo.json includes lib/a.json, which wants to include
> lib/b.json.

> foo.json:       include("lib/a.json")
> lib/a.json:     include("lib/b.json")   # relative to foo.json's directory

> Now throw in bar/bar.json including lib/a.json:

> bar/bar.json:   include("../lib/a.json")
> lib/a.json:     include("lib/b.json")   # relative to bar/ -> ENOENT

> Make it relative to the file with the include directive.

Right, sorry. The documentation was not updated after removing the explicit
include directory argument. What I'm not sure though is what would be a better
option, having current-file-relative includes, or resuscitating the old explicit
include argument.


>> +
>> +
>> === Complex types ===
>> 
>> A complex type is a dictionary containing a single key whose value is a
> [...]

> Are you aware of Wenchao Xia's "[PATCH V8 00/10] qapi script: support
> enum as discriminator and better enum name"?  I'm afraid there's a
> (semantic?) conflict.  With include files, "[PATCH V8 03/10] qapi
> script: remember line number in schema parsing" needs to remember the
> source file, too.

> Wenchao's series is likely go in first.  Perhaps you want to base on it
> now.

I was not aware of that. Since I'm managing multiple series on a single branch
with stgit (and extracting that is somewhat tedious), I will redo this series
once Xia's is merged upstream. Is the merge going to happen anytime soon?


Thanks,
  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