From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37472) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZjqA1-0008G3-DM for qemu-devel@nongnu.org; Wed, 07 Oct 2015 10:57:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zjq9y-0004Q3-2S for qemu-devel@nongnu.org; Wed, 07 Oct 2015 10:57:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57974) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zjq9x-0004Py-P4 for qemu-devel@nongnu.org; Wed, 07 Oct 2015 10:57:38 -0400 References: <8737xylxce.fsf@blackfin.pond.sub.org> <1444165843-3695-1-git-send-email-eblake@redhat.com> <877fmydh22.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <561532DB.1010301@redhat.com> Date: Wed, 7 Oct 2015 08:57:31 -0600 MIME-Version: 1.0 In-Reply-To: <877fmydh22.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="lSUfxtfWr9p0BPf6gQss6RhkGCIlCqlld" Subject: Re: [Qemu-devel] [RFC PATCH] qapi: split visit_end_struct() into pieces List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: "Michael S. Tsirkin" , qemu-devel@nongnu.org, Michael Roth , Luiz Capitulino , Paolo Bonzini , =?UTF-8?Q?Andreas_F=c3=a4rber?= This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --lSUfxtfWr9p0BPf6gQss6RhkGCIlCqlld Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 10/07/2015 06:00 AM, Markus Armbruster wrote: >>> Looks like we're getting drawn into visitor contract territory again.= >>> >> +++ b/hmp.c >> @@ -1658,8 +1658,9 @@ void hmp_object_add(Monitor *mon, const QDict *q= dict) >> >> object_add(type, id, pdict, opts_get_visitor(ov), &err); >> >> + visit_check_struct(opts_get_visitor(ov), &err_end); >> out_end: >> - visit_end_struct(opts_get_visitor(ov), &err_end); >> + visit_end_struct(opts_get_visitor(ov)); >> if (!err && err_end) { >> qmp_object_del(id, NULL); >> } >=20 > Preexisting: calling object_add() before visit_end_struct() is awkward.= > Can we simplify things now we have separate visit_check_struct() and > visit_end_struct()? Call visit_check_struct() before object_add(), > bypass object_add() on error, avoiding the need to undo it with > qmp_object_del(). Okay, it sounds like I'm sitting on a pile of pre-patch cleanups, and that I'm on the right track for having done the split. >> +++ b/include/qapi/visitor.h >> @@ -56,11 +56,19 @@ typedef struct GenericList >> void visit_start_struct(Visitor *v, void **obj, const char *kind, >> const char *name, size_t size, Error **errp);= >> /** >> + * Check whether completing a struct is safe. >=20 > "Safe"? We need to complete the struct visit with visit_end_struct() > regardless of what this function returns... >=20 >> + * Should be called prior to visit_end_struct() if all other intermed= iate >> + * visit steps were successful, to allow the caller one last chance t= o >> + * report errors such as remaining data that was not consumed by the = visit. >> + */ >> +void visit_check_struct(Visitor *v, Error **errp); Maybe: Declare the current struct complete, and check for unvisited contents. >> static void >> -opts_end_struct(Visitor *v, Error **errp) >> +opts_check_struct(Visitor *v, Error **errp) >> { >> OptsVisitor *ov =3D DO_UPCAST(OptsVisitor, visitor, v); >> GQueue *any; >=20 > if (--ov->depth > 0) { >=20 > Do we want to decrement ov->depth here? We'll decrement it again in > opts_end_struct()... Oh, good catch. This was an awkward split, and I got it off-by-one. >> +++ b/qapi/qmp-input-visitor.c >> @@ -88,14 +88,14 @@ static void qmp_input_push(QmpInputVisitor *qiv, Q= Object *obj, Error **errp) >> qiv->nb_stack++; >> } >> >> -/** Only for qmp_input_pop. */ >> +/** Only for qmp_input_check. */ >=20 > Drop the comment instead? >=20 > Aside: a loop would be more idiomatic C. Leave higher order functions > to languages that are actually equipped for the job. >=20 >> static gboolean always_true(gpointer key, gpointer val, gpointer user= _pkey) >> { >> *(const char **)user_pkey =3D (const char *)key; >> return TRUE; >> } >> >> -static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp) >> +static void qmp_input_check(QmpInputVisitor *qiv, Error **errp) >> { >> assert(qiv->nb_stack > 0); >> >> @@ -107,6 +107,17 @@ static void qmp_input_pop(QmpInputVisitor *qiv, E= rror **errp) >> g_hash_table_find(top_ht, always_true, &key); always_true() exists for g_hash_table_find() - unless you know of some other way to grab any arbitrary element of the hash table that doesn't require a higher-order function. >> +++ b/scripts/qapi-event.py >> @@ -73,13 +73,14 @@ def gen_event_send(name, arg_type): >> ret +=3D gen_err_check() >> ret +=3D gen_visit_fields(arg_type.members, need_cast=3DTrue)= >> ret +=3D mcgen(''' >> - visit_end_struct(v, &err); >> + visit_check_struct(v, &err); >> if (err) { >> goto out; >> } >> + visit_end_struct(v); >> >> obj =3D qmp_output_get_qobject(qov); >> - g_assert(obj !=3D NULL); >> + g_assert(obj); >=20 > I prefer the more laconic form myself, but it's an unrelated change. I can split that one-line change into a more appropriate patch. >> out_obj: >> error_propagate(errp, err); >> err =3D NULL; >> visit_end_union(v, !!(*obj)->data, &err); >=20 > Should visit_end_union() be similarly split? Or should its Error ** > parameter be dropped? As far as I can tell, no visitor implements this= > method... >=20 visit_end_union() gets completely dropped in a different patch. See v5 28/46. >> +++ b/tests/test-qmp-output-visitor.c >> @@ -194,10 +194,11 @@ static void visit_type_TestStruct(Visitor *v, Te= stStruct **obj, >> } >> visit_type_str(v, &(*obj)->string, "string", &err); >> >> + if (!err) { >> + visit_check_struct(v, &err); >> + } >=20 > ... but here, you guard it with an if. Either way works, but I'd like > us to pick just one for the generators. Sure. >> - for (*head =3D i =3D visit_next_list(v, head, errp); i; i =3D vis= it_next_list(v, &i, errp)) { >> + for (*head =3D i =3D visit_next_list(v, head, &err); >> + !err && i; >> + i =3D visit_next_list(v, &i, &err)) { >> TestStructList *native_i =3D (TestStructList *)i; >> - visit_type_TestStruct(v, &native_i->value, NULL, errp); >> + visit_type_TestStruct(v, &native_i->value, NULL, &err); >> } >=20 > Is this a silent bug fix? Before your patch, the loop doesn't break on= > error. >=20 Yes, looks like it. And all the more reason for our test code to NOT hand-write this, but to rely on the generator (so that we are testing a single version of visit_* calls, rather than subtle differences in generated vs. hand-rolled). >> +++ b/vl.c >> @@ -2796,23 +2796,25 @@ static int object_create(void *opaque, QemuOpt= s *opts, Error **errp) >> qdict_del(pdict, "qom-type"); >> visit_type_str(opts_get_visitor(ov), &type, "qom-type", &err); >> if (err) { >> - goto out; >> + goto obj_out; >> +obj_out: >> + visit_end_struct(opts_get_visitor(ov)); >> if (err) { >> qmp_object_del(id, NULL); >> } >=20 > Silent bug fix: we now call visit_end_struct() even on error. Impact? > Separate patch? Yes, separate patch, and I'll evaluate impact there. So sounds like I should proceed with this RFC (which means more respin of my other patches, before I can post subset C - but that's okay, since we aren't even through reviewing subset B, nor is subset A in a pull request). --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --lSUfxtfWr9p0BPf6gQss6RhkGCIlCqlld 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/ iQEcBAEBCAAGBQJWFTLbAAoJEKeha0olJ0Nq//UH/i2gxUfn18FLGgflOjPIvbjR vKGEl8+OYtUB2rCOp8FaNGPLsTCw5z8QgCbLOgzMIZvNQAMZ4Kjk22wwZLf6lsAA TQjlWtWA+Fkhoy3HR1WhBzOd3k8LbQclZnYkVg3X24Ix+GcKEdpph6lCqpylSdzY JJjjV2i07DJ2Iqc9YIxkiWQcLz77XWcexYogdkVel8urM6o4fi4lG+bgbe5+nCru g3xffMLhiO+qiqeuHruCul5mn8Q0xT4WRCR1AzcK/qlq07VTLQZloufF4pUIrP6V lR+MzQcwIFeASdhoFCTXUa9E0PIhji5NUSsE1vl9T2C01A1KmqrRfe6D1xPIRLg= =1+Y9 -----END PGP SIGNATURE----- --lSUfxtfWr9p0BPf6gQss6RhkGCIlCqlld--