From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33490) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bL7cZ-0000ls-1e for qemu-devel@nongnu.org; Thu, 07 Jul 2016 07:37:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bL7cV-0005G0-OG for qemu-devel@nongnu.org; Thu, 07 Jul 2016 07:37:30 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51709) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bL7cV-0005Fp-G1 for qemu-devel@nongnu.org; Thu, 07 Jul 2016 07:37:27 -0400 From: Markus Armbruster References: <1467514729-29366-1-git-send-email-eblake@redhat.com> <1467514729-29366-12-git-send-email-eblake@redhat.com> Date: Thu, 07 Jul 2016 13:37:24 +0200 In-Reply-To: <1467514729-29366-12-git-send-email-eblake@redhat.com> (Eric Blake's message of "Sat, 2 Jul 2016 20:58:44 -0600") Message-ID: <871t35ptl7.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v8 11/16] qapi-event: Reduce chance of collision with event data List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, Michael Roth Eric Blake 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 > > --- > 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.