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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 08/04/2015 09:57 AM, 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.
>> 
>> We generate array types eagerly, even though most of them aren't used.
>> Mark as TODO.
>
> I'm not sure if there are any array types that the rest of the code base
> uses even though it doesn't appear as a directly used type within the
> schemata.  Perhaps some of the builtin types (for example, if qom-get
> needs to return ['uint8']). Besides builtins, maybe we can add some sort
> of 'needs-array':'bool' key to each 'struct'/'union'/'enum'/'alternate',
> which defaults to true only if the schema refers to the array type, but
> which can be explicitly set to true to force the array type generation
> even without a schema reference.  But as it is all properly marked TODO,
> the idle ramblings in this paragraph don't affect review.

Because code for built-ins can be shared among schemata (see -b), we
probably have to keep generating code for array of built-in type
unconditionally.

Re needs-array: stupidest solution that could possibly work is to have
an otherwise useless struct mention all the needed arrays.

>> Nothing uses the new intermediate representation just yet, thus no
>> change to generated files.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  scripts/qapi-commands.py       |   2 +-
>>  scripts/qapi-event.py          |   2 +-
>>  scripts/qapi-types.py          |   2 +-
>>  scripts/qapi-visit.py          |   2 +-
>>  scripts/qapi.py                | 361 
>> ++++++++++++++++++++++++++++++++++++++++-
>>  tests/qapi-schema/test-qapi.py |   2 +-
>>  6 files changed, 357 insertions(+), 14 deletions(-)
>> 
>
>> +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)
>
> Doesn't check whether any of the distinct values map to the same C name.

Pervasive issue.

>  But not a show-stopper to this patch (the earlier semantic checking in
> check_exprs() covers it, and your TODO about moving those checks here at
> a later date is the right time to worry about it here).
>
>> +
>> +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
>
> Is it worth adding:
>
> assert not isinstance(self.element_type, QAPISchemaArrayType)
>
> since we don't allow 2D arrays?

If the generators actually rely on it, yes.

If it's just an arbitrary schema language restriction, probably no.

>> +
>> +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)
>> +        if variants != None:
>> +            assert isinstance(variants, QAPISchemaObjectTypeVariants)
>
> Style nit: the 'base' and 'variants' checks are identical patterns
> (checking for None or specific type), but only one uses an 'if'.
> Possibly because of line-length issues, though, so I can live with it.

I'll clean it up.

>> +
>> +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 # not implemented
>
> and probably never will be (looks like this assert is copy-and-pasted
> from other locations where it makes sense that we might implement
> support for variants, but I don't see it ever happening for the
> generated ':obj-*-wrapper' type for the branch of a simple union)

True.  I'll drop the comment.

> At any rate, I concur that we have a difference in the generated code
> for simple unions compared to flat unions (even if simple unions could
> be rewritten in terms of a flat union from the QMP side, the generated C
> side is not the same), so I agree that you need this function for now,
> as well as the comment for if we later try to clean up the code base to
> drop that difference.

That's the plan.

>> +class QAPISchema(object):
>
>> +    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
>
> Hmm - we probably have collisions if a user tries to explicitly name a
> 'struct' or other type with a 'List' suffix.  Not made worse by this
> patch and not an actual problem with any of our existing .json files, so
> we can save it for another day.

qapi-code-gen.txt reserves the 'Kind' suffix.

We should either adopt a sane, non-colliding scheme for generated names,
or prevent collisions by rejecting reserved names with a sane error
message (documenting them is then optional), or document reserved names.
The latter two require us to figure out what names we reserve.  But as
you say, it's a task for another day.

>> +
>> +    def _make_members(self, data):
>> +        return [self._make_member(key, data[key]) for key in data.keys()]
>
> You could write this as:
>
> return [self._make_member(key, value) for (key, value) in data.iteritems()]
>
> for fewer copies and lookups (dict.keys() and dict.items() creates
> copies, while dict.iteritems() visits key/value pairs in place).
>
> I don't care strongly enough to hold up review, whether or not you
> change it.

I'll check it out.

>> +
>> +    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, data[key])
>> +                        for key in data.keys()]
>> +        else:
>> +            variants = [self._make_simple_variant(key, data[key])
>> +                        for key in data.keys()]
>
> Same comments about the possibility for writing these list
> comprehensions more efficiently.

There are a few more.  I'll either change all or none.

>                                   [and "list comprehensions" is my
> latest bit of cool python language terminology that I learned the name
> of, in order to do this review]

If I remember correctly, I first met them in an obscure hybrid of Prolog
and ML that was at the time a Professor's pet project, which naturally
meant every student must experience it, implemented in Lisp by a gifted
grad student, on an underpowered, memory-starved Mac.  Let's say not
every student was able to see the beauty within the beast.

>> +    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])
>> +        elif isinstance(rets, OrderedDict):
>> +            rets = self._make_implicit_object_type(name, 'ret',
>> +                                                   self._make_members(rets))
>
> Dead 'elif' branch (since you reject dictionaries in "[PATCH 21/26]
> qapi: Command returning anonymous type doesn't work, outlaw")

Yup, will drop it.

> Looks like there will be some minor tweaks when you drop the RFC, but
> it's close enough at this point that I'm okay with:
>
> Reviewed-by: Eric Blake <address@hidden>

Thanks!