From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60546) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCrCc-0001Y0-Dh for qemu-devel@nongnu.org; Tue, 14 Jun 2016 12:28:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bCrCY-0002Cm-2Z for qemu-devel@nongnu.org; Tue, 14 Jun 2016 12:28:33 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60232) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCrCX-0002Cc-Qf for qemu-devel@nongnu.org; Tue, 14 Jun 2016 12:28:30 -0400 From: Markus Armbruster References: <1463784024-17242-1-git-send-email-eblake@redhat.com> <1463784024-17242-11-git-send-email-eblake@redhat.com> Date: Tue, 14 Jun 2016 18:28:26 +0200 In-Reply-To: <1463784024-17242-11-git-send-email-eblake@redhat.com> (Eric Blake's message of "Fri, 20 May 2016 16:40:19 -0600") Message-ID: <8760tbag5x.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v7 10/15] 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 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, ¶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 > > --- > 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, ¶m, &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(¶m, errp); > +} > +''', > + c_name=c_name(name.lower())) > + > return ret