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

[Markus Armbruster <armbru@redhat.com>]

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.
>>
>> The generated file changes as follows:
>>
>> |-void qapi_event_send_migration(MigrationStatus status, Error **errp)
>> |+static void do_qapi_event_send_migration(q_obj_MIGRATION_arg *arg, Error **errp)
>> | {
>> |     QDict *qmp;
>> |     Error *err = NULL;
>> |     QMPEventFuncEmit emit;
>> |     QObject *obj;
>> |     Visitor *v;
>> |-    q_obj_MIGRATION_arg param = {
>> |-        status
>> |-    };
>> |
>> |     emit = qmp_event_get_func_emit();
>> |     if (!emit) {
>> ...
>> |     if (err) {
>> |         goto out;
>> |     }
>> |-    visit_type_q_obj_MIGRATION_arg_members(v, &param, &err);
>> |+    visit_type_q_obj_MIGRATION_arg_members(v, arg, &err);
>> |     if (!err) {
>> |         visit_check_struct(v, &err);
>> ...
>> |+void qapi_event_send_migration(MigrationStatus status, Error **errp)
>> |+{
>> |+    q_obj_MIGRATION_arg param = {
>> |+        status
>> |+    };
>> |+    do_qapi_event_send_migration(&param, errp);
>> |+}
>> |+
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>> v7: new patch
>> ---
>>  scripts/qapi.py       |  1 -
>>  scripts/qapi-event.py | 47 ++++++++++++++++++++++++++---------------------
>>  2 files changed, 26 insertions(+), 22 deletions(-)
>>
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index 6742e7a..7d568d9 100644
>> --- a/scripts/qapi.py
>> +++ 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?
>
>>          return c_name(self.name) + pointer_suffix
>>
>>      def c_unboxed_type(self):
>> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
>> index b8ca8c8..fe4e50d 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):
>> +    intro='void '
>> +    if impl and arg_type and not box:
>> +        box = True
>> +        intro='static void do_'
>> +    return '%(intro)sqapi_event_send_%(c_name)s(%(param)s)' % {
>> +        'intro': intro,
>>          'c_name': c_name(name.lower()),
>>          'param': gen_params(arg_type, box, 'Error **errp')}
>>
>
> I'd call it prefix rather than intro.
>
>> @@ -53,12 +58,8 @@ def gen_param_var(typ):
>>
>>
>>  def gen_event_send(name, arg_type, box):
>> -    # FIXME: Our declaration of local variables (and of 'errp' in the
>> -    # parameter list) can collide with exploded members of the event's
>> -    # data type passed in as parameters.  If this collision ever hits in
>> -    # practice, we can rename our local variables with a leading _ prefix,
>> -    # or split the code into a wrapper function that creates a boxed
>> -    # 'param' object then calls another to do the real work.
>> +    if not arg_type or arg_type.is_empty():
>> +        box = False
>>      ret = mcgen('''
>>
>>  %(proto)s
>> @@ -67,15 +68,13 @@ def gen_event_send(name, arg_type, box):
>>      Error *err = NULL;
>>      QMPEventFuncEmit emit;
>>  ''',
>> -                proto=gen_event_send_proto(name, arg_type, box))
>> +                proto=gen_event_send_proto(name, arg_type, box, True))
>>
>>      if arg_type and not arg_type.is_empty():
>>          ret += mcgen('''
>>      QObject *obj;
>>      Visitor *v;
>>  ''')
>> -        if not box:
>> -            ret += gen_param_var(arg_type)
>
> Because the not box case moves to the wrapper function (last hunk).
>
>>
>>      ret += mcgen('''
>>
>> @@ -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?
>
> 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

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

     void qapi_event_send_event_e(UserDefZero *arg, Error **errp)
     {
         QDict *qmp;
         Error *err = NULL;
         QMPEventFuncEmit emit;
         QObject *obj;
         Visitor *v;

         emit = qmp_event_get_func_emit();
         if (!emit) {
             return;
         }

         qmp = qmp_event_build_dict("EVENT_E");

         v = qmp_output_visitor_new(&obj);

    -    visit_type_UserDefZero(v, NULL, &arg, &err);
    +    visit_start_struct(v, "EVENT_E", NULL, 0, &err);
    +    if (err) {
    +        goto out;
    +    }
    +    visit_type_UserDefZero_members(v, arg, &err);
    +    if (!err) {
    +        visit_check_struct(v, &err);
    +    }
    +    visit_end_struct(v, NULL);
         if (err) {
             goto out;
         }

         visit_complete(v, &obj);
         qdict_put_obj(qmp, "data", obj);
         emit(TEST_QAPI_EVENT_EVENT_E, qmp, &err);

     out:
         visit_free(v);
         error_propagate(errp, err);
         QDECREF(qmp);
     }

Compare:

     void visit_type_UserDefZero(Visitor *v, const char *name, UserDefZero **obj, Error **errp)
     {
         Error *err = NULL;

         visit_start_struct(v, name, (void **)obj, sizeof(UserDefZero), &err);
         if (err) {
             goto out;
         }
--->     if (!*obj) {
--->         goto out_obj;
--->     }
         visit_type_UserDefZero_members(v, *obj, &err);
--->     if (err) {
--->         goto out_obj;
--->     }
--->     visit_check_struct(v, &err);
     out_obj:
         visit_end_struct(v, (void **)obj);
--->     if (err && visit_is_input(v)) {
--->         qapi_free_UserDefZero(*obj);
--->         *obj = NULL;
--->     }
     out:
         error_propagate(errp, err);
     }

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?

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

    -void qapi_event_send_acpi_device_ost(ACPIOSTInfo *info, Error **errp)
    +static void do_qapi_event_send_acpi_device_ost(q_obj_ACPI_DEVICE_OST_arg *arg, Error **errp)
     {
         QDict *qmp;
         Error *err = NULL;
         QMPEventFuncEmit emit;
         QObject *obj;
         Visitor *v;
    -    q_obj_ACPI_DEVICE_OST_arg param = {
    -        info
    -    };

         emit = qmp_event_get_func_emit();
         if (!emit) {
             return;
         }

         qmp = qmp_event_build_dict("ACPI_DEVICE_OST");

         v = qmp_output_visitor_new(&obj);

         visit_start_struct(v, "ACPI_DEVICE_OST", NULL, 0, &err);
         if (err) {
             goto out;
         }
    -    visit_type_q_obj_ACPI_DEVICE_OST_arg_members(v, &param, &err);
    +    visit_type_q_obj_ACPI_DEVICE_OST_arg_members(v, arg, &err);
         if (!err) {
             visit_check_struct(v, &err);
         }
         visit_end_struct(v, NULL);
         if (err) {
             goto out;
         }

         visit_complete(v, &obj);
         qdict_put_obj(qmp, "data", obj);
         emit(QAPI_EVENT_ACPI_DEVICE_OST, qmp, &err);

     out:
         visit_free(v);
         error_propagate(errp, err);
         QDECREF(qmp);
     }

    -void qapi_event_send_balloon_change(int64_t actual, Error **errp)
    +void qapi_event_send_acpi_device_ost(ACPIOSTInfo *info, Error **errp)
    +{
    +    q_obj_ACPI_DEVICE_OST_arg param = {
    +        info
    +    };
    +    do_qapi_event_send_acpi_device_ost(&param, errp);
    +}
    +

This is the case the commit message advertises.

There is no visit_type_FOO() we could compare too, since FOO is an
implicit type

>> @@ -136,6 +126,21 @@ out:
>>      QDECREF(qmp);
>>  }
>>  ''')
>> +
>> +    if arg_type and not box:
>> +        ret += mcgen('''
>> +
>> +%(proto)s
>> +{
>> +''',
>> +                     proto=gen_event_send_proto(name, arg_type, box))
>> +        ret += gen_param_var(arg_type)
>> +        ret += mcgen('''
>> +    do_qapi_event_send_%(c_name)s(&param, errp);
>> +}
>> +''',
>> +                     c_name=c_name(name.lower()))
>> +
>>      return ret