From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46845) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aqNm4-0008Dy-Kd for qemu-devel@nongnu.org; Wed, 13 Apr 2016 12:36:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aqNm1-0003qX-Ej for qemu-devel@nongnu.org; Wed, 13 Apr 2016 12:36:16 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54450) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aqNm1-0003qM-6V for qemu-devel@nongnu.org; Wed, 13 Apr 2016 12:36:13 -0400 References: <1460131992-32278-1-git-send-email-eblake@redhat.com> <1460131992-32278-6-git-send-email-eblake@redhat.com> <871t69ld38.fsf@dusky.pond.sub.org> From: Eric Blake Message-ID: <570E757B.5070909@redhat.com> Date: Wed, 13 Apr 2016 10:36:11 -0600 MIME-Version: 1.0 In-Reply-To: <871t69ld38.fsf@dusky.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="uVUg6ImhfBKPNtEBrRimAvbh036b5H60d" Subject: Re: [Qemu-devel] [PATCH v14 05/19] qmp-input: Clean up stack handling 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) --uVUg6ImhfBKPNtEBrRimAvbh036b5H60d Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 04/13/2016 09:53 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Management of the top of stack was a bit verbose; creating a >> temporary variable and adding some comments makes the existing >> code more legible before the next few patches improve things. >> No semantic changes other than asserting that we are always >> visiting a QObject, and not a NULL value. >> >> Signed-off-by: Eric Blake >> >> --- >=20 > The mixture of block comments and comments to the right is a bit > awkward. What about: >=20 > typedef struct StackObject { > QObject *obj; /* Object being visited */ >=20 > GHashTable *h; /* if obj is dict: unvisited keys */= > const QListEntry *entry; /* if obj is list: unvisited tail */= > } StackObject; >=20 Works for me. >> >> struct QmpInputVisitor >> { >> Visitor visitor; >> + >> + /* Stack of objects being visited. stack[0] is root of visit, >> + * stack[1] and below correspond to visit_start_struct (nested >> + * QDict) and visit_start_list (nested QList). */ >=20 > I guess what you want to say is stack[1..] record the nesting of > start_struct() ... end_struct() and start_list() ... end_list() pairs. >=20 > Comment gets rewritten in PATCH 17, no need to worry too much about it.= >=20 >> StackObject stack[QIV_STACK_SIZE]; >> int nb_stack; >> + >> + /* True to track whether all keys in QDict have been parsed. */ >> bool strict; >=20 > I think @strict switches on rejection of unexpected dictionary keys. > See qmp_input_pop() below. >=20 > I dislike the fact that we have two input visitors, and the one with th= e > obvious name ignores certain errors. I don't doubt that it has its > uses, but reporting errors should be the default, and ignoring them > should be a conscious decision. Anyway, not this patch's problem. Dan also has a pending patch that reworks it to add yet another parameter (the ability to take input in string format and auto-convert it to the correct type). In that one, he exposes a third method for choosing which visitor you get, and which then under the hood call a helper with two boolean flags. Maybe it's time to just convert all clients to always passing the parameters they want, along with auditing whether ignoring extra input is a sane option for that client - but as you say, it's fine for a separate patch. >> + >> + /* If we have a name, and we're in a dictionary, then return that= >> + * value. */ >=20 > Can we be in a dictionary and not have a name? The converse happens: we can certainly have a name and not be in a dictionary, for a top-level visit. But it has weird semantics until I clean it up later in 17/19. For this patch, it was just code motion and documentation (the 'if (name && qobject_type...)' condition here is the same pre- and post-patch), where I was just getting rid of a dead 'if (qobj)'. >=20 >=20 > static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp) > { > assert(qiv->nb_stack > 0); >=20 > if (qiv->strict) { > GHashTable * const top_ht =3D qiv->stack[qiv->nb_stack - 1].= h; > if (top_ht) { > GHashTableIter iter; > const char *key; >=20 > g_hash_table_iter_init(&iter, top_ht); > if (g_hash_table_iter_next(&iter, (void **)&key, NULL)) = { > error_setg(errp, QERR_QMP_EXTRA_MEMBER, key); >=20 > This looks wrong. If we have more than one extra members, the second > call error_setg() will fail an assertion in error_setv(), unless errp i= s > null. Whoops - looks like f96493b1 is broken for missing a 'break' statement. I'll send that as a separate for-2.6 cleanup that we should pull sooner rather than later. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --uVUg6ImhfBKPNtEBrRimAvbh036b5H60d 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/ iQEcBAEBCAAGBQJXDnV7AAoJEKeha0olJ0NqDO4H/3aU/8Q/oW4Kg8wPrpPhJI+j NUWkk42kUN703yYTrVewCR9j/cBr0PqRFa3X6MG3puOCWFdlCCTp8bmwscZ9rRcR qKApA2CyLo38UupXPmI7ku9pLgQEG0jsRKStvASN028+cbVOEyZOopPL9exv+ino hcZTpzyhi2SQbz/d9qkmKb/LBAM57ZKkXw/kXP4cK0z+1ef5DMXMBbeWXdErOysi RwlnS9Tn5VMI+kcZxuXaZMOrniMes8YTYKbHxG8pAIjmp8AkyfquuMCpXv4WS5PM X11/x/zHKvyeSeY7FEKrVy8/xW9vFtctDteDmSAFxUpEgUDfdxG8djAjx0E4zgA= =Ngib -----END PGP SIGNATURE----- --uVUg6ImhfBKPNtEBrRimAvbh036b5H60d--