"Daniel P. Berrange" <berra...@redhat.com> 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 <arm...@redhat.com>
>> Reviewed-by: Eric Blake <ebl...@redhat.com>
>
> Reviewed-by: Daniel P. Berrange <berra...@redhat.com>
>
> 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!
