| Message ID | 1453219845-30939-27-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
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> > > --- > v9: no change > v8: rebase to earlier changes > v7: retitle, rebase to earlier context changes > v6: new patch, based on comments on RFC against v5 7/46 > --- > qapi/opts-visitor.c | 12 +++--------- > qapi/qmp-input-visitor.c | 14 +++++--------- > 2 files changed, 8 insertions(+), 18 deletions(-) > > diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c > index 62ffdd4..df312e6 100644 > --- a/qapi/opts-visitor.c > +++ b/qapi/opts-visitor.c > @@ -156,17 +156,11 @@ opts_start_struct(Visitor *v, const char *name, void **obj, > } > > > -static gboolean > -ghr_true(gpointer ign_key, gpointer ign_value, gpointer ign_user_data) > -{ > - return TRUE; > -} > - > - > static void > opts_end_struct(Visitor *v, Error **errp) > { > OptsVisitor *ov = to_ov(v); > + GHashTableIter iter; > 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? > const QemuOpt *first; > > first = g_queue_peek_head(any); > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c > index ad23bec..91ef945 100644 > --- a/qapi/qmp-input-visitor.c > +++ b/qapi/qmp-input-visitor.c > @@ -88,12 +88,6 @@ static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj, Error **errp) > qiv->nb_stack++; > } > > -/** Only for qmp_input_pop. */ > -static gboolean always_true(gpointer key, gpointer val, gpointer user_pkey) > -{ > - *(const char **)user_pkey = (const char *)key; > - return TRUE; > -} > > static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp) > { > @@ -102,9 +96,11 @@ static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp) > if (qiv->strict) { > GHashTable * const top_ht = qiv->stack[qiv->nb_stack - 1].h; > 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? > error_setg(errp, QERR_QMP_EXTRA_MEMBER, key); > } > g_hash_table_unref(top_ht);
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> writes: > 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. 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.
On 01/25/16 10:27, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> 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... %-) 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 <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. > > 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. 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 = wrap". Thanks Laszlo > >>>> 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.
I got stuck reviewing "[PATCH v9 31/37] qapi-visit: Unify struct and union visit". The result is probably fine, but the patch itself is impenetrable for me. Here's my version. I believe it's really simple to review (but as the author, I'm hopelessly biased). I based on PATCH 26/37 instead of PATCH 30, because I found the empty struct stuff from PACTH 27-30 not useful for this unification job. If we want it for other reasons (I have no opinion, yet), rebasing it onto this series shouldn't be hard. Markus Armbruster (3): qapi-visit: Simplify how we visit common union members qapi-visit: Clean up code generated around visit_end_union() qapi-visit: Unify struct and union visit scripts/qapi-visit.py | 122 +++++++++++++++----------------------------------- 1 file changed, 36 insertions(+), 86 deletions(-)
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c index 62ffdd4..df312e6 100644 --- a/qapi/opts-visitor.c +++ b/qapi/opts-visitor.c @@ -156,17 +156,11 @@ opts_start_struct(Visitor *v, const char *name, void **obj, } -static gboolean -ghr_true(gpointer ign_key, gpointer ign_value, gpointer ign_user_data) -{ - return TRUE; -} - - static void opts_end_struct(Visitor *v, Error **errp) { OptsVisitor *ov = to_ov(v); + GHashTableIter iter; 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)) { const QemuOpt *first; first = g_queue_peek_head(any); diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c index ad23bec..91ef945 100644 --- a/qapi/qmp-input-visitor.c +++ b/qapi/qmp-input-visitor.c @@ -88,12 +88,6 @@ static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj, Error **errp) qiv->nb_stack++; } -/** Only for qmp_input_pop. */ -static gboolean always_true(gpointer key, gpointer val, gpointer user_pkey) -{ - *(const char **)user_pkey = (const char *)key; - return TRUE; -} static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp) { @@ -102,9 +96,11 @@ static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp) if (qiv->strict) { GHashTable * const top_ht = qiv->stack[qiv->nb_stack - 1].h; 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)) { error_setg(errp, QERR_QMP_EXTRA_MEMBER, key); } g_hash_table_unref(top_ht);