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

[Markus Armbruster <armbru@redhat.com>]

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);