Re: [Qemu-devel] [PATCH v7 10/15] qapi-event: Reduce chance of collision with event data

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4273 bytes --]

On 06/16/2016 06:25 AM, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> When an event has data that is not boxed, we are exposing all of
>>> its members alongside our local variables.  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 a shell that creates a
>>> simple wrapper, then calls a worker that operates on only the
>>> boxed version and thus has no user-supplied names to worry about
>>> in naming its local variables.  For boxed events, we don't need
>>> the wrapper.
>>>
>>> There is still a chance for collision with 'errp' (if that happens,
>>> tweak c_name() to rename a QMP member 'errp' into the C member
>>> 'q_errp'), and with 'param' (if that happens, tweak gen_event_send()
>>> and gen_param_var() to name the temporary variable 'q_param').  But
>>> with the division done here, the real worker function no longer has
>>> to worry about collisions.
>>>

>>> +++ b/scripts/qapi.py
>>> @@ -1016,7 +1016,6 @@ class QAPISchemaObjectType(QAPISchemaType):
>>>          return QAPISchemaType.c_name(self)
>>>
>>>      def c_type(self):
>>> -        assert not self.is_implicit()
>>
>> Huh?

Required, because we now pass a pointer to an implicit type from
qapi_event_send_FOO() to do_qapi_event_send_FOO(), so the c_type() of
that implicit type is required for generating the C type for that
parameter.  Will document it better in the commit message.


>>> @@ -93,20 +92,11 @@ def gen_event_send(name, arg_type, box):
>>>          ret += mcgen('''
>>>      v = qmp_output_visitor_new(&obj);
>>>
>>> -''')
>>> -
>>> -        if box:
>>> -            ret += mcgen('''
>>> -    visit_type_%(c_name)s(v, NULL, &arg, &err);
>>> -''',
>>> -                         c_name=arg_type.c_name(), name=arg_type.name)
>>> -        else:
>>> -            ret += mcgen('''
>>>      visit_start_struct(v, "%(name)s", NULL, 0, &err);
>>>      if (err) {
>>>          goto out;
>>>      }
>>> -    visit_type_%(c_name)s_members(v, &param, &err);
>>> +    visit_type_%(c_name)s_members(v, arg, &err);
>>>      if (!err) {
>>>          visit_check_struct(v, &err);
>>>      }
>>
>> Getting confused...  why are we getting rid of the box case here?

No good reason. The visit_type_FOO() is more compact than
visit_type_FOO_members(), but only when we have a non-implicit type.  So
for v8, I'm switching the conditional from 'if box:' to 'if
arg_type.is_implicit():', more or less.

>>
>> Too many conditionals...  gen_event_send() has three cases: empty
>> arg_type, non-empty arg_type and box, non-empty arg_type and not box.
>> The commit message shows the change to generated code for the second
>> case.  It doesn't show visit_type_%(c_name)s(v, NULL, &arg, &err) going
>> away.
> 
> Case empty arg_type: no change
> Example: POWERDOWN

Good.

> 
> Case non-empty arg_type and box: visit gets open-coded
> Example: EVENT_E

Fixed in v8 so that it no longer changes.

> The open-coded visit drops the !*obj check (okay, @arg isn't going
> anywhere), skips the visit_check_struct() differently, and drops the
> qapi_free_FOO() (okay, condition is always false here).
> 
> So this isn't wrong.  But why open-code?

No need to add new open-coding, but we already had existing open-coding
for anonymous non-boxed 'data' (in part because commit 7ce106a9
intentionally chose not to create visit_type_FOO() for implicit types).

> 
> Case non-empty arg_type and not box:
> Example: ACPI_DEVICE_OST
> 

> 
> This is the case the commit message advertises.
> 
> There is no visit_type_FOO() we could compare too, since FOO is an
> implicit type

And in reviewing your message, I realize we have NO testsuite coverage of:

{ 'event': 'EVENT', 'data': 'NamedStruct' }

Guess I get to add that first.  Such a usage will then be improved by
using visit_type_NamedStruct() rather than open-coding around
visit_type_NamedStruct_members().

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]