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

[Markus Armbruster]

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.
>
> 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 <address@hidden>
>
> ---
> 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.

> @@ -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