[Qemu-devel] [PATCH 0/3] qapi: Top-level type reference for command definitions

[Qemu-devel] [PATCH 0/3] qapi: Top-level type reference for command definitions

[Kevin Wolf]

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(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/3] qapi.py: Move common code to evaluate()

[Kevin Wolf]

Don't duplicate more code than is really necessary.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 scripts/qapi.py | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 02ad668..3a64769 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -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
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 1/3] qapi.py: Move common code to evaluate()

[mdroth]

On Wed, Jun 19, 2013 at 06:28:05PM +0200, Kevin Wolf wrote:
> Don't duplicate more code than is really necessary.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  scripts/qapi.py | 24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 02ad668..3a64769 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -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

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 expr

def 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 exprs

> 
>  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
> 
> -- 
> 1.8.1.4
> 
> 

Re: [Qemu-devel] [PATCH 1/3] qapi.py: Move common code to evaluate()

[Kevin Wolf]

Am 21.06.2013 um 18:39 hat mdroth geschrieben:
> On Wed, Jun 19, 2013 at 06:28:05PM +0200, Kevin Wolf wrote:
> > Don't duplicate more code than is really necessary.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  scripts/qapi.py | 24 ++++++++++--------------
> >  1 file changed, 10 insertions(+), 14 deletions(-)
> > 
> > diff --git a/scripts/qapi.py b/scripts/qapi.py
> > index 02ad668..3a64769 100644
> > --- a/scripts/qapi.py
> > +++ b/scripts/qapi.py
> > @@ -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
> 
> 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".

> 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()

[mdroth]

On Mon, Jun 24, 2013 at 05:17:35PM +0200, Kevin Wolf wrote:
> Am 21.06.2013 um 18:39 hat mdroth geschrieben:
> > On Wed, Jun 19, 2013 at 06:28:05PM +0200, Kevin Wolf wrote:
> > > Don't duplicate more code than is really necessary.
> > > 
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > ---
> > >  scripts/qapi.py | 24 ++++++++++--------------
> > >  1 file changed, 10 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/scripts/qapi.py b/scripts/qapi.py
> > > index 02ad668..3a64769 100644
> > > --- a/scripts/qapi.py
> > > +++ b/scripts/qapi.py
> > > @@ -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
> > 
> > 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".
> 
> > 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.

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.

> 
> Kevin
> 

[Qemu-devel] [PATCH 2/3] qapi.py: Allow top-level type reference for command definitions

[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.

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.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 scripts/qapi.py | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 3a64769..e151659 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -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
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 2/3] qapi.py: Allow top-level type reference for command definitions

[Eric Blake]

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

On 06/19/2013 05:28 PM, Kevin Wolf wrote:
> 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.

>  
>  def parse_args(typeinfo):
> +    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.

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH 2/3] qapi.py: Allow top-level type reference for command definitions

[Kevin Wolf]

Am 21.06.2013 um 12:30 hat Eric Blake geschrieben:
> On 06/19/2013 05:28 PM, Kevin Wolf wrote:
> > 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.
> 
> >  
> >  def parse_args(typeinfo):
> > +    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)?

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.

> 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

[Eric Blake]

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

On 06/21/2013 12:00 PM, Kevin Wolf wrote:

>> 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?

-- 
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: 621 bytes --]

[Qemu-devel] [PATCH 3/3] qapi-schema: Use BlockdevSnapshot type for blockdev-snapshot-sync

[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 <kwolf@redhat.com>
---
 qapi-schema.json | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index a80ee40..ee19d6a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -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:
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 3/3] qapi-schema: Use BlockdevSnapshot type for blockdev-snapshot-sync

[Eric Blake]

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

On 06/19/2013 05:28 PM, Kevin Wolf wrote:
> We don't have to duplicate the definition any more now that we may refer
> to a type instead.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi-schema.json | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index a80ee40..ee19d6a 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -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.

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH 3/3] qapi-schema: Use BlockdevSnapshot type for blockdev-snapshot-sync

[Luiz Capitulino]

On Fri, 21 Jun 2013 11:31:28 +0100
Eric Blake <eblake@redhat.com> wrote:

> On 06/19/2013 05:28 PM, Kevin Wolf wrote:
> > We don't have to duplicate the definition any more now that we may refer
> > to a type instead.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  qapi-schema.json | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index a80ee40..ee19d6a 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -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.

Re: [Qemu-devel] [PATCH 0/3] qapi: Top-level type reference for command definitions

[Stefan Hajnoczi]

On Wed, Jun 19, 2013 at 06:28:04PM +0200, Kevin Wolf wrote:
> 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(-)

Nice, I'll use this for drive-backup.

Thanks,
Stefan

Re: [Qemu-devel] [PATCH 0/3] qapi: Top-level type reference for command definitions

[Eric Blake]

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

On 06/19/2013 05:28 PM, Kevin Wolf wrote:
> 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(-)

Series: Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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: 621 bytes --]