From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48846) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bHo2u-0004Td-Qw for qemu-devel@nongnu.org; Tue, 28 Jun 2016 04:07:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bHo2q-0006qk-ID for qemu-devel@nongnu.org; Tue, 28 Jun 2016 04:06:59 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46333) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bHo2q-0006qf-Aj for qemu-devel@nongnu.org; Tue, 28 Jun 2016 04:06:56 -0400 From: Markus Armbruster References: <1463784024-17242-1-git-send-email-eblake@redhat.com> <1463784024-17242-11-git-send-email-eblake@redhat.com> <8760tbag5x.fsf@dusky.pond.sub.org> <871t3x723b.fsf@dusky.pond.sub.org> <5771ED16.9070100@redhat.com> Date: Tue, 28 Jun 2016 10:06:53 +0200 In-Reply-To: <5771ED16.9070100@redhat.com> (Eric Blake's message of "Mon, 27 Jun 2016 21:20:54 -0600") Message-ID: <87shvxenz6.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: > On 06/16/2016 06:25 AM, Markus Armbruster wrote: >> Markus Armbruster writes: >> >>> 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. >>>> > >>>> +++ 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? > > Required, because we now pass a pointer to an implicit type from > qapi_event_send_FOO() to do_qapi_event_send_FOO(), so the c_type() of > that implicit type is required for generating the C type for that > parameter. Will document it better in the commit message. Hmm. The real assertion here is "we generate a C type for this QAPI object type." Can we express that in code? >>>> @@ -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? > > No good reason. The visit_type_FOO() is more compact than > visit_type_FOO_members(), but only when we have a non-implicit type. So > for v8, I'm switching the conditional from 'if box:' to 'if > arg_type.is_implicit():', more or less. > >>> >>> 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. >> >> Case empty arg_type: no change >> Example: POWERDOWN > > Good. > >> >> Case non-empty arg_type and box: visit gets open-coded >> Example: EVENT_E > > Fixed in v8 so that it no longer changes. > >> The open-coded visit drops the !*obj check (okay, @arg isn't going >> anywhere), skips the visit_check_struct() differently, and drops the >> qapi_free_FOO() (okay, condition is always false here). >> >> So this isn't wrong. But why open-code? > > No need to add new open-coding, but we already had existing open-coding > for anonymous non-boxed 'data' (in part because commit 7ce106a9 > intentionally chose not to create visit_type_FOO() for implicit types). > >> >> Case non-empty arg_type and not box: >> Example: ACPI_DEVICE_OST >> > >> >> This is the case the commit message advertises. >> >> There is no visit_type_FOO() we could compare too, since FOO is an >> implicit type > > And in reviewing your message, I realize we have NO testsuite coverage of: > > { 'event': 'EVENT', 'data': 'NamedStruct' } > > Guess I get to add that first. Such a usage will then be improved by > using visit_type_NamedStruct() rather than open-coding around > visit_type_NamedStruct_members(). Makes sense.