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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 06/16/2016 06:25 AM, Markus Armbruster wrote:
>> Markus Armbruster <address@hidden> writes:
>> 
>>> Eric Blake <address@hidden> 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, &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().

Makes sense.