Re: [Qemu-devel] [PATCH v9 26/37] qapi: Simplify excess input reporting in input visitors

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 01/22/2016 12:24 PM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> 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 <address@hidden>
>> 
>> Whoops, it's even mine!  I forgot... %-)
>> 
>>> Signed-off-by: Eric Blake <address@hidden>
>>> Reviewed-by: Marc-André Lureau <address@hidden>
>>>
>>> ---
>
>>>      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 = 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, and
> 'any' qualifies as such a pointer.  We are then taking the address of
> that (or void**) to pass by reference.

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.

(void **)iter converts iter, not *iter.  *iter gets reinterpreted on
dereference.

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.

> I suppose a stricter version would be:
>
> void *wrap;
> GQueue *any;
> if (g_hash_table_iter_next(&iter, NULL, &wrap)) {
>     any = 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?

Perhaps Laszlo can confirm or refute my reading of the standard.

>>>          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 const.