Re: [Qemu-devel] [PATCH v4 2/5] qapi: add qapi-introspect.py code generator

[Eric Blake]

On 01/23/2014 07:46 AM, Amos Kong wrote:
> This is a code generator for qapi introspection. It will parse
> qapi-schema.json, extend schema definitions and generate a schema
> table with metadata, it references to the new structs which we used
> to describe dynamic data structs.  The metadata will help C code to
> allocate right structs and provide useful information to management
> to checking suported feature and QMP commandline detail. The schema

s/suported/supported/

> table will be saved to qapi-introspect.h.
> 
> The $(prefix) is used to as a namespace to keep the generated code
> from one schema/code-generation separated from others so code and
> be generated from multiple schemas with clobbering previously
> created code.
> 
> Signed-off-by: Amos Kong <address@hidden>
> ---
>  .gitignore                      |   1 +
>  Makefile                        |   5 +-
>  docs/qmp-full-introspection.txt |  17 ++++
>  scripts/qapi-introspect.py      | 172 
> ++++++++++++++++++++++++++++++++++++++++

I am NOT a python expert.  But what I _can_ do is apply this patch and
review the generated code for sanity.

>  4 files changed, 194 insertions(+), 1 deletion(-)
>  create mode 100644 scripts/qapi-introspect.py
> 

> +++ b/docs/qmp-full-introspection.txt
> @@ -42,3 +42,20 @@ types.
>  
>  'anonymous-struct' will be used to describe arbitrary structs
>  (dictionary, list or string).
> +
> +== Avoid dead loop in recursive extending ==

I'm still not convinced whether recursive extending is necessary.

Let's step back and look at the big picture: if libvirt asks for the
ENTIRE schema in one go, then it would be nice to have the
representation as compact as possible (minimal time sending data across
the wire, minimal memory consumed).  Libvirt can then create its own
hash table of type references encountered as it consumes the JSON, and
do its own recursive lookups by reading from its hash table on an
as-needed basis.  Recursive expansion avoids the need to look up type
references, but explodes the size of the data sent over the wire.

On the other hand, if we support filtering by one type, then I agree
that getting all details about that type in one command is nicer than
having to issue a command, notice unknown type references, then issue
followup commands to learn about them.  So in this regards, having qemu
do the expansion minimizes back-and-forth traffic.  BUT, should the
expansion be inlined (ie. the return is an array of 1 type, but that
type contains several further layers of JSON giving the full expansion
of every type referenced), or is it smarter to do the expansion via
returning multiple types even though libvirt only asked about one (as an
example, return an array of 10 types, where the first array entry is the
requested type, and the remaining 9 types fill out the type references
mentioned by the first array entry).

> +
> +We have four types (ImageInfo, BlockStats, PciDeviceInfo, ObjectData)
> +that uses themself in their own define data directly or indirectly,

s/uses/themself/use themselves/

> +we will not repeatedly extend them to avoid dead loop.

s/dead loop/infinite loop/

> +
> +We use a 'parents List' to record the visit path, type name of each
> +extended node will be saved to the List.
> +
> +Append type name to the list before extending, and remove type name
> +from the list after extending.
> +
> +If the type name is already extended in parents List, we won't extend
> +it repeatedly for avoiding dead loop.
> +
> +'recursive' indicates if the type is extended or not.

Ah, now we get to the definition of an extended type that I was asking
about in 1/5.

> diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
> new file mode 100644
> index 0000000..03179fa
> --- /dev/null
> +++ b/scripts/qapi-introspect.py
> @@ -0,0 +1,172 @@
> +#
> +# QAPI introspection info generator
> +#
> +# Copyright (C) 2014 Red Hat, Inc.
> +#
> +# Authors:
> +#  Amos Kong <address@hidden>
> +#
> +# This work is licensed under the terms of the GNU GPLv2.

Any reason you can't use GPLv2+ (that is, use the "or later" clause)?
(Red Hat already has blanket approval for any contributions from
employees to be relicensed to GPLv2+, but if you copied code from other
files with a tighter license and non-RH contributor, it's harder to use
that blanket approval, so it's easier to ask you now up front.)

> +fdecl.write(mcgen('''
> +/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
> +
> +/*
> + * Head file to store parsed information of QAPI schema
> + *
> + * Copyright (C) 2014 Red Hat, Inc.
> + *
> + * Authors:
> + *  Amos Kong <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or 
> later.

Or even license your .py under LGPLv2+, since the generated code is
under that license?

> +def extend_schema(expr, parents=[], member=True):
> +    ret = {}
> +    recu = 'False'
> +    name = ""
> +
> +    if type(expr) is OrderedDict:
> +        if not member:
> +            e = expr.popitem(last=False)
> +            typ = e[0]
> +            name = e[1]
> +        else:
> +            typ = "anonymous-struct"
> +
> +        if typ == 'enum':
> +            for key in expr.keys():
> +                ret[key] = expr[key]
> +        else:
> +            ret = {}
> +            for key in expr.keys():
> +                ret[key], parents = extend_schema(expr[key], parents)

This is where I question whether extend by default is the right action.

> +
> +    elif type(expr) is list:
> +        typ = 'anonymous-struct'
> +        ret = []
> +        for i in expr:
> +            tmp, parents = extend_schema(i, parents)
> +            ret.append(tmp)
> +    elif type(expr) is str:
> +        name = expr
> +        if schema_dict.has_key(expr) and expr not in parents:
> +            parents.append(expr)
> +            typ = schema_dict[expr][1]
> +            recu = 'True'
> +            ret, parents = extend_schema(schema_dict[expr][0].copy(),
> +                                         parents, False)
> +            parents.remove(expr)
> +            ret['_obj_recursive'] = 'True'
> +            return ret, parents
> +        else:
> +            return expr, parents
> +
> +    return {'_obj_member': "%s" % member, '_obj_type': typ,
> +            '_obj_name': name, '_obj_recursive': recu,
> +            '_obj_data': ret}, parents
> +

What's with the leading underscore?

> +
> +exprs = parse_schema(sys.stdin)
> +schema_dict = {}
> +
> +for expr in exprs:
> +    if expr.has_key('type') or expr.has_key('enum') or expr.has_key('union'):
> +        e = expr.copy()
> +

We'll eventually need to cover event types.

> +        first = e.popitem(last=False)
> +        schema_dict[first[1]] = [expr.copy(), first[0]]
> +
> +fdecl.write('''const char *const qmp_schema_table[] = {
> +''')
> +
> +def convert(odict):
> +    d = {}
> +    for k, v in odict.items():
> +        if type(v) is OrderedDict:
> +            d[k] = convert(v)
> +        elif type(v) is list:
> +            l = []
> +            for j in v:
> +                if type(j) is OrderedDict:
> +                    l.append(convert(j))
> +                else:
> +                    l.append(j)
> +            d[k] = l
> +        else:
> +            d[k] = v
> +    return d
> +
> +count = 0
> +for expr in exprs:
> +    fdecl.write('''    /* %s */
> +''' % expr)
> +
> +    expr, parents = extend_schema(expr, [], False)
> +    fdecl.write('''    "%s",
> +
> +''' % convert(expr))
> +
> +fdecl.write('''    NULL };
> +
> +#endif
> +''')
> +
> +fdecl.flush()
> +fdecl.close()
> 

Like I said, I think my review will be more helpful by looking at the
generated file; I'll follow up in another email (probably tomorrow,
since it's now late for me) with more comments once I actually finish that.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature