From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58918) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1avVyq-0006of-OE for qemu-devel@nongnu.org; Wed, 27 Apr 2016 16:22:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1avVym-0007CH-Nl for qemu-devel@nongnu.org; Wed, 27 Apr 2016 16:22:40 -0400 References: <1460131992-32278-1-git-send-email-eblake@redhat.com> <1460131992-32278-19-git-send-email-eblake@redhat.com> <87fuunq7za.fsf@dusky.pond.sub.org> <87k2jqhvye.fsf@dusky.pond.sub.org> <874matho4r.fsf@dusky.pond.sub.org> From: Eric Blake Message-ID: <57211F88.70807@redhat.com> Date: Wed, 27 Apr 2016 14:22:32 -0600 MIME-Version: 1.0 In-Reply-To: <874matho4r.fsf@dusky.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="E1A7KBlGEV3RSUkvabXbSwDbKXQIxwrV9" Subject: Re: [Qemu-devel] [PATCH v14 18/19] qapi: Simplify semantics of visit_next_list() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, Alexander Graf , David Gibson , "open list:sPAPR" , Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --E1A7KBlGEV3RSUkvabXbSwDbKXQIxwrV9 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 04/22/2016 05:35 AM, Markus Armbruster wrote: >>>> >>>> static void >>>> -start_list(Visitor *v, const char *name, Error **errp) >>>> +start_list(Visitor *v, const char *name, GenericList **list, size_t= size, >>>> + >>>> + parse_str(siv, &err); >>>> + if (err) { >>>> + *list =3D NULL; >>>> + error_propagate(errp, err); >>>> + return; >>>> + } >> >> parse_str() never sets an error, and therefore your new error check is= >> dead. Just as well, because it would be wrong. >> >> parse_str() parses a complete string into a non-empty list of uint64_t= >> ranges. On success, it sets siv->ranges to this list. On error, it >> sets it to null. It could also set an error then, but it doesn't. >> >> If it did, then what would start_list() do with it? Reporting it woul= d >> be wrong, because the list members need not be integers. parse_str() is only ever called for start_list and parse_type_int64 - so the real question is do we ever support the string input visitor on anything other than an integral list. Per the comments I'm adding earlier in the series: * The string input visitor does not implement support for visiting * QAPI structs, alternates, null, or arbitrary QTypes. so it sounds like the only lists it supports ARE integer lists. >> >> If they aren't, the speculative parse_str()'s failure will be ignored.= >> >> If they are, parse_type_int64() will call parse_str() again, then use >> siv->ranges. >> >> If the first parse_str() succeeds, the second will do nothing, and we'= ll >> use the first one's siv->ranges. Works. >> >> If the first parse_str() fails, the second will fail as well, because >> its input is the same. We'll use the second one's failure. Works. >=20 > No, it doesn't: failure gets interpreted as empty list. I'll post my > test case separately. >=20 >> When used outside list context, parse_type_int64() will call parse_str= () >> for the first time, and use its result. Works. >> >> Note that opts-visitor does it differently: opts_start_list() doesn't >> parse numbers, opts_type_int64() and opts_type_uint64() do. I like the approach used in opts-visitor (start_list should only check if a list is present, but save the parsing for when the items are actually consumed off the list). But opts-visitor also handles structs, unlike the string visitor, which forces the separation (at start_list, we don't know what the list element will be, unlike the string visitor where we know the list element is integral). >> >> Further note the latent bug in parse_type_int64(): we first call >> parse_str(siv, errp), and goto error if it fails, where we promptly >> error_setg(errp, ...). If parse_str() set an error, the error_setg() >> would fail error_setv()'s assertion. >> >> Please drop parse_str()'s unused errp parameter, and add a comment to >> start_list() explaining the speculative call to parse_str() there. >=20 > Insufficient, doesn't fix the bug. After parse_str(), we need to be > able to distinguish empty list from error. Moving the error_set() into= > parse_str() could work. Returning succes/failure and dropping the errp= > parameter could also work. I'm playing with these ideas, but will get the bug fixed (along with your testsuite addition) as a prerequisite to the list refactoring (to make sure that the refactored list still passes the fixed test). >=20 >> Alternatively, change the string visitor to work like the opts visitor= =2E Trickier, but may be my only option if the other approaches don't work. Thanks again for spotting yet another ugly corner case worth fixing (this series seems to have been a never-ending source of them...) --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --E1A7KBlGEV3RSUkvabXbSwDbKXQIxwrV9 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/ iQEcBAEBCAAGBQJXIR+IAAoJEKeha0olJ0Nqmi4H/3cCgfRHOPdugR1xahgfPe0L 5pNRqNNOIYgtSrN4/FCjy447O5gIdwnj1IjGnLzxtl3EOhQZnp0gDMizOQS7uqIE eH9u1HoJ4a4mazExo4MVzAwWGhVX36QMj1ibJ/cei2tLFZGrT4xhb8ySi1/+0/SL ptuDqon2ALrRqm7Cj9+MzDtBJdT8NTEu4pdJTWMqgwfvDDCX3U67dSZ500666H9F ExKLXUzKl1L7uS0+lL4eWMZ0vfeefLC+vS2i8BqEc+JRx1H3dSmPlpu/DbMjaOQZ 3CXnqqLGpaN6JiMW87hZntPkybUl903dInoeXqZ4osXK09f0/qEtKKFme6yflA4= =i3ic -----END PGP SIGNATURE----- --E1A7KBlGEV3RSUkvabXbSwDbKXQIxwrV9--