Re: [Qemu-devel] [PATCH v8 11/16] qapi-event: Reduce chance of collision with event data

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> When an unboxed event has accompanying data, we are exposing all
> of its members alongside our local variables in the generated
> qapi_event_send_FOO() function.  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 (qapi_event_send_FOO())
> a shell that boxes things up without having to worry about
> collisions, then calls into a new worker function
> (do_qapi_event_send_FOO()) that gets generated to look like what
> we already output for boxed events.
>
> 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.
>
> Adding the new wrapper now means that we need .c_type() for an
> anonymous type given as data to an event, because that type is used
> in the signature of the new helper function, so we have to relax
> an assertion in QAPISchemaObjectType.
>
> The generated file is unchanged for events without data, and for
> boxed events; for unboxed events, the changes are 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 <address@hidden>
>
> ---
> v8: don't open-code for boxed types, improve commit message,
> s/intro/prefix/
> v7: new patch
> ---
>  scripts/qapi.py       |  1 -
>  scripts/qapi-event.py | 45 ++++++++++++++++++++++++++-------------------
>  2 files changed, 26 insertions(+), 20 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 48263c4..e051892 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1019,7 +1019,6 @@ class QAPISchemaObjectType(QAPISchemaType):
       def c_name(self):
>          return QAPISchemaType.c_name(self)

Aside: this method became pointless in commit 7ce106a.

>
>      def c_type(self):
> -        assert not self.is_implicit()
>          return c_name(self.name) + pointer_suffix

Correct if we emit a C type with this name for *any* object type.  I
don't think we do for the empty object type.  Any others?

The assertion should be relaxed to reject exactly the object types for
which we don't emit a C type.

>
>      def c_unboxed_type(self):
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index 2cab588..b2da0a9 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):

The function has changed from one that does just one thing to one that
can do many things depending on arg_type, box and impl.  It's not even
obvious which combinations are valid.  Either split it into simple,
self-documenting functions, or explain this complex one with a function
comment.

> +    prefix='void '
> +    if impl and arg_type and not box:
> +        box = True
> +        prefix='static void do_'
> +    return '%(prefix)sqapi_event_send_%(c_name)s(%(param)s)' % {
> +        'prefix': prefix,
>          'c_name': c_name(name.lower()),
>          'param': gen_params(arg_type, box, 'Error **errp')}
>
> @@ -49,21 +54,12 @@ def gen_param_var(typ):
>
>      };
>  ''')
> -    if not typ.is_implicit():
> -        ret += mcgen('''
> -    %(c_name)s *arg = &param;
> -''',
> -                c_name=typ.c_name())
>      return ret
>
>
>  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():
> +        assert not box
>      ret = mcgen('''
>
>  %(proto)s
> @@ -72,17 +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)
> -    else:
> -        assert not box
>
>      ret += mcgen('''
>
> @@ -112,7 +104,7 @@ def gen_event_send(name, arg_type, box):
>      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);
>      }
> @@ -144,6 +136,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

This function spans 95 lines, and you have to read to the end to see
that it first emits a function that can either be the
qapi_event_send_FOO() function or a helper, and if it's the latter, then
emits the former.  Either split it up into parts that can be more easily
understood, or add suitable comments to the complex function.