From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37566) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aNecp-0003bA-S5 for qemu-devel@nongnu.org; Mon, 25 Jan 2016 05:44:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aNecm-0004uM-J1 for qemu-devel@nongnu.org; Mon, 25 Jan 2016 05:43:59 -0500 Received: from mx1.redhat.com ([209.132.183.28]:48305) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aNecm-0004u7-B8 for qemu-devel@nongnu.org; Mon, 25 Jan 2016 05:43:56 -0500 References: <1453219845-30939-1-git-send-email-eblake@redhat.com> <1453219845-30939-27-git-send-email-eblake@redhat.com> <8737tpfmec.fsf@blackfin.pond.sub.org> <56A284FB.50502@redhat.com> <874me2c8ku.fsf@blackfin.pond.sub.org> From: Laszlo Ersek Message-ID: <56A5FC69.9020105@redhat.com> Date: Mon, 25 Jan 2016 11:43:53 +0100 MIME-Version: 1.0 In-Reply-To: <874me2c8ku.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v9 26/37] qapi: Simplify excess input reporting in input visitors List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , Eric Blake Cc: marcandre.lureau@redhat.com, qemu-devel@nongnu.org, Michael Roth On 01/25/16 10:27, Markus Armbruster wrote: > Eric Blake writes: >=20 >> On 01/22/2016 12:24 PM, Markus Armbruster wrote: >>> Eric Blake writes: >>> >>>> When reporting that an unvisited member remains at the end of an >>>> input visit for a struct, we were using g_hash_table_find() >>>> coupled with a callback function that always returns true, to >>>> locate an arbitrary member of the hash table. But if all we >>>> need is an arbitrary entry, we can get that from a single-use >>>> iterator, without needing a tautological callback function. >>> >>> Good idea. >>> >>>> Suggested-by: Markus Armbruster >>> >>> Whoops, it's even mine! I forgot... %-) Side remark: https://developer.gnome.org/glib/stable/glib-Hash-Tables.html g_hash_table_find (): Since: 2.4 g_hash_table_iter_next (): Since: 2.16 I can't prove that when I wrote this code, g_hash_table_iter_next() wasn't available, but I may easily have googled & looked at GLib *documentation* that lacked g_hash_table_iter_next(). :) >>> >>>> Signed-off-by: Eric Blake >>>> Reviewed-by: Marc-Andr=C3=A9 Lureau >>>> >>>> --- >> >>>> GQueue *any; >>>> >>>> if (--ov->depth > 0) { >>>> @@ -174,8 +168,8 @@ opts_end_struct(Visitor *v, Error **errp) >>>> } >>>> >>>> /* we should have processed all (distinct) QemuOpt instances */ >>>> - any =3D g_hash_table_find(ov->unprocessed_opts, &ghr_true, NULL= ); >>>> - if (any) { >>>> + g_hash_table_iter_init(&iter, ov->unprocessed_opts); >>>> + if (g_hash_table_iter_next(&iter, NULL, (void **)&any)) { >>> >>> Is this cast kosher? >> >> You may have a point that it violates some corner of C99, but I'm not >> the first such user: >> >> hw/i386/intel_iommu.c: while (g_hash_table_iter_next (&bus_it, NULL= , >> (void**)&vtd_bus)) { >> qom/object.c: while (g_hash_table_iter_next(&iter, NULL, (gpointer >> *)&prop)) { >> >> Conceptually, it seems fine - void* can be assigned to any pointer, an= d >> 'any' qualifies as such a pointer. We are then taking the address of >> that (or void**) to pass by reference. >=20 > Yes, any pointer can be converted to and from void *. Doesn't mean the > conversion results in the same bits. It does on machines with a single= , > uniform pointer format, i.e. any sane machine. >=20 > (void **)iter converts iter, not *iter. *iter gets reinterpreted on > dereference. >=20 > You're right in that this kind of type punning already occurs in QEMU. > Also in other programs. We can hope it's common enough to deter > optimizers from screwing it up even on sane machines. >=20 >> I suppose a stricter version would be: >> >> void *wrap; >> GQueue *any; >> if (g_hash_table_iter_next(&iter, NULL, &wrap)) { >> any =3D wrap; >> ... >> >> but is it worth the bother? Put another way, will a compiler ever do >> the wrong thing to us because C99 might have some corner case? Laszlo= , >> what's your take? >=20 > Perhaps Laszlo can confirm or refute my reading of the standard. The concern is justified; the expression "(void **)&any" does not convert a pointer-to-void to pointer-to-GQueue, it causes the bit pattern to be reinterpreted (it is stored as pointer-to-void and reinterpreted as pointer-to-GQueue). 6.2.5 Types p27 says "A pointer to void shall have the same representation and alignment requirements as a pointer to a character type. [...] All pointers to structure types shall have the same representation and alignment requirements as each other." (I'll assume that GQueue is a struct.) But, they need not match each other. At worst, "any" might not even have enough storage for a pointer-to-void. I'll mention that edk2 is chock-full of the above style cast. I think even the UEFI spec is, in code examples. But, edk2 builds with -fno-strict-aliasing & friends. ... I think it doesn't matter in practice, but if we're trying to prevent compilers from outsmarting us, I'd feel better about "any =3D wra= p". Thanks Laszlo >=20 >>>> if (top_ht) { >>>> - if (g_hash_table_size(top_ht)) { >>>> - const char *key; >>>> - g_hash_table_find(top_ht, always_true, &key); >>>> + GHashTableIter iter; >>>> + const char *key; >>>> + >>>> + g_hash_table_iter_init(&iter, top_ht); >>>> + if (g_hash_table_iter_next(&iter, (void **)&key, NULL))= { >>> >>> Is this cast kosher? >> >> Here, in addition to the above argument, we also needed to cast away c= onst.