Re: [Qemu-devel] [PATCH RFC v4 02/32] qapi: New QAPISchema intermediate reperesentation

[Daniel P. Berrange]

On Thu, Sep 03, 2015 at 04:29:53PM +0200, Markus Armbruster wrote:
> The QAPI code generators work with a syntax tree (nested dictionaries)
> plus a few symbol tables (also dictionaries) on the side.
> 
> They have clearly outgrown these simple data structures.  There's lots
> of rummaging around in dictionaries, and information is recomputed on
> the fly.  For the work I'm going to do, I want more clearly defined
> and more convenient interfaces.
> 
> Going forward, I also want less coupling between the back-ends and the
> syntax tree, to make messing with the syntax easier.
> 
> Create a bunch of classes to represent QAPI schemata.
> 
> Have the QAPISchema initializer call the parser, then walk the syntax
> tree to create the new internal representation, and finally perform
> semantic analysis.
> 
> Shortcut: the semantic analysis still relies on existing check_exprs()
> to do the actual semantic checking.  All this code needs to move into
> the classes.  Mark as TODO.
> 
> Catching name collisions in generated code would be nice.  Mark as
> TODO.
> 
> We generate array types eagerly, even though most of them aren't used.
> Mark as TODO.
> 
> Nothing uses the new intermediate representation just yet, thus no
> change to generated files.
> 
> Signed-off-by: Markus Armbruster <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>

Reviewed-by: Daniel P. Berrange <address@hidden>

A few comments inline, but nothing worth blocking on, just style.


> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index a0165dd..c42bea1 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -747,15 +749,355 @@ def check_exprs(exprs):
>          else:
>              assert False, 'unexpected meta type'
>  
> -    return map(lambda expr_elem: expr_elem['expr'], exprs)
> -
> -def parse_schema(fname):
> -    try:
> -        schema = QAPISchemaParser(open(fname, "r"))
> -        return check_exprs(schema.exprs)
> -    except (QAPISchemaError, QAPIExprError), e:
> -        print >>sys.stderr, e
> -        exit(1)
> +    return exprs
> +
> +#
> +# Schema compiler frontend
> +#
> +
> +class QAPISchemaEntity(object):
> +    def __init__(self, name, info):
> +        assert isinstance(name, str)
> +        self.name = name
> +        self.info = info
> +    def check(self, schema):
> +        pass

>From a style POV, I'd recommend having a single blank
line before each new function def, to make things more
readable. I won't nack the patch for that, but if you
have need to rebase & post a new version of this series
I think it'd be nice to add the the extra blank lines
when defining all these classes.

Python has a nice tool called pep8 which can apply a
configurable bunch of  style checks, it is probably
worth someone wiring it up to make check in QEMU, since
we have an increasing amount of python code. An exercise
for a motivated reader.....

