From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37974) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjkrG-0004Bn-LX for qemu-devel@nongnu.org; Tue, 13 Sep 2016 06:22:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bjkrC-0001bE-Sn for qemu-devel@nongnu.org; Tue, 13 Sep 2016 06:22:30 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59186) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjkrC-0001ao-L0 for qemu-devel@nongnu.org; Tue, 13 Sep 2016 06:22:26 -0400 Date: Tue, 13 Sep 2016 11:22:22 +0100 From: "Daniel P. Berrange" Message-ID: <20160913102222.GN30949@redhat.com> Reply-To: "Daniel P. Berrange" References: <1473088600-17930-1-git-send-email-berrange@redhat.com> <1473088600-17930-6-git-send-email-berrange@redhat.com> <160921d9-efab-50c4-ed67-67866c240345@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <160921d9-efab-50c4-ed67-67866c240345@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v11 5/6] qapi: add a QmpInputVisitor that does string conversion List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, Markus Armbruster , Max Reitz , =?utf-8?Q?Marc-Andr=C3=A9?= Lureau , Paolo Bonzini , Andreas =?utf-8?Q?F=C3=A4rber?= On Mon, Sep 12, 2016 at 01:39:50PM -0500, Eric Blake wrote: > On 09/05/2016 10:16 AM, Daniel P. Berrange wrote: > > Currently the QmpInputVisitor assumes that all scalar > > values are directly represented as their final types. > > ie it assumes an 'int' is using QInt, and a 'bool' is > > using QBool. > >=20 > > This adds an alternative constructor for QmpInputVisitor > > that will set it up such that it expects a QString for > > all scalar types instead. > >=20 > > This makes it possible to use QmpInputVisitor with a > > QDict produced from QemuOpts, where everything is in > > string format. > >=20 > > Reviewed-by: Marc-Andr=C3=A9 Lureau > > Signed-off-by: Daniel P. Berrange > > --- > > include/qapi/qobject-input-visitor.h | 41 +++++++++- > > qapi/qobject-input-visitor.c | 115 +++++++++++++++++++++++++= - > > tests/test-qobject-input-visitor.c | 152 +++++++++++++++++++++++++= +++++++++- > > 3 files changed, 298 insertions(+), 10 deletions(-) > >=20 >=20 > > +/** > > + * qobject_string_input_visitor_new: > > + * @obj: the input object to visit > > + * > > + * Create a new input visitor that converts a QObject to a QAPI obje= ct. > > + * > > + * Any scalar values in the @obj input data structure should always = be > > + * represented as strings. i.e. if visiting a boolean, the value sho= uld > > + * be a QString whose contents represent a valid boolean. > > + * > > + * The visitor always operates in strict mode, requiring all dict ke= ys > > + * to be consumed during visitation. >=20 > Good; I like that strict mode on the new constructor is not optional. >=20 >=20 > > +static void qobject_input_type_int64_str(Visitor *v, const char *nam= e, > > + int64_t *obj, Error **errp) > > +{ > > + QObjectInputVisitor *qiv =3D to_qiv(v); > > + QString *qstr =3D qobject_to_qstring(qobject_input_get_object(qi= v, name, > > + true= )); > > + uint64_t ret; >=20 > Uninitialized... >=20 > > + > > + parse_option_number(name, qstr ? qstr->string : NULL, &ret, errp= ); >=20 > ...and parse_option_number() explicitly leaves *ret untouched on error.= .. >=20 > > + *obj =3D ret; >=20 > so if errp was set, then *obj now contains uninitialized memory. I > guess valgrind is smart enough to only complain if callers then try to > branch based on that value (that is, assigning one uninit location to > another silently propagates uninit status to the new location, but it i= s > only when you branch or otherwise USE uninit data, not just copy it, > that valgrind complains). On the other hand, if the caller explicitly > set value =3D 0 before calling qobject_input_type_int64_str(&value), we= 've > now messed with the caller's expectations of value being at its pre-set > value on error. I'll use a local Error, so we can avoid the uninitialized value and also avoid splattering *obj on failure. > > +++ b/tests/test-qobject-input-visitor.c > > =20 > > +static void test_visitor_in_int_autocast(TestInputVisitorData *data, > > + const void *unused) > > +{ > > + int64_t res =3D 0, value =3D -42; > > + Visitor *v; > > + > > + v =3D visitor_input_test_init_full(data, true, true, > > + "\"-42\""); > > + > > + visit_type_int(v, NULL, &res, &error_abort); > > + g_assert_cmpint(res, =3D=3D, value); > > +} > > + > > +static void test_visitor_in_int_noautocast(TestInputVisitorData *dat= a, > > + const void *unused) > > +{ > > + int64_t res =3D 0; > > + Visitor *v; > > + Error *err =3D NULL; > > + > > + v =3D visitor_input_test_init(data, "\"-42\""); > > + > > + visit_type_int(v, NULL, &res, &err); > > + g_assert(err !=3D NULL); > > + error_free(err); > > +} >=20 > So we've previously tested that int->int works without autocast, and yo= u > add that str->int works with autocast, and that str->int fails without > autocast. Is it also worth testing that int->int fails with autocast > (that is, when doing string parsing, a QInt is intentionally rejected > even though we are parsing to get an int result)? [snip] > Similar question for autocast causing QBool->bool, QInt->int under size= , > and QFloat->number failures. You always notice all the edge cases :-) I've added such tests and it exposed a bug in my impl which is nice :-) Regards, Daniel --=20 |: http://berrange.com -o- http://www.flickr.com/photos/dberrange= / :| |: http://libvirt.org -o- http://virt-manager.or= g :| |: http://autobuild.org -o- http://search.cpan.org/~danberr= / :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vn= c :|