qapi: Top-level type reference for command definitions
cover qapi: Top-level type reference for command definitions
Kevin Wolf (3):
qapi.py: Move common code to evaluate()
qapi.py: Allow top-level type reference for command definitions
qapi-schema: Use BlockdevSnapshot type for blockdev-snapshot-sync
qapi-schema.json | 3 +-- scripts/qapi.py | 43 +++++++++++++++++++++++++++++-------------- 2 files changed, 30 insertions(+), 16 deletions(-)
Signature · 1 line
1.8.1.4
PATCH 1/3 qapi.py: Move common code to evaluate()
Don't duplicate more code than is really necessary.
--- scripts/qapi.py | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-)
scripts/qapi.py +10 -14
@@ -76,12 +76,18 @@ def parse(tokens): return tokens[0], tokens[1:] def evaluate(string): - return parse(map(lambda x: x, tokenize(string)))[0] + expr_eval = parse(map(lambda x: x, tokenize(string)))[0] + + if expr_eval.has_key('enum'): + add_enum(expr_eval['enum']) + elif expr_eval.has_key('union'): + add_enum('%sKind' % expr_eval['union']) + + return expr_eval def parse_schema(fp): exprs = [] expr = '' - expr_eval = None for line in fp: if line.startswith('#') or line == '\n':
@@ -90,23 +96,13 @@ def parse_schema(fp): if line.startswith(' '): expr += line elif expr: - expr_eval = evaluate(expr) - if expr_eval.has_key('enum'): - add_enum(expr_eval['enum']) - elif expr_eval.has_key('union'): - add_enum('%sKind' % expr_eval['union']) - exprs.append(expr_eval) + exprs.append(evaluate(expr)) expr = line else: expr += line if expr: - expr_eval = evaluate(expr) - if expr_eval.has_key('enum'): - add_enum(expr_eval['enum']) - elif expr_eval.has_key('union'): - add_enum('%sKind' % expr_eval['union']) - exprs.append(expr_eval) + exprs.append(evaluate(expr)) return exprs
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 1/3] qapi.py: Move common code to evaluate()
Quoted 14 lines · Kevin Wolf
Don't duplicate more code than is really necessary.
Signed-off-by Kevin Wolf <[email protected]>--- scripts/qapi.py | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-)scripts/qapi.py +0
-0@@ -76,12 +76,18 @@ def parse(tokens): return tokens[0], tokens[1:]
scripts/qapi.py def parse(tokens): · line 86
Quoted 10 lines
scripts/qapi.py +8
-1def evaluate(string): - return parse(map(lambda x: x, tokenize(string)))[0] + expr_eval = parse(map(lambda x: x, tokenize(string)))[0] + + if expr_eval.has_key('enum'): + add_enum(expr_eval['enum']) + elif expr_eval.has_key('union'): + add_enum('%sKind' % expr_eval['union']) + + return expr_eval
add_enum() adds stuff to a global table, so I don't really like the idea of pushing it further down the stack (however inconsequential it may be in this case...)
I think the real reason we have repetition is the extra 'flush' we do after the for loop below to handle the last expression we read from a schema, which leads to a repeat of one of the clauses in the loop body. I've wanted to get rid of it a few times in the past so this is probably a good opportunity to do so.
Could you try adapting something like the following to keep the global stuff in parse_schema()?
def get_expr(fp):
expr = ""
for line in fp:
if line.startswith('#') or line == '\n':
continue
if line.startswith(' '):
expr += line
elif expr:
yield expr
expr = line
else:
expr += line
yield exprdef parse_schema(fp):
exprs = []
expr_eval
for expr in get_expr(fp):
expr_eval = evaluate(expr)
if expr_eval.has_key('enum'):
add_enum(expr_eval['enum'])
...
return exprsQuoted 38 lines
scripts/qapi.py +2
-13def parse_schema(fp): exprs = [] expr = '' - expr_eval = None for line in fp: if line.startswith('#') or line == '\n':@@ -90,23 +96,13 @@ def parse_schema(fp): if line.startswith(' '): expr += line elif expr: - expr_eval = evaluate(expr) - if expr_eval.has_key('enum'): - add_enum(expr_eval['enum']) - elif expr_eval.has_key('union'): - add_enum('%sKind' % expr_eval['union']) - exprs.append(expr_eval) + exprs.append(evaluate(expr)) expr = line else: expr += line if expr: - expr_eval = evaluate(expr) - if expr_eval.has_key('enum'): - add_enum(expr_eval['enum']) - elif expr_eval.has_key('union'): - add_enum('%sKind' % expr_eval['union']) - exprs.append(expr_eval) + exprs.append(evaluate(expr)) return exprsSignature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 1/3] qapi.py: Move common code to evaluate()
Am 21.06.2013 um 18:39 hat mdroth geschrieben:
Quoted 15 lines · Kevin Wolf
Quoted 14 lines
Don't duplicate more code than is really necessary.
Signed-off-by Kevin Wolf <[email protected]>--- scripts/qapi.py | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-)scripts/qapi.py +0
-0@@ -76,12 +76,18 @@ def parse(tokens): return tokens[0], tokens[1:]
Quoted 14 lines
scripts/qapi.py def parse(tokens): · line 86
Quoted 10 lines
scripts/qapi.py +8
-1def evaluate(string): - return parse(map(lambda x: x, tokenize(string)))[0] + expr_eval = parse(map(lambda x: x, tokenize(string)))[0] + + if expr_eval.has_key('enum'): + add_enum(expr_eval['enum']) + elif expr_eval.has_key('union'): + add_enum('%sKind' % expr_eval['union']) + + return expr_eval
add_enum()adds stuff to a global table, so I don't really like the idea of pushing it further down the stack (however inconsequential it may be in this case...)
As if it made any difference if it's one level more or less... It's already buried in the "library".
Quoted 8 lines
I think the real reason we have repetition is the extra 'flush' we do after the for loop below to handle the last expression we read from a schema, which leads to a repeat of one of the clauses in the loop body. I've wanted to get rid of it a few times in the past so this is probably a good opportunity to do so.
Could you try adapting something like the following to keep the global stuff in
parse_schema()?
I don't think there's any value in keeping the global stuff in parse_schema() (or, to be precise, in functions directly called by parse_schema()) , and I found it quite nice to have evaluate() actually evaluate something instead of just parsing it.
But I agree that duplicating code, as small as it may be now, isn't nice, so I can try to use the get_expr() thing. I just would prefer to move the evaluation to evaluate() anyway.
Kevin
Re: [Qemu-devel] [PATCH 1/3] qapi.py: Move common code to evaluate()
Quoted 43 lines · Kevin Wolf
Am 21.06.2013 um 18:39 hat mdroth geschrieben:
Quoted 15 lines · Kevin Wolf
Quoted 14 lines
Don't duplicate more code than is really necessary.
Signed-off-by Kevin Wolf <[email protected]>--- scripts/qapi.py | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-)scripts/qapi.py +0
-0@@ -76,12 +76,18 @@ def parse(tokens): return tokens[0], tokens[1:]Quoted 14 lines
scripts/qapi.py def parse(tokens): · line 86
Quoted 10 lines
scripts/qapi.py +8
-1def evaluate(string): - return parse(map(lambda x: x, tokenize(string)))[0] + expr_eval = parse(map(lambda x: x, tokenize(string)))[0] + + if expr_eval.has_key('enum'): + add_enum(expr_eval['enum']) + elif expr_eval.has_key('union'): + add_enum('%sKind' % expr_eval['union']) + + return expr_eval
add_enum()adds stuff to a global table, so I don't really like the idea of pushing it further down the stack (however inconsequential it may be in this case...)As if it made any difference if it's one level more or less... It's already buried in the "library".
Quoted 8 lines
I think the real reason we have repetition is the extra 'flush' we do after the for loop below to handle the last expression we read from a schema, which leads to a repeat of one of the clauses in the loop body. I've wanted to get rid of it a few times in the past so this is probably a good opportunity to do so.
Could you try adapting something like the following to keep the global stuff in
parse_schema()?
Quoted 8 lines
I don't think there's any value in keeping the global stuff in
parse_schema()(or, to be precise, in functions directly called byparse_schema()) , and I found it quite nice to haveevaluate()actually evaluate something instead of just parsing it.But I agree that duplicating code, as small as it may be now, isn't nice, so I can try to use the
get_expr()thing. I just would prefer to move the evaluation toevaluate()anyway.
evaluate() is basically the qapi version of python's eval(), but with specially handling for JSON objects that retains the original ordering of the keys by mapping JSON strings to OrderedDicts instead of dicts. If it weren't for that requirement we'd be call eval() in place of evaluate() and drop all our awesome parsing/tokenizing code.
It's still not a huge deal, but given an alternative that's also beneficial in other ways I think we should avoid it.
Quoted 3 lines
Kevin
PATCH 2/3 qapi.py: Allow top-level type reference for command definitions
In reply to Kevin WolfIf 'data' for a command definition isn't a dict, but a string, it is taken as a (struct) type name and the fields of this struct are directly used as parameters.
This is useful for transactionable commands that can use the same type definition for both the transaction action and the arguments of the standalone command.
--- scripts/qapi.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
scripts/qapi.py +19 -0
@@ -82,6 +82,8 @@ def evaluate(string): add_enum(expr_eval['enum']) elif expr_eval.has_key('union'): add_enum('%sKind' % expr_eval['union']) + elif expr_eval.has_key('type'): + add_struct(expr_eval) return expr_eval
@@ -107,6 +109,11 @@ def parse_schema(fp): return exprs def parse_args(typeinfo): + if isinstance(typeinfo, basestring): + struct = find_struct(typeinfo) + assert struct != None + typeinfo = struct['data'] + for member in typeinfo: argname = member argentry = typeinfo[member]
@@ -176,6 +183,18 @@ def type_name(name): return name enum_types = [] +struct_types = [] + +def add_struct(definition): + global struct_types + struct_types.append(definition) + +def find_struct(name): + global struct_types + for struct in struct_types: + if struct['type'] == name: + return struct + return None def add_enum(name): global enum_types
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 2/3] qapi.py: Allow top-level type reference for command definitions
Quoted 3 lines · Kevin Wolf
If 'data' for a command definition isn't a dict, but a string, it is taken as a (struct) type name and the fields of this struct are directly used as parameters.
I like it! I suspect it may cause conflicts with Amos' work on adding introspection, but it is still worth doing.
scripts/qapi.py def parse_schema(fp): · line 116
Quoted 7 lines
def
parse_args(typeinfo):scripts/qapi.py +5
-0+ if isinstance(typeinfo, basestring): + struct = find_struct(typeinfo) + assert struct != None + typeinfo = struct['data'] +
Does this mean that .json files must be written in topological order (in that we can't use 'data':'Type' unless 'Type' was declared earlier in the file)? As the .json file gets larger, I've been wondering if enforcing alphabetical ordering would make it easier to manage; but if topological sorting is required, alphabetical sorting might not always be possible.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 2/3] qapi.py: Allow top-level type reference for command definitions
Am 21.06.2013 um 12:30 hat Eric Blake geschrieben:
Quoted 16 lines · Kevin Wolf
Quoted 3 lines
If 'data' for a command definition isn't a dict, but a string, it is taken as a (struct) type name and the fields of this struct are directly used as parameters.
I like it! I suspect it may cause conflicts with Amos' work on adding introspection, but it is still worth doing.
scripts/qapi.py def parse_schema(fp): · line 116
Quoted 7 lines
def
parse_args(typeinfo):scripts/qapi.py +5
-0+ if isinstance(typeinfo, basestring): + struct = find_struct(typeinfo) + assert struct != None + typeinfo = struct['data'] +
Quoted 3 lines
Does this mean that .json files must be written in topological order (in that we can't use 'data':'Type' unless 'Type' was declared earlier in the file)?
No, you have effectively two passes: First the qapi.py function reads in the file and adds any structs, enums and unions to the respective lists and returns something like an array of all objects found. The generator scripts then filter that array for the type of objects they want (e.g. only commands, or only types) and indirectly call parse_args(). At this point all types have already been registered.
Quoted 4 lines
As the .json file gets larger, I've been wondering if enforcing alphabetical ordering would make it easier to manage; but if topological sorting is required, alphabetical sorting might not always be possible.
Yeah, possibly. Another thing I was considering is introducing include files, so that I could for example specify { 'include':
'block/qapi-schema.json' } and have all block-related things separated in this file.
Kevin
Re: [Qemu-devel] [PATCH 2/3] qapi.py: Allow top-level type reference for command definitions
Quoted 9 lines · Kevin Wolf
Quoted 4 lines
As the .json file gets larger, I've been wondering if enforcing alphabetical ordering would make it easier to manage; but if topological sorting is required, alphabetical sorting might not always be possible.
Yeah, possibly. Another thing I was considering is introducing include files, so that I could for example specify { 'include':
'block/qapi-schema.json' } and have all block-related things separated in this file.
Patches for include files have already been proposed:
https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg04078.html
Maybe it's worth isolating that from its larger (and now abandoned?) series into an independent smaller series?
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 3/3 qapi-schema: Use BlockdevSnapshot type for blockdev-snapshot-sync
In reply to Kevin WolfWe don't have to duplicate the definition any more now that we may refer to a type instead.
--- qapi-schema.json | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
qapi-schema.json +1 -2
@@ -1669,8 +1669,7 @@ # Since 0.14.0 ## { 'command': 'blockdev-snapshot-sync', - 'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str', - '*mode': 'NewImageMode'} } + 'data': 'BlockdevSnapshot' } ## # @human-monitor-command:
Signature · 1 line
1.8.1.4
Re: [Qemu-devel] [PATCH 3/3] qapi-schema: Use BlockdevSnapshot type for blockdev-snapshot-sync
Quoted 13 lines · Kevin Wolf
We don't have to duplicate the definition any more now that we may refer to a type instead.
Signed-off-by Kevin Wolf <[email protected]>--- qapi-schema.json | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)qapi-schema.json +0
-0
qapi-schema.json line 1672
Quoted 6 lines
qapi-schema.json +1
-2@@ -1669,8 +1669,7 @@ # Since 0.14.0 ## { 'command': 'blockdev-snapshot-sync', - 'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str', - '*mode': 'NewImageMode'} } + 'data': 'BlockdevSnapshot' }
There's still duplication of the doc comments, but as we don't have anything parsing doc comments to convert them into a more usable form yet, that can be something we worry about later.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 3/3] qapi-schema: Use BlockdevSnapshot type for blockdev-snapshot-sync
Quoted 9 lines · On Fri, 21 Jun 2013 11:31:28 +0100 Eric Blake <[email protected]> wrote:
Quoted 8 lines · Kevin Wolf
We don't have to duplicate the definition any more now that we may refer to a type instead.
Signed-off-by Kevin Wolf <[email protected]>--- qapi-schema.json | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Quoted 15 lines
qapi-schema.json line 1672
Quoted 11 lines
qapi-schema.json +1
-2@@ -1669,8 +1669,7 @@ # Since 0.14.0 ## { 'command': 'blockdev-snapshot-sync', - 'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str', - '*mode': 'NewImageMode'} } + 'data': 'BlockdevSnapshot' }There's still duplication of the doc comments, but as we don't have anything parsing doc comments to convert them into a more usable form yet, that can be something we worry about later.
Let's document it now.
Series looks good otherwise.
Quoted 8 lines · Kevin Wolf
Kevin Wolf (3):
qapi.py: Move common code toevaluate()
qapi.py: Allow top-level type reference for command definitions
qapi-schema: Use BlockdevSnapshot type for blockdev-snapshot-syncqapi-schema.json | 3 +-- scripts/qapi.py | 43 +++++++++++++++++++++++++++++-------------- 2 files changed, 30 insertions(+), 16 deletions(-)
Nice, I'll use this for drive-backup.
Thanks,
Stefan
Quoted 8 lines · Kevin Wolf
Kevin Wolf (3):
qapi.py: Move common code toevaluate()
qapi.py: Allow top-level type reference for command definitions
qapi-schema: Use BlockdevSnapshot type for blockdev-snapshot-syncqapi-schema.json | 3 +-- scripts/qapi.py | 43 +++++++++++++++++++++++++++++-------------- 2 files changed, 30 insertions(+), 16 deletions(-)
Series: Reviewed-by: Eric Blake <[email protected]>
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org