[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 11/16] qapi-event: Reduce chance of collision
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v8 11/16] qapi-event: Reduce chance of collision with event data |
Date: |
Thu, 07 Jul 2016 13:37:24 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
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, ¶m, &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(¶m, 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 = ¶m;
> -''',
> - 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, ¶m, &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(¶m, 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.
[Qemu-devel] [PATCH v8 07/16] qapi: Plumb in 'box' to qapi generator lower levels, Eric Blake, 2016/07/02
[Qemu-devel] [PATCH v8 11/16] qapi-event: Reduce chance of collision with event data, Eric Blake, 2016/07/02
- Re: [Qemu-devel] [PATCH v8 11/16] qapi-event: Reduce chance of collision with event data,
Markus Armbruster <=
[Qemu-devel] [PATCH v8 09/16] block: Simplify block_set_io_throttle, Eric Blake, 2016/07/02
[Qemu-devel] [PATCH v8 16/16] schema: Drop pointless empty type CpuInfoOther, Eric Blake, 2016/07/02
[Qemu-devel] [PATCH v8 12/16] qapi: Change Netdev into a flat union, Eric Blake, 2016/07/02
Re: [Qemu-devel] [PATCH for-2.7 v8 00/16] qapi netdev_add introspection (post-introspection cleanups subset F), Markus Armbruster, 2016/07/07