From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35858) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zal4u-0003d6-Cr for qemu-devel@nongnu.org; Sat, 12 Sep 2015 09:42:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zal4p-0001Wv-CP for qemu-devel@nongnu.org; Sat, 12 Sep 2015 09:42:52 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42901) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zal4p-0001Wi-2T for qemu-devel@nongnu.org; Sat, 12 Sep 2015 09:42:47 -0400 References: <1441999089-28960-1-git-send-email-armbru@redhat.com> <1441999089-28960-21-git-send-email-armbru@redhat.com> <55F33D06.1090103@redhat.com> <55F3411B.5070905@redhat.com> <87a8ssf47y.fsf@blackfin.pond.sub.org> <55F41788.6030008@redhat.com> From: Eric Blake Message-ID: <55F42BD0.3000401@redhat.com> Date: Sat, 12 Sep 2015 07:42:40 -0600 MIME-Version: 1.0 In-Reply-To: <55F41788.6030008@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="7Qff5KfGLMJMRamBo3O82gHUpVL6AiP7s" Subject: Re: [Qemu-devel] [PATCH v6 20/26] qapi: Fix output visitor to return qnull() instead of NULL List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --7Qff5KfGLMJMRamBo3O82gHUpVL6AiP7s Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 09/12/2015 06:16 AM, Eric Blake wrote: >> >> where e =3D QTAILQ_LAST(&qov->stack, QStack) >> >> Questions: >> >> * How can e become NULL? >> >> The only place that pushes onto &qov->stack appears to be >> qmp_output_push_obj(). Can obviously push e with !e->value, but can= 't >> push null e. >=20 > My understanding is that qov->stack corresponds to nesting levels of {}= > or [] in the JSON code. The testsuite shows a case where the stack is > empty as one case where e is NULL, but if e is non-NULL, I'm not sure > whether e->value can ever be NULL. I'll have to read the code more clo= sely. I agree that qmp_object_push_obj() is the only place that pushes; and it promptly calls qobject_type(e->value) which blindly dereferences value->type. So it sounds like e->value can never be NULL (or we would segfault), although a well-placed assert in qmp-output-visitor.c would be nicer than having to chase through qobject.h. So the only way for qmp_object_first() to return NULL is if e is NULL (the stack was empty), since e->value would be non-NULL. Your patch fixed it to never return NULL, mine fixed it to handle a NULL return in callers that care (only 1 of the 2 callers). >=20 >> >> * What about qmp_output_last()? >> >> Why does qmp_output_first() check for !e, but not qmp_output_last()?= >> >> My patch makes them less symmetric (bad smell). Yours makes them mo= re >> symmetric, but not quite. >=20 > Which is awkward in its own right. qmp_output_last() is only used by qmp_output_add_obj(), which first checks for an empty queue; so the queue cannot be empty. Therefore, qmp_output_last() could assert that e !=3D NULL. At any rate, qmp_output_last() never returns NULL; pre-patch, only qmp_output_first() could do so. And qmp_output_add_obj() blindly calls qobject_type() on the result of qmp_output_last(), which as argued before would segfault if e->value could ever be NULL. It looks like the role of qmp_output_last() is to determine the last thing that was being output; if it is a QDict or QList, then add the current visit on to that existing object; otherwise, the last thing output is a scalar and is complete, so we are replacing the root with the new object being output. Meanwhile, the role of qmp_output_first() appears to be to grab the root of the output tree - either to give the caller the overall QObject* created by visiting a structure (qmp_output_get_qobject, which must not return NULL), or to clean up all references still stored in the stack. It also looks like qmp_output_get_qobject() can be called multiple times before cleanup (to grab copies of the current root). >=20 >> >> * How does this contraption work? >=20 > Indeed. Without reading further, we're both shooting in the dark for > something that makes tests pass, but without being a clean interface. It looks like the stack is actually tracking two things at once: the oldest member of the stack (qmp_output_first(), or QTAILQ_LAST) is the root (can be any QObject), and all other members of the stack hold any open-ended container QObject (necessarily QDict or QList) that is still having contents added (the newest member is qmp_output_last(), or QTAILQ_FIRST). It's a bit confusing that QTAILQ works in the opposite direction of our terminology. Hmm, qmp_output_add_obj() is still a bit odd. If, on a brand new visitor, we try to visit two scalars in a row (via consecutive visit_type_int()), the first scalar will become the stack root, then the second scalar will replace the first as the root. But if we try to visit two QDict in a row (via consecutive visit_start_struct(),visit_type_FOO()...,visit_end_struct() sequences), the first QDict becomes the stack root, gets pushed onto the stack a second time to be the open-ended container for the visit_type_FOO() calls, then gets popped only once from the stack when complete. That means the second QDict will attempt to add itself into the existing root, instead of replacing the root. A better behavior would be to blindly replace the root node if the stack has exactly one element (so that we are consistent on behavior when using a single visitor on two top-level visits in a row), but that should be a separate patch - in particular, I don't know how often we ever use a visitor on consecutive top-level items to need to replace the root (our testsuite allocates a new visitor every time, instead of reusing visitors). More in another ma= il. For the sake of our current issue, I think that adding comments to the existing behavior is sufficient to explain why my approach works. So how about squashing this: diff --git c/qapi/qmp-output-visitor.c w/qapi/qmp-output-visitor.c index 2d6083e..d42ca0e 100644 --- c/qapi/qmp-output-visitor.c +++ w/qapi/qmp-output-visitor.c @@ -41,10 +41,12 @@ static QmpOutputVisitor *to_qov(Visitor *v) return container_of(v, QmpOutputVisitor, visitor); } +/* Push @value onto the stack of current QObjects being built */ static void qmp_output_push_obj(QmpOutputVisitor *qov, QObject *value) { QStackEntry *e =3D g_malloc0(sizeof(*e)); + assert(value); e->value =3D value; if (qobject_type(e->value) =3D=3D QTYPE_QLIST) { e->is_list_head =3D true; @@ -52,39 +54,51 @@ static void qmp_output_push_obj(QmpOutputVisitor *qov, QObject *value) QTAILQ_INSERT_HEAD(&qov->stack, e, node); } +/* Grab and remove the most recent QObject from the stack */ static QObject *qmp_output_pop(QmpOutputVisitor *qov) { QStackEntry *e =3D QTAILQ_FIRST(&qov->stack); QObject *value; + + assert(e); QTAILQ_REMOVE(&qov->stack, e, node); value =3D e->value; g_free(e); return value; } +/* Grab the root QObject, if any, in preparation to empty the stack */ static QObject *qmp_output_first(QmpOutputVisitor *qov) { QStackEntry *e =3D QTAILQ_LAST(&qov->stack, QStack); if (!e) { - return qnull(); + /* No root */ + return NULL; } - + assert (e->value); return e->value; } +/* Grab the most recent QObject from the stack, which must exist */ static QObject *qmp_output_last(QmpOutputVisitor *qov) { QStackEntry *e =3D QTAILQ_FIRST(&qov->stack); + + assert(e); return e->value; } +/* Add @value to the current QObject being built. + * If the stack is visiting a dictionary or list, @value is now owned + * by that container. Otherwise, @value is now the root. */ static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name, QObject *value) { QObject *cur; if (QTAILQ_EMPTY(&qov->stack)) { + /* Stack was empty, track this object as root */ qmp_output_push_obj(qov, value); return; } @@ -93,13 +107,16 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name, switch (qobject_type(cur)) { case QTYPE_QDICT: + assert(name); qdict_put_obj(qobject_to_qdict(cur), name, value); break; case QTYPE_QLIST: qlist_append_obj(qobject_to_qlist(cur), value); break; default: + /* The previous root was a scalar, replace it with a new root */= qobject_decref(qmp_output_pop(qov)); + assert(QTAILQ_EMPTY(&qov->stack)); qmp_output_push_obj(qov, value); break; } @@ -185,11 +202,14 @@ static void qmp_output_type_number(Visitor *v, double *obj, const char *name, qmp_output_add(qov, name, qfloat_from_double(*obj)); } +/* Finish building, and return the root object. Will not be NULL. */ QObject *qmp_output_get_qobject(QmpOutputVisitor *qov) { QObject *obj =3D qmp_output_first(qov); if (obj) { qobject_incref(obj); + } else { + obj =3D qnull(); } return obj; } --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --7Qff5KfGLMJMRamBo3O82gHUpVL6AiP7s 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/ iQEcBAEBCAAGBQJV9CvQAAoJEKeha0olJ0NqV0sIAKaW3CMIuc2F5PnGSrwVsCj2 h4g1ifK6roxcgPbFYwYJ963k+L3oaUY99pcCSrFDp6/LxKg44eZ4XTbdmmD2JR7O GyITzySEUif25jndgHPp70WlXVgKKKmqlzpaiSST+NGtpiORUxrM1dw2WGbN5upc Trlv5eJ5ZyN8spumni6iYLw32vSnQHmhK3Q7d9wgg0EKwC/QHG5OZl3XV/rQzRte BC41M0u1cQpMBOC7pl9C3EGK1LuP+RQ2kipTPZEdoXbQIJT+k/FsVqAbI/15SP6h Nd9obnqlLR/bGB/q70bvzrlJAG+r2f9U8QCnIDn7hInjE0+N0xX/Rp+RQ4C/ePQ= =Gt8r -----END PGP SIGNATURE----- --7Qff5KfGLMJMRamBo3O82gHUpVL6AiP7s--