> +
> +class QAPISchemaType(QAPISchemaEntity):
> +    pass
> +
> +class QAPISchemaBuiltinType(QAPISchemaType):
> +    def __init__(self, name):
> +        QAPISchemaType.__init__(self, name, None)
> +
> +class QAPISchemaEnumType(QAPISchemaType):
> +    def __init__(self, name, info, values):
> +        QAPISchemaType.__init__(self, name, info)
> +        for v in values:
> +            assert isinstance(v, str)
> +        self.values = values
> +    def check(self, schema):
> +        assert len(set(self.values)) == len(self.values)
> +
> +class QAPISchemaArrayType(QAPISchemaType):
> +    def __init__(self, name, info, element_type):
> +        QAPISchemaType.__init__(self, name, info)
> +        assert isinstance(element_type, str)
> +        self.element_type_name = element_type
> +        self.element_type = None
> +    def check(self, schema):
> +        self.element_type = schema.lookup_type(self.element_type_name)
> +        assert self.element_type
> +
> +class QAPISchemaObjectType(QAPISchemaType):
> +    def __init__(self, name, info, base, local_members, variants):
> +        QAPISchemaType.__init__(self, name, info)
> +        assert base == None or isinstance(base, str)
> +        for m in local_members:
> +            assert isinstance(m, QAPISchemaObjectTypeMember)
> +        assert variants == None \
> +            or isinstance(variants, QAPISchemaObjectTypeVariants)
> +        self.base_name = base
> +        self.base = None
> +        self.local_members = local_members
> +        self.variants = variants
> +        self.members = None
> +    def check(self, schema):
> +        assert self.members != False    # not running in cycles
> +        if self.members:
> +            return
> +        self.members = False            # mark as being checked
> +        if self.base_name:
> +            self.base = schema.lookup_type(self.base_name)
> +            assert isinstance(self.base, QAPISchemaObjectType)
> +            assert not self.base.variants # not implemented
> +            self.base.check(schema)
> +            members = list(self.base.members)
> +        else:
> +            members = []
> +        seen = {}
> +        for m in members:
> +            seen[m.name] = m
> +        for m in self.local_members:
> +            m.check(schema, members, seen)
> +        if self.variants:
> +            self.variants.check(schema, members, seen)
> +        self.members = members
> +
> +class QAPISchemaObjectTypeMember(object):
> +    def __init__(self, name, typ, optional):
> +        assert isinstance(name, str)
> +        assert isinstance(typ, str)
> +        assert isinstance(optional, bool)
> +        self.name = name
> +        self.type_name = typ
> +        self.type = None
> +        self.optional = optional
> +    def check(self, schema, all_members, seen):
> +        assert self.name not in seen
> +        self.type = schema.lookup_type(self.type_name)
> +        assert self.type
> +        all_members.append(self)
> +        seen[self.name] = self
> +
> +class QAPISchemaObjectTypeVariants(object):
> +    def __init__(self, tag_name, tag_enum, variants):
> +        assert tag_name == None or isinstance(tag_name, str)
> +        assert tag_enum == None or isinstance(tag_enum, str)
> +        for v in variants:
> +            assert isinstance(v, QAPISchemaObjectTypeVariant)
> +        self.tag_name = tag_name
> +        if tag_name:
> +            assert not tag_enum
> +            self.tag_member = None
> +        else:
> +            self.tag_member = QAPISchemaObjectTypeMember('kind', tag_enum,
> +                                                         False)
> +        self.variants = variants
> +    def check(self, schema, members, seen):
> +        if self.tag_name:
> +            self.tag_member = seen[self.tag_name]
> +        else:
> +            self.tag_member.check(schema, members, seen)
> +        assert isinstance(self.tag_member.type, QAPISchemaEnumType)
> +        for v in self.variants:
> +            vseen = dict(seen)
> +            v.check(schema, self.tag_member.type, vseen)
> +
> +class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
> +    def __init__(self, name, typ):
> +        QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
> +    def check(self, schema, tag_type, seen):
> +        QAPISchemaObjectTypeMember.check(self, schema, [], seen)
> +        assert self.name in tag_type.values
> +    # TODO try to get rid of .simple_union_type()
> +    def simple_union_type(self):
> +        if isinstance(self.type, QAPISchemaObjectType) and not 
> self.type.info:
> +            assert len(self.type.members) == 1
> +            assert not self.type.variants
> +            return self.type.members[0].type
> +        return None
> +
> +class QAPISchemaAlternateType(QAPISchemaType):
> +    def __init__(self, name, info, variants):
> +        QAPISchemaType.__init__(self, name, info)
> +        assert isinstance(variants, QAPISchemaObjectTypeVariants)
> +        assert not variants.tag_name
> +        self.variants = variants
> +    def check(self, schema):
> +        self.variants.check(schema, [], {})
> +
> +class QAPISchemaCommand(QAPISchemaEntity):
> +    def __init__(self, name, info, arg_type, ret_type, gen, 
> success_response):
> +        QAPISchemaEntity.__init__(self, name, info)
> +        assert not arg_type or isinstance(arg_type, str)
> +        assert not ret_type or isinstance(ret_type, str)
> +        self.arg_type_name = arg_type
> +        self.arg_type = None
> +        self.ret_type_name = ret_type
> +        self.ret_type = None
> +        self.gen = gen
> +        self.success_response = success_response
> +    def check(self, schema):
> +        if self.arg_type_name:
> +            self.arg_type = schema.lookup_type(self.arg_type_name)
> +            assert isinstance(self.arg_type, QAPISchemaObjectType)
> +            assert not self.arg_type.variants # not implemented
> +        if self.ret_type_name:
> +            self.ret_type = schema.lookup_type(self.ret_type_name)
> +            assert isinstance(self.ret_type, QAPISchemaType)
> +
> +class QAPISchemaEvent(QAPISchemaEntity):
> +    def __init__(self, name, info, arg_type):
> +        QAPISchemaEntity.__init__(self, name, info)
> +        assert not arg_type or isinstance(arg_type, str)
> +        self.arg_type_name = arg_type
> +        self.arg_type = None
> +    def check(self, schema):
> +        if self.arg_type_name:
> +            self.arg_type = schema.lookup_type(self.arg_type_name)
> +            assert isinstance(self.arg_type, QAPISchemaObjectType)
> +            assert not self.arg_type.variants # not implemented
> +
> +class QAPISchema(object):
> +    def __init__(self, fname):
> +        try:
> +            self.exprs = check_exprs(QAPISchemaParser(open(fname, 
> "r")).exprs)
> +        except (QAPISchemaError, QAPIExprError), err:

