Eric Blake <ebl...@redhat.com> writes:
> 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.
Hmm. The real assertion here is "we generate a C type for this QAPI
object type." Can we express that in code?
>>>> @@ -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().
Makes sense.
