Re: [Qemu-devel] [PATCH RFC v2 26/47] qapi-types: Convert to QAPISchemaVisitor, fixing flat unions

[Eric Blake]

On 07/01/2015 02:22 PM, Markus Armbruster wrote:
> Fixes flat unions to get the base's base members.  Test case is from
> commit 2fc0043, in qapi-schema-test.json:
> 

> 
> Flat union visitors remain broken.  They'll be fixed next.

Sadly, the generated files had a huge diffstat, making it very hard to
determine if this change is sane:

 qapi-types.c                        | 1412 ++++++-------
 qapi-types.h                        | 3705
++++++++++++++++++++----------------
 qga/qapi-generated/qga-qapi-types.c |  110 -
 qga/qapi-generated/qga-qapi-types.h |  542 +++--
 4 files changed, 3208 insertions(+), 2561 deletions(-)

At first, it looks easy, as in:

qapi-types.c:
+const int BlockdevRef_qtypes[QTYPE_MAX] = {
...
 const char *const BlockdevRefKind_lookup[] = {
...
-const int BlockdevRef_qtypes[QTYPE_MAX] = {
...

(that is, the new visitor outputs the two arrays in a different order -
I can live with that).

Then it gets a bit crazy when using normal diff algorithms:

-void qapi_free_int32List(int32List *obj)
+void qapi_free_ACPIOSTInfo(ACPIOSTInfo *obj)

where declarations are rearranged (so the previous commit, which
attempted to sort declarations to make this one easier, did not quite
succeed).

Even in qapi-types.h things were rearranged:


-#ifndef QAPI_TYPES_BUILTIN_STRUCT_DECL
-#define QAPI_TYPES_BUILTIN_STRUCT_DECL
+#ifndef QAPI_TYPES_BUILTIN
+#define QAPI_TYPES_BUILTIN


-typedef struct int32List {
+typedef struct boolList boolList;
+
+struct boolList {

I can understand the minor change in #ifdef name, and even the
separation between typedef and struct declaration (even though the old
approach of doing it all at once works).  But the overall diff in the
generated files would be easier to review if done in stages, and either
make 25/47 sort closer to what this patch does, or add yet more
prerequisite patches that further tweak sorting.  Ideally, this patch
will be a lot easier to review if the generated code is much closer to
the pre-patch version (that is, separating sorting changes from other
changes).  On the other hand, yes, it's kind of a pain to patch old code
to do things differently just before throwing it away with the new code
in its place, so it becomes a judgment call of how confident we want to
be that the new implementation isn't breaking things.

I was able to make a bit more sense of things by using git's 'diff
patience' algorithm, which showed things more as code motion rather than
one-or-two-line changes to lots of common boilerplate, but even that
diffstat is still big:

 qapi-types1.c | 2030 +++++++++++++++----------------
 qapi-types1.h | 3707
+++++++++++++++++++++++++++++++++------------------------
 2 files changed, 3148 insertions(+), 2589 deletions(-)

That said, I'll still at least review this code by inspection, and
things still compile fine, so although I'm reluctant to give R-b while
the patch is in RFC stage (because I didn't want to take the time to be
certain the results are the same amidst so much churn), they are mostly
sane.

> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  scripts/qapi-types.py                   | 268 
> +++++++++++++-------------------
>  tests/qapi-schema/qapi-schema-test.json |   4 +-
>  2 files changed, 114 insertions(+), 158 deletions(-)
> 
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index a48ad9c..d6185c6 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -2,88 +2,67 @@
>  # QAPI types generator
>  #
>  # Copyright IBM, Corp. 2011
> +# Copyright (c) 2013-2015 Red Hat Inc.
>  #
>  # Authors:
>  #  Anthony Liguori <address@hidden>
> +#  Markus Armbruster <address@hidden>
>  #
>  # This work is licensed under the terms of the GNU GPL, version 2.
>  # See the COPYING file in the top-level directory.
>  
> -from ordereddict import OrderedDict
>  from qapi import *
>  
> -def generate_fwd_builtin(name):
> -    return mcgen('''
> -
> -typedef struct %(name)sList {

For example, the change of splitting this into:

typedef struct %(name)sList;

struct %(name)sList {

done as a separate patch would make the generated diff of this patch
smaller.


>  
> -def generate_struct_fields(members):
> +def gen_struct_field(name, typ, optional):
>      ret = ''
>  
> -    for argname, argentry, optional in parse_args(members):
> -        if optional:
> -            ret += mcgen('''
> +    if optional:
> +        ret += mcgen('''

(sometimes it's annoying that python requires indentation changes when
changing control flow; for C files, sometimes I split a patch that
merely adds {} and indentation from another patch that changes logic, so
that the logic change isn't drowned by the reindentation)

> +class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
> +    def __init__(self):
> +        self.decl = None
> +        self.defn = None
> +        self.fwdecl = None
> +        self.fwdefn = None
> +        self.btin = None
> +    def visit_begin(self):
> +        self.decl = ''
> +        self.defn = ''
> +        self.fwdecl = ''
> +        self.fwdefn = ''
> +        self.btin = guardstart('QAPI_TYPES_BUILTIN')
> +    def visit_end(self):
> +        self.decl = self.fwdecl + self.decl
> +        self.fwdecl = None
> +        self.defn = self.fwdefn + self.defn
> +        self.fwdefn = None
> +        # To avoid header dependency hell, we always generate
> +        # declarations for built-in types in our header files and
> +        # simply guard them.
> +        self.btin += guardend('QAPI_TYPES_BUILTIN')
> +        self.decl = self.btin + self.decl
> +        self.btin = None
> +        # Doesn't work for cases where we link in multiple objects
> +        # that have the functions defined, so generate them only with
> +        # option -b (do_builtins).

Does this comment...

> +    def _gen_type_cleanup(self, name):
> +        self.decl += generate_type_cleanup_decl(name)
> +        self.defn += generate_type_cleanup(name)
> +    def visit_enum_type(self, name, info, values):
> +        self.fwdecl += generate_enum(name, values)
> +        self.fwdefn += generate_enum_lookup(name, values)
> +    def visit_array_type(self, name, info, element_type):
> +        if isinstance(element_type, QAPISchemaBuiltinType):
> +            self.btin += gen_fwd_object_or_array(name)
> +            self.btin += gen_array(name, element_type)
> +            self.btin += generate_type_cleanup_decl(name)
> +            if do_builtins:
> +                self.defn += generate_type_cleanup(name)

...fit better here?

> +        else:
> +            self.fwdecl += gen_fwd_object_or_array(name)
> +            self.decl += gen_array(name, element_type)
> +            self._gen_type_cleanup(name)
> +    def visit_object_type(self, name, info, base, members, variants):
> +        if info:

So to make sure I understand, this ensures that implicit types (those
handled merely by parameters in a function or as members of a struct)
have no declarations required in the -types files.

> +            self.fwdecl += gen_fwd_object_or_array(name)
> +            if variants:
> +                self.decl += gen_union(name, base, variants)

Is it worth 'assert not members' at this point?

> +            else:
> +                self.decl += gen_struct(name, base, members)
> +            self._gen_type_cleanup(name)
> +    def visit_alternate_type(self, name, info, variants):
> +        self.fwdecl += gen_fwd_object_or_array(name)
> +        self.fwdefn += gen_alternate_qtypes(name, variants)
> +        self.decl += gen_union(name, None, variants)
> +        self.decl += gen_alternate_qtypes_decl(name)
> +        self._gen_type_cleanup(name)
> +

The visitor looks reasonable from inspection review.

>  do_builtins = False
>  
>  (input_file, output_dir, do_c, do_h, prefix, opts) = \
> @@ -325,77 +348,10 @@ fdecl.write(mcgen('''
>  #include <stdint.h>
>  '''))
>  
> -exprs = QAPISchema(input_file).get_exprs()
> -
> -fdecl.write(guardstart("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
> -for typename in builtin_types.keys():
> -    fdecl.write(generate_fwd_builtin(typename))
> -fdecl.write(guardend("QAPI_TYPES_BUILTIN_STRUCT_DECL"))

This is a place where adding sorting in 25/47 (or a separate patch)
would make the impact on generated code from this patch smaller.

> -
> -for expr in exprs:
> -    ret = ""
> -    if expr.has_key('struct'):
> -        ret += generate_fwd_struct(expr['struct'])
> -    elif expr.has_key('enum'):
> -        ret += generate_enum(expr['enum'], expr['data'])
> -        ret += generate_fwd_enum_struct(expr['enum'])
> -        fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
> -    elif expr.has_key('union'):
> -        ret += generate_fwd_struct(expr['union'])
> -        enum_define = discriminator_find_enum_define(expr)
> -        if not enum_define:
> -            ret += generate_enum('%sKind' % expr['union'], 
> expr['data'].keys())
> -            fdef.write(generate_enum_lookup('%sKind' % expr['union'],
> -                                            expr['data'].keys()))
> -    elif expr.has_key('alternate'):
> -        ret += generate_fwd_struct(expr['alternate'])
> -        ret += generate_enum('%sKind' % expr['alternate'], 
> expr['data'].keys())
> -        fdef.write(generate_enum_lookup('%sKind' % expr['alternate'],
> -                                        expr['data'].keys()))

Tweaking the order of _qtypes[] vs. _lookup[] here in a pre-req patch
would also help review of the diff generated by this patch.

> -        fdef.write(generate_alternate_qtypes(expr))
> -    else:
> -        continue
> -    fdecl.write(ret)
> -
> -# to avoid header dependency hell, we always generate declarations
> -# for built-in types in our header files and simply guard them
> -fdecl.write(guardstart("QAPI_TYPES_BUILTIN_CLEANUP_DECL"))
> -for typename in builtin_types.keys():
> -    fdecl.write(generate_type_cleanup_decl(typename + "List"))
> -fdecl.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DECL"))
> -
> -# ...this doesn't work for cases where we link in multiple objects that
> -# have the functions defined, so we use -b option to provide control
> -# over these cases
> -if do_builtins:
> -    for typename in builtin_types.keys():
> -        fdef.write(generate_type_cleanup(typename + "List"))

Another iteration worth sorting in an earlier patch.

> -
> -for expr in exprs:
> -    ret = ""
> -    if expr.has_key('struct'):
> -        ret += generate_struct(expr) + "\n"
> -        ret += generate_type_cleanup_decl(expr['struct'] + "List")
> -        fdef.write(generate_type_cleanup(expr['struct'] + "List"))
> -        ret += generate_type_cleanup_decl(expr['struct'])
> -        fdef.write(generate_type_cleanup(expr['struct']))

I think part of the large size of the generated diff comes from whether
array fooList types are emitted in a different order via the visitor.

> -    elif expr.has_key('union'):
> -        ret += generate_union(expr, 'union') + "\n"
> -        ret += generate_type_cleanup_decl(expr['union'] + "List")
> -        fdef.write(generate_type_cleanup(expr['union'] + "List"))
> -        ret += generate_type_cleanup_decl(expr['union'])
> -        fdef.write(generate_type_cleanup(expr['union']))
> -    elif expr.has_key('alternate'):
> -        ret += generate_union(expr, 'alternate') + "\n"
> -        ret += generate_type_cleanup_decl(expr['alternate'] + "List")
> -        fdef.write(generate_type_cleanup(expr['alternate'] + "List"))
> -        ret += generate_type_cleanup_decl(expr['alternate'])
> -        fdef.write(generate_type_cleanup(expr['alternate']))
> -    elif expr.has_key('enum'):
> -        ret += "\n" + generate_type_cleanup_decl(expr['enum'] + "List")
> -        fdef.write(generate_type_cleanup(expr['enum'] + "List"))
> -    else:
> -        continue
> -    fdecl.write(ret)
> +schema = QAPISchema(input_file)
> +gen = QAPISchemaGenTypeVisitor()
> +schema.visit(gen)
> +fdef.write(gen.defn)
> +fdecl.write(gen.decl)
>  
>  close_output(fdef, fdecl)
> diff --git a/tests/qapi-schema/qapi-schema-test.json 
> b/tests/qapi-schema/qapi-schema-test.json
> index a9e5aab..257b4d4 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -39,8 +39,8 @@
>    'data': { 'value1' : 'UserDefA',
>              'value2' : 'UserDefB',
>              'value3' : 'UserDefB' } }
> -# FIXME generated struct UserDefFlatUnion has members for direct base
> -# UserDefUnionBase, but lacks members for indirect base UserDefZero
> +# FIXME generated visit_type_UserDefFlatUnion_fields() fails to visit
> +# members of indirect base UserDefZero
>  
>  { 'struct': 'UserDefUnionBase',
>    'base': 'UserDefZero',
> 

Overall, moving in the right direction.

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

signature.asc
Description: OpenPGP digital signature