From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33421) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aW8nc-0004bK-1T for qemu-devel@nongnu.org; Wed, 17 Feb 2016 15:34:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aW8nX-0007NF-1l for qemu-devel@nongnu.org; Wed, 17 Feb 2016 15:34:11 -0500 Received: from mx1.redhat.com ([209.132.183.28]:48503) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aW8nW-0007N4-Pv for qemu-devel@nongnu.org; Wed, 17 Feb 2016 15:34:06 -0500 References: <1455582057-27565-1-git-send-email-eblake@redhat.com> <1455582057-27565-11-git-send-email-eblake@redhat.com> <87io1oo4o5.fsf@blackfin.pond.sub.org> <56C37E29.6040500@redhat.com> <877fi3v1ra.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <56C4D93C.8090805@redhat.com> Date: Wed, 17 Feb 2016 13:34:04 -0700 MIME-Version: 1.0 In-Reply-To: <877fi3v1ra.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="8GbrqB7QJuTPhdcPC2vmCiKdS45lnW732" Subject: Re: [Qemu-devel] [PATCH v10 10/13] qapi: Don't box struct branch of alternate List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --8GbrqB7QJuTPhdcPC2vmCiKdS45lnW732 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 02/17/2016 07:40 AM, Markus Armbruster wrote: >>>> There's no reason to do two malloc's for an alternate type visiting >>>> a QAPI struct; let's just inline the struct directly as the C union >>>> branch of the struct. >>>> >> Also, here we pass 'obj'; visit_type_FOO() had to pass '*obj' (again, >> because we have one less level of indirection, and 7/13 reduced the >> indirection required in visit_type_FOO_fields()). >> >>> // visit_start_union() + switch dropped >>> error_propagate(errp, err); >>> err =3D NULL; >>> visit_end_struct(v, &err); >>> out: >>> error_propagate(errp, err); >>> } >>> >>> Why can we drop visit_start_union() + switch? >> >> visit_start_union() is dropped because its only purpose was to determi= ne >> if the dealloc visitor needs to visit the default branch. When we had = a >> separate allocation, we did not want to visit the branch if the >> discriminator was not successfully parsed, because otherwise we would >> dereference NULL. But now that we don't have a second pointer >> allocation, we no longer have anything to dealloc, and we can no longe= r >> dereference NULL. Explained better in 12/13, where I delete >> visit_start_union() altogether. But maybe I could keep it in this pat= ch >> in the meantime, to minimize confusion. >=20 > Perhaps squashing the two patches together could be less confusing. Yes, I'm closer to posting v11, and in that version, visit_start_union is only dropped in a single patch; and this patch just inlines the visit_start_struct() allocation directly into the visit_type_ALT() rather than creating a new visit_type_alternate_ALT(). >=20 >> Dropped switch, on the other hand, looks to be a genuine bug. Eeek. >> That should absolutely be present, and it proves that our testsuite is= >> not strong enough for not catching me on it. >> >> And now that you've made me think about it, maybe I have yet another >> idea. Right now, we've split the visit of local members into >> visit_type_FOO_fields(), while leaving the variant members to be visit= ed >> in visit_type_FOO() >=20 > Yes. I guess that's to support visiting "implicit" structs. Up to now, we've forbidden the use of one union as a branch of another (but allowed a union as a branch of an alternate), so the types passed to visit_struct_FOO_fields() never had variants. As part of our generic "object" work, I _want_ to support one union as a branch of another (as long as we can prove there will be no QMP name collisions); and that's another reason why visit_struct_FOO_fields() would need to always visit variants if present. > Let me get to this result from a different angle. A "boxed" visit is >=20 > allocate hook > visit the members (common and variant) > cleanup hook Correct, and we have two choices of allocate hook: visit_start_struct() [allocate and consume {}, for visit_type_FOO() in general], and visit_start_implicit_struct() [allocate, but don't consume {}, for flat union branches prior to this series]. >=20 > An "unboxed" visit is basically the same without the two hooks. Done anywhere we don't have a separate C struct [base classes prior to this series; and then this series is adding unboxed variant visits within flat unions and alternates]. >=20 > "Implicit" is like unboxed, except the members are inlined rather than > wrapped in a JSON object. >=20 > So the common code to factor out is "visit the members". Yep, we're on the same wavelength, and it is looking fairly nice for what I'm about to post as v11. And I like 'unboxed' better than 'is_member': >>>> - c_type=3Dtyp.c_type(), >>>> + c_type=3Dtyp.c_type(is_member=3Dinline), >=20 > I don't like the name is_member. The thing we're dealing with here is = a > member, regardless of the value of inline. I think it's boxed > vs. unboxed. Hmm. I have later patches that add a 'box':true QAPI designation to commands and events, to let us do qmp_command(Foo *arg, Error **errp) instead of qmp_command(type Foo_member1, type Foo_member2 ..., Error **errp) (that is, instead of breaking out each parameter, we pass them all boxed behind a single pointer). What we are doing here is in the opposite direction - we are taking code that has boxed all the sub-type fields behind a single pointer, and unboxing it so that they occur inline in the parent type's storage. Works for me; I'm switching to 'is_unboxed' as the case for when we want to omit the pointer designation during the member declaration. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --8GbrqB7QJuTPhdcPC2vmCiKdS45lnW732 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWxNk8AAoJEKeha0olJ0NqvyMH/35RgW2CwkPCJNsaDmnvInf5 exD9OunSwzpbhI3aoH9siBTCn/ZZsCacHUI/sYU0S49BM3wi8eqh09lQ5U8RVpyd 7w5TeWNhr/hnsizwecH9F19EzuTOtKrhLilrF9QQjdUDE+ymUg0QCpdCPUyWhK9T avGKYRmQico7FhOaFpluADxZvJoU4D50JhDOdKoFOROIcSKY9GpG0wGznP1UEn3N 0db5p1ePmFYplsMMBaAVFTtJpi1XOfjSnWqRIq87mBo+44dz6Y9pqQk0OGsNGoeI brQbCh7Xs3wYPp5mmht397CbOxX1FsKkI/SSThbEhfy9/W8qqDATnmQooqPP/cg= =yVEy -----END PGP SIGNATURE----- --8GbrqB7QJuTPhdcPC2vmCiKdS45lnW732--