Re: [Qemu-devel] [PATCH 18/19] qapi: Fix excessive QAPISchemaEntity.check() recursion

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 9/14/19 10:35 AM, Markus Armbruster wrote:
>> Entity checking goes back to commit ac88219a6c "qapi: New QAPISchema
>> intermediate representation", v2.5.0.  It's designed to work as
>> follows: QAPISchema.check() calls .check() for all the schema's
>> entities.  An entity's .check() recurses into another entity's
>> .check() only if the C struct generated for the former contains the C
>> struct generated for the latter (pointers don't count).  This is used
>> to detect "object contains itself".
>> 
>> There are two instances of this:
>> 
>> * An object's C struct contains its base's C struct
>> 
>>   QAPISchemaObjectType.check() calls self.base.check()
>> 
>> * An object's C struct contains its variants' C structs
>> 
>>   QAPISchemaObjectTypeVariants().check calls v.type.check().  Since
>>   commit b807a1e1e3 "qapi: Check for QAPI collisions involving variant
>>   members", v2.6.0.
>> 
>
>> Sadly, this design has since eroded:
>> 
>
>> * A QAPISchemaEntity's .module becomes valid at .check().  This is
>>   because computing it needs info and schema.fname, and because array
>>   types get it from their element type instead.
>> 
>>   Make it a property  just like .ifcond.
>
> Extra space.

Will fix.

>> Additionally, have QAPISchemaEntity.check() assert it gets called at
>> most once, so the design invariant will stick this time.  Neglecting
>> that was entirely my fault.
>
> Lengthy explanation, but it makes sense.

Writing the commit message was more work than writing the patch...

>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  scripts/qapi/common.py | 74 +++++++++++++++++++++++++++++-------------
>>  1 file changed, 52 insertions(+), 22 deletions(-)
>> 
>
> Took me a bit longer to read this (and refresh myself on what
> '@property' actually does in Python, but you're not the first user of it
> in qemu), but it matches the commit message.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!