Re: [Qemu-devel] [PATCH v8 11/16] qapi-event: Reduce chance of collision with event data

[Eric Blake]

On 07/07/2016 05:37 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> When an unboxed event has accompanying data, we are exposing all
>> of its members alongside our local variables in the generated
>> qapi_event_send_FOO() function.  So far, we haven't hit a
>> collision, but it may be a matter of time before someone wants
>> to name a QMP data element 'err' or similar.  We can separate
>> the names by making the public function (qapi_event_send_FOO())
>> a shell that boxes things up without having to worry about
>> collisions, then calls into a new worker function
>> (do_qapi_event_send_FOO()) that gets generated to look like what
>> we already output for boxed events.
>>

>> +++ b/scripts/qapi.py
>> @@ -1019,7 +1019,6 @@ class QAPISchemaObjectType(QAPISchemaType):
>        def c_name(self):
>>          return QAPISchemaType.c_name(self)
> 
> Aside: this method became pointless in commit 7ce106a.
> 

Sounds like an easy separate cleanup.

>>
>>      def c_type(self):
>> -        assert not self.is_implicit()
>>          return c_name(self.name) + pointer_suffix
> 
> Correct if we emit a C type with this name for *any* object type.  I
> don't think we do for the empty object type.  Any others?
> 
> The assertion should be relaxed to reject exactly the object types for
> which we don't emit a C type.

Will fix.

> 
>>
>>      def c_unboxed_type(self):
>> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
>> index 2cab588..b2da0a9 100644
>> --- a/scripts/qapi-event.py
>> +++ b/scripts/qapi-event.py
>> @@ -14,8 +14,13 @@
>>  from qapi import *
>>
>>
>> -def gen_event_send_proto(name, arg_type, box):
>> -    return 'void qapi_event_send_%(c_name)s(%(param)s)' % {
>> +def gen_event_send_proto(name, arg_type, box, impl=False):
> 
> The function has changed from one that does just one thing to one that
> can do many things depending on arg_type, box and impl.  It's not even
> obvious which combinations are valid.  Either split it into simple,
> self-documenting functions, or explain this complex one with a function
> comment.

At this point, this cleanup is separate from the main work to get
'boxed' commands working, so I'll focus on getting the important first
half of the series posted today, and leave this particular patch for
when I have more time to get the refactoring done right.


> 
> This function spans 95 lines, and you have to read to the end to see
> that it first emits a function that can either be the
> qapi_event_send_FOO() function or a helper, and if it's the latter, then
> emits the former.  Either split it up into parts that can be more easily
> understood, or add suitable comments to the complex function.

Indeed.  The changes were all made piecemeal with minimal churn, but the
end result is messier than if we step back and do it right.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature