From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46205) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bArNe-0005w0-7U for qemu-devel@nongnu.org; Thu, 09 Jun 2016 00:15:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bArNZ-0001LF-TR for qemu-devel@nongnu.org; Thu, 09 Jun 2016 00:15:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58283) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bArNZ-0001L9-FS for qemu-devel@nongnu.org; Thu, 09 Jun 2016 00:15:37 -0400 References: <1463632874-28559-1-git-send-email-eblake@redhat.com> <1463632874-28559-14-git-send-email-eblake@redhat.com> <8737oviu3s.fsf@dusky.pond.sub.org> From: Eric Blake Message-ID: <5758ED67.1050304@redhat.com> Date: Wed, 8 Jun 2016 22:15:35 -0600 MIME-Version: 1.0 In-Reply-To: <8737oviu3s.fsf@dusky.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="HgjBdBlp304xMSiUat6X0Gkw0DdobE93T" Subject: Re: [Qemu-devel] [PATCH v4 13/28] qapi: Add new clone visitor 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) --HgjBdBlp304xMSiUat6X0Gkw0DdobE93T Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 06/02/2016 07:43 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> We have a couple places in the code base that want to deep-clone >> one QAPI object into another, and they were resorting to serializing >> the struct out to QObject then reparsing it. A much more efficient >> version can be done by adding a new clone visitor. >> > [...] > * If an error is detected during visit_type_FOO() with an input > * visitor, then *@obj will be NULL for pointer types, and left > * unchanged for scalar types. Using an output visitor with an > * incomplete object has undefined behavior (other than a special ca= se > * for visit_type_str() treating NULL like ""), while the dealloc > * visitor safely handles incomplete objects. Since input visitors > * never produce an incomplete object, such an object is possible on= ly > * by manual construction. >=20 > What about the clone visitor? Probably safest to document it as undefined on incomplete objects. > /* > * Start visiting an object @obj (struct or union). > * > * @name expresses the relationship of this object to its parent > * container; see the general description of @name above. > * > * @obj must be non-NULL for a real walk, in which case @size > * determines how much memory an input visitor will allocate into > * *@obj. @obj may also be NULL for a virtual walk, in which case > * @size is ignored. >=20 > What about the clone visitor? Yes, clone visitors also use size. >=20 > * > * @errp obeys typical error usage, and reports failures such as a > * member @name is not present, or present but not an object. On > * error, input visitors set *@obj to NULL. >=20 > What about the clone visitor? Never sets an error (ie. it can't fail on a complete source object, if you don't include abort-due-to-OOM scenarios), so I'm not sure I need to word anything differently here. > * Start visiting a list. > * > * @name expresses the relationship of this list to its parent > * container; see the general description of @name above. > * > * @list must be non-NULL for a real walk, in which case @size > * determines how much memory an input visitor will allocate into > * *@list (at least sizeof(GenericList)). Some visitors also allow > * @list to be NULL for a virtual walk, in which case @size is > * ignored. >=20 > What about the clone visitor? >=20 > * > * @errp obeys typical error usage, and reports failures such as a > * member @name is not present, or present but not a list. On error= , > * input visitors set *@list to NULL. >=20 > What about the clone visitor? Same as for start_struct. > /* > * Does optional struct member @name need visiting? > * > * @name must not be NULL. This function is only useful between > * visit_start_struct() and visit_end_struct(), since only objects > * have optional keys. > * > * @present points to the address of the optional member's has_ flag= =2E > * > * Input visitors set *@present according to input; other visitors > * leave it unchanged. In either case, return *@present for > * convenience. >=20 > I guess this is correct for the clone visitor. Clone visitor leaves it alone (it is reading *@present on the dest, which was already set earlier during the g_memdup() of visit_start_*). > /* > * Visit an enum value. > * > * @name expresses the relationship of this enum to its parent > * container; see the general description of @name above. > * > * @obj must be non-NULL. Input visitors parse input and set *@obj = to > * the enumeration value, leaving @obj unchanged on error; other > * visitors use *@obj but leave it unchanged. >=20 > I guess this is correct for the clone visitor. It's a bit of a stretch, but "use *@obj" can certainly mean "do nothing with it, because it is a scalar that was already set earlier during the g_memdup() of visit_start_*". So yes, the clone visitor wants visit_type_enum() to be a no-op. >=20 > /* > * Check if visitor is an input visitor. >=20 > Does the clone visitor count as input visitor here? Should it? No, and probably no. A clone visitor never sets errp, and therefore there is no reason to clean up after a failed clone; and our current use of visit_is_input() is only for cleaning up after failures in an input visitor. >=20 > */ > bool visit_is_input(Visitor *v); >=20 > /*** Visiting built-in types ***/ >=20 > /* > * Visit an integer value. > * > * @name expresses the relationship of this integer to its parent > * container; see the general description of @name above. > * > * @obj must be non-NULL. Input visitors set *@obj to the value; > * other visitors will leave *@obj unchanged. >=20 > I guess this is correct for the clone visitor. Again correct - the clone visitor doesn't set anything at this point, because the integer was already copied earlier during the g_memdup() of visit_start_*(). > /* > * Visit a string value. > * > * @name expresses the relationship of this string to its parent > * container; see the general description of @name above. > * > * @obj must be non-NULL. Input visitors set *@obj to the value > * (never NULL). Other visitors leave *@obj unchanged, and commonly= > * treat NULL like "". >=20 > I guess this is correct for the clone visitor. The clone visitor could morph NULL into "" (I didn't code it that way, though). Here, the clone visitor DOES set *@obj, in order to dedupe the pointer from the source object, so maybe a third sentence is needed? > /* > * Visit an arbitrary value. > * > * @name expresses the relationship of this value to its parent > * container; see the general description of @name above. > * > * @obj must be non-NULL. Input visitors set *@obj to the value; > * other visitors will leave *@obj unchanged. *@obj must be non-NUL= L > * for output visitors. >=20 > Fine, as the clone visitor doesn't support any. It could, if we use the JSON output visitor code later in the series to create a QObject deep-cloner, but I'd rather not do it unless we find an actual need (keeping 'any' out of clone does simplify the number of corner cases to think about). >> +++ b/qapi/qapi-visit-core.c >=20 > As we'll see further down, @obj points into the clone, except at the > root, where it points to qapi_clone()'s local variable @dst. A > pointer-valued *@obj still points into the source. >=20 > Now let's go through the v->type checks real slow. >=20 >> @@ -44,10 +44,10 @@ void visit_start_struct(Visitor *v, const char *na= me, void **obj, >> >> if (obj) { >> assert(size); >> - assert(v->type !=3D VISITOR_OUTPUT || *obj); >> + assert(!(v->type & VISITOR_OUTPUT) || *obj); >> } >=20 > For real walks (obj !=3D NULL): >=20 > * Input visitors write *obj, and don't care for the old value. >=20 > * Output visitors read *obj, and a struct can't be null. >=20 > * The dealloc visitor reads *obj, but null is fine (partially > constructed object). >=20 > * The clone visitor reads like an output visitor (except at the root) > and writes like an input visitor. >=20 > Before the patch, we assert "if output visitor, then *obj isn't null". >=20 > After the patch, we do the same for the clone visitor. Correct, except= > at the root. There, @obj points to qapi_clone()'s @dst, which is > uninitialized. I'm afraid this assertion fails if @dst happens to be > null. >=20 > Can we fix this by removing the "except at the root" special case? > Change qapi_clone to initialize dst =3D src, drop QapiCloneVisitor memb= er > @root and qapi_clone_visitor_new() parameter @src. Cool idea! And it avoids the crash (I was indeed compiling without optimization, and getting lucky that the uninit value wasn't crashing my tests; wonder why valgrind wasn't flagging it). > [...] > bool visit_is_input(Visitor *v) > { > return v->type =3D=3D VISITOR_INPUT; > } >=20 > This answers my question "Does the clone visitor count as input visitor= > here?" Remaining question: "Should it?" I'm still not convinced, again on the grounds that this is used for cleanup after a failed visit, but clone visits don't fail. >=20 >> @@ -252,9 +252,10 @@ void visit_type_str(Visitor *v, const char *name,= char **obj, Error **errp) >> assert(obj); >> /* TODO: Fix callers to not pass NULL when they mean "", so that = we >> * can enable: >> - assert(v->type !=3D VISITOR_OUTPUT || *obj); >> + assert(!(v->type & VISITOR_OUTPUT) || *obj); >> */ >> v->type_str(v, name, obj, &err); >> + /* Likewise, use of NULL means we can't do (v->type & VISITOR_INP= UT) */ >> if (v->type =3D=3D VISITOR_INPUT) { >> assert(!err !=3D !*obj); >> } >=20 > If your head doesn't hurt by know, you either wrote this, or you're not= > reading closely :) And there's my idea of making the clone visitor auto-magically clone NULL into "", at which point the conditions in the assertions would chang= e. >=20 > If the TODOs were already addressed, we'd again get the same analysis a= s > for visit_start_struct(), except for the arguments about the root, whic= h > don't apply here, because the clone visitor doesn't accept scalar roots= =2E >=20 > In the current state, the analysis needs to be modified as follows. >=20 > First assertion: >=20 > Before the patch, we'd like to assert "if output or clone visitor, then= > *obj isn't null". We can't as long as we need to treat null as the > empty string. >=20 > After the patch, the situation is the same for the clone visitor. Okay= =2E >=20 > Second assertion: >=20 > Before the patch, we assert "input visitor must either fail or create > *obj for a real walk." The TODO doesn't apply; we create "", not null.= >=20 > After the patch, we'd like to assert the same for the clone visitor, bu= t > we can't: the clone of null is null. Okay. >=20 >> @@ -273,9 +274,9 @@ void visit_type_any(Visitor *v, const char *name, = QObject **obj, Error **errp) >> Error *err =3D NULL; >> >> assert(obj); >> - assert(v->type !=3D VISITOR_OUTPUT || *obj); >> + assert(!(v->type & VISITOR_OUTPUT) || *obj); >> v->type_any(v, name, obj, &err); >> - if (v->type =3D=3D VISITOR_INPUT) { >> + if (v->type & VISITOR_INPUT) { >> assert(!err !=3D !*obj); >> } >> error_propagate(errp, err); >=20 > v->type_any() will crash for the clone visitor, so these changes aren't= =2E > necessary. If you want them to future-proof the code, I need to > convince myself the changes make sense, similar to what I did for the > other ones in this file. Okay, I can leave this hunk out for now. >=20 >> @@ -342,4 +343,5 @@ void visit_type_enum(Visitor *v, const char *name,= int *obj, >> } else if (v->type =3D=3D VISITOR_OUTPUT) { >> output_type_enum(v, name, obj, strings, errp); >> } >> + /* dealloc and clone visitors have nothing to do */ >> } >=20 > I'm upgrade my verdict from "subtle" to "scarily subtle" %-} >=20 Any comments I can add to make it more obvious to the next reader? >> + >> +static void qapi_clone_start_struct(Visitor *v, const char *name, voi= d **obj, >> + size_t size, Error **errp) >> +{ >> + QapiCloneVisitor *qcv =3D to_qcv(v); >> + >> + if (!obj) { >> + /* Only possible when visiting an alternate's object >> + * branch. Nothing to do here, since the earlier >> + * visit_start_alternate() already copied memory. */ >=20 > Should visitor-impl.h explain how method start_struct() is used with > alternates? I once again forgot how this works... Hmm, you explained > it to me during review of v3. >=20 > Despite there's "nothing to do here", you found something to do: >=20 >> + assert(qcv->depth); That's really only asserting that the clone itself is a real visit; we don't allow cloning on a virtual visit, even though the real visit of an alternate also involves the virtual visit of an object, if the alternate's object branch is selected. > [Skipping the tests for now to get this review out today...] Did you want to review the tests in any further detail? --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --HgjBdBlp304xMSiUat6X0Gkw0DdobE93T 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/ iQEcBAEBCAAGBQJXWO1nAAoJEKeha0olJ0NqZNoIAJUKj+eTCnuW2qtnShUyNqxO /opq4CHESHGlnFRncEcvC8ZvzmiP6Bi/ivRUOl5B8Vl8ZfmcRgx5vZaDVpDjmICK tNt1TXtX818lNytag3O664QhfEsi6Oai9FPPnvFdFCZgudz6fgZ6/Vz0+AjU03+a Bp2F6J6svIo15Ws73JTbx0vEuw6lqz3FtrDSJC8dLBqYRYLH5pd9n8MdG8ElDeU3 CP3VhWw9ZaHf0To0exPHRg3Qjj7b/+Kh5sjImgB/SViRxLukqohC1nrYPuXal9Em oTBicgweDdZNrHXJy1YvUMPjJUMYb8K5mPZwm0AMeEGhhtKhpWCiHS+KwqRdV3M= =p2L2 -----END PGP SIGNATURE----- --HgjBdBlp304xMSiUat6X0Gkw0DdobE93T--