Preferred syntax for exceptions these days is

  except Foo as err

instead of

  except Foo, err

since the former is portable to Python3, should we need to consider
that in future. Again, not a show stopper.

> +            print >>sys.stderr, err
> +            exit(1)

I think it is probably better practice to not have classes print
to stderr / exit.  I'd just let the error propagate to the caller
and have the top level script catch exceptions, print error message
and exit. Again since this is just style issue I won't nack it for
that but something to consider if needing to re-spin this series
again.

> +        self.entity_dict = {}
> +        self._def_predefineds()
> +        self._def_exprs()
> +        self.check()
> +
> +    def get_exprs(self):
> +        return [expr_elem['expr'] for expr_elem in self.exprs]
> +
> +    def _def_entity(self, ent):
> +        assert ent.name not in self.entity_dict
> +        self.entity_dict[ent.name] = ent
> +
> +    def lookup_entity(self, name, typ=None):
> +        ent = self.entity_dict.get(name)
> +        if typ and not isinstance(ent, typ):
> +            return None
> +        return ent
> +
> +    def lookup_type(self, name):
> +        return self.lookup_entity(name, QAPISchemaType)
> +
> +    def _def_builtin_type(self, name):
> +        self._def_entity(QAPISchemaBuiltinType(name))
> +        if name != '**':
> +            self._make_array_type(name) # TODO really needed?
> +
> +    def _def_predefineds(self):
> +        for t in ['str', 'number', 'int', 'int8', 'int16', 'int32', 'int64',
> +                  'uint8', 'uint16', 'uint32', 'uint64', 'size', 'bool', 
> '**']:
> +            self._def_builtin_type(t)
> +
> +    def _make_implicit_enum_type(self, name, values):
> +        name = name + 'Kind'
> +        self._def_entity(QAPISchemaEnumType(name, None, values))
> +        return name
> +
> +    def _make_array_type(self, element_type):
> +        name = element_type + 'List'
> +        if not self.lookup_type(name):
> +            self._def_entity(QAPISchemaArrayType(name, None, element_type))
> +        return name
> +
> +    def _make_implicit_object_type(self, name, role, members):
> +        if not members:
> +            return None
> +        name = ':obj-%s-%s' % (name, role)
> +        if not self.lookup_entity(name, QAPISchemaObjectType):
> +            self._def_entity(QAPISchemaObjectType(name, None, None,
> +                                                  members, None))
> +        return name
> +
> +    def _def_enum_type(self, expr, info):
> +        name = expr['enum']
> +        data = expr['data']
> +        self._def_entity(QAPISchemaEnumType(name, info, data))
> +        self._make_array_type(name) # TODO really needed?
> +
> +    def _make_member(self, name, typ):
> +        optional = False
> +        if name.startswith('*'):
> +            name = name[1:]
> +            optional = True
> +        if isinstance(typ, list):
> +            assert len(typ) == 1
> +            typ = self._make_array_type(typ[0])
> +        return QAPISchemaObjectTypeMember(name, typ, optional)
> +
> +    def _make_members(self, data):
> +        return [self._make_member(key, value) 

Trailing whitespace at end of this line

> +                for (key, value) in data.iteritems()]
> +
> +    def _def_struct_type(self, expr, info):
> +        name = expr['struct']
> +        base = expr.get('base')
> +        data = expr['data']
> +        self._def_entity(QAPISchemaObjectType(name, info, base,
> +                                              self._make_members(data),
> +                                              None))
> +        self._make_array_type(name) # TODO really needed?
> +
> +    def _make_variant(self, case, typ):
> +        return QAPISchemaObjectTypeVariant(case, typ)
> +
> +    def _make_simple_variant(self, case, typ):
> +        if isinstance(typ, list):
> +            assert len(typ) == 1
> +            typ = self._make_array_type(typ[0])
> +        typ = self._make_implicit_object_type(typ, 'wrapper',
> +                                              [self._make_member('data', 
> typ)])
> +        return QAPISchemaObjectTypeVariant(case, typ)
> +
> +    def _make_tag_enum(self, type_name, variants):
> +        return self._make_implicit_enum_type(type_name,
> +                                             [v.name for v in variants])
> +
> +    def _def_union_type(self, expr, info):
> +        name = expr['union']
> +        data = expr['data']
> +        base = expr.get('base')
> +        tag_name = expr.get('discriminator')
> +        tag_enum = None
> +        if tag_name:
> +            variants = [self._make_variant(key, value)
> +                        for (key, value) in data.iteritems()]
> +        else:
> +            variants = [self._make_simple_variant(key, value)
> +                        for (key, value) in data.iteritems()]
> +            tag_enum = self._make_tag_enum(name, variants)
> +        self._def_entity(QAPISchemaObjectType(name, info, base,
> +                                    self._make_members(OrderedDict()),
> +                                    QAPISchemaObjectTypeVariants(tag_name,
> +                                                                 tag_enum,
> +                                                                 variants)))
> +        self._make_array_type(name) # TODO really needed?
> +
> +    def _def_alternate_type(self, expr, info):
> +        name = expr['alternate']
> +        data = expr['data']
> +        variants = [self._make_variant(key, value)
> +                    for (key, value) in data.iteritems()]
> +        tag_enum = self._make_tag_enum(name, variants)
> +        self._def_entity(QAPISchemaAlternateType(name, info,
> +                                    QAPISchemaObjectTypeVariants(None,
> +                                                                 tag_enum,
> +                                                                 variants)))
> +        self._make_array_type(name) # TODO really needed?
> +
> +    def _def_command(self, expr, info):
> +        name = expr['command']
> +        data = expr.get('data')
> +        rets = expr.get('returns')
> +        gen = expr.get('gen', True)
> +        success_response = expr.get('success-response', True)
> +        if isinstance(data, OrderedDict):
> +            data = self._make_implicit_object_type(name, 'arg',
> +                                                   self._make_members(data))
> +        if isinstance(rets, list):
> +            assert len(rets) == 1
> +            rets = self._make_array_type(rets[0])
> +        self._def_entity(QAPISchemaCommand(name, info, data, rets, gen,
> +                                           success_response))
> +
> +    def _def_event(self, expr, info):
> +        name = expr['event']
> +        data = expr.get('data')
> +        if isinstance(data, OrderedDict):
> +            data = self._make_implicit_object_type(name, 'arg',
> +                                                   self._make_members(data))
> +        self._def_entity(QAPISchemaEvent(name, info, data))
> +
> +    def _def_exprs(self):
> +        for expr_elem in self.exprs:
> +            expr = expr_elem['expr']
> +            info = expr_elem['info']
> +            if 'enum' in expr:
> +                self._def_enum_type(expr, info)
> +            elif 'struct' in expr:
> +                self._def_struct_type(expr, info)
> +            elif 'union' in expr:
> +                self._def_union_type(expr, info)
> +            elif 'alternate' in expr:
> +                self._def_alternate_type(expr, info)
> +            elif 'command' in expr:
> +                self._def_command(expr, info)
> +            elif 'event' in expr:
> +                self._def_event(expr, info)
> +            else:
> +                assert False
> +
> +    def check(self):
> +        for ent in self.entity_dict.values():
> +            ent.check(self)
>  
>  #
>  # Code generation helpers
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index 634ef2d..461c713 100644
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -16,7 +16,7 @@ import os
>  import sys
>  
>  try:
> -    exprs = parse_schema(sys.argv[1])
> +    exprs = QAPISchema(sys.argv[1]).get_exprs()
>  except SystemExit:
>      

Following on earlier comment, this could have a block

  except Exception as e:
      print >>sys.stderr, e
      sys.exit(1)

instead of having the object constructor call exit itself.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|