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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2594 bytes --]

On 01/22/2016 12:24 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> 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 <armbru@redhat.com>
> 
> Whoops, it's even mine!  I forgot... %-)
> 
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> ---

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

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?

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]