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

[Markus Armbruster]

"Daniel P. Berrange" <address@hidden> writes:

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

I have such blank lines in the more complex stuff.  Didn't feel like it
in trivial classes like this, but if others want the blank lines, I'm
happy to add them.

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

I've been using pylint (and dear-oh-dear how much pre-existing lint it
finds!), wasn't aware of pep8, can throw it in.

Automatic checking would be nice, but our current code is probably too
untidy for that.  Besides, I don't have the time and energy to set it
up now.

pylint is reasonably happy with the qapi-*.py now.  I hope the follow-up
work on qapi.py will get it into the same state.

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

Does it work with Python 2.4?  That's where we're currently stuck.

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

Quadruplicates the exception handling...

  I think I'll leave this question open for now.

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

Will fix, and search for more.

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

Thanks!