On 06/16/2016 06:25 AM, Markus Armbruster wrote: > Markus Armbruster <arm...@redhat.com> writes: > >> Eric Blake <ebl...@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, ¶m, &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
signature.asc
Description: OpenPGP digital signature
