Re: [Qemu-devel] [PATCH v14 14/19] qapi: Split visit_end_struct() into pieces

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> As mentioned in previous patches, we want to call visit_end_struct()
> functions unconditionally, so that visitors can release resources
> tied up since the matching visit_start_struct() without also having
> to worry about error priority if more than one error occurs.
>
> Even though error_propagate() can be safely used to ignore a second
> error during cleanup caused by a first error, it is simpler if the
> cleanup cannot set an error.  So, split out the error checking
> portion (basically, input visitors checking for unvisited keys) into
> a new function visit_check_struct(), which can be safely skipped if
> any earlier errors are encountered, and leave the cleanup portion
> (which never fails, but must be called unconditionally if
> visit_start_struct() succeeded) in visit_end_struct().
>
> Generated code has diffs resembling:
>
> |@@ -59,10 +59,12 @@ void visit_type_ACPIOSTInfo(Visitor *v,
> |         goto out_obj;
> |     }
> |     visit_type_ACPIOSTInfo_members(v, obj, &err);
> |-    error_propagate(errp, err);
> |-    err = NULL;
> |+    if (err) {
> |+        goto out_obj;
> |+    }
> |+    visit_check_struct(v, &err);
> | out_obj:
> |-    visit_end_struct(v, &err);
> |+    visit_end_struct(v);
> | out:
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v14: rebase to master
> v13: rebase to earlier changes
> v12: rebase to earlier changes, fix bug in spapr_drc not calling
> visit_end_struct, fold in docs, fix stray DO_UPCAST from sneaking
> back in
> [no v10, v11]
> v9: rebase to earlier changes, drop Marc-Andre's R-b
> v8: rebase to 'name' motion
> v7: rebase to earlier changes
> v6: new patch, revised version of RFC based on discussion of v5 7/46
> ---
>  include/qapi/visitor.h         | 20 +++++++++++++++-----
>  include/qapi/visitor-impl.h    |  5 ++++-
>  scripts/qapi-event.py          |  5 ++++-
>  scripts/qapi-visit.py          | 15 +++++++++------
>  qapi/qapi-visit-core.c         | 11 +++++++++--
>  block/crypto.c                 | 14 ++++++++------
>  hw/ppc/spapr_drc.c             |  3 ++-
>  hw/virtio/virtio-balloon.c     | 15 ++++++++-------
>  qapi/opts-visitor.c            | 17 +++++++++++++++--
>  qapi/qapi-dealloc-visitor.c    |  2 +-
>  qapi/qmp-input-visitor.c       | 34 +++++++++++++++++++---------------
>  qapi/qmp-output-visitor.c      |  2 +-
>  qom/object.c                   |  5 ++---
>  qom/object_interfaces.c        | 16 +++++++---------
>  tests/test-qmp-input-visitor.c |  3 ++-
>  docs/qapi-code-gen.txt         |  8 +++++---
>  16 files changed, 111 insertions(+), 64 deletions(-)
>
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index 735b389..4c29722 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -182,10 +182,11 @@
>   *  }
>   * outlist:
>   *  visit_end_list(v);
> + *  if (!err) {
> + *      visit_check_struct(v, &err);
> + *  }
>   * outobj:
> - *  error_propagate(errp, err);
> - *  err = NULL;
> - *  visit_end_struct(v, &err);
> + *  visit_end_struct(v);
>   *  error_propagate(errp, err);
>   *  ...clean up v...
>   * </example>
> @@ -238,17 +239,26 @@ void visit_start_struct(Visitor *v, const char *name, void **obj,
>                          size_t size, Error **errp);
>
>  /*
> - * Complete an object visit started earlier.
> + * Prepare for completing an object visit.
>   *
>   * @errp must be NULL-initialized, and is set if an error is detected
>   * (such as unparsed keys remaining in the input stream).
>   *
> + * Should be called prior to visit_end_struct() if all other
> + * intermediate visit steps were successful, to allow the visitor one
> + * last chance to report errors.
> + */
> +void visit_check_struct(Visitor *v, Error **errp);
> +
> +/*
> + * Complete an object visit started earlier.
> + *
>   * Must be called after any successful use of visit_start_struct(),
>   * even if intermediate processing was skipped due to errors, to allow
>   * the backend to release any resources.  Destroying the visitor may
>   * behave as if this was implicitly called.
>   */
> -void visit_end_struct(Visitor *v, Error **errp);
> +void visit_end_struct(Visitor *v);
>
>
>  /* === Visiting lists */
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index 90bcaec..d44fcd1 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -39,8 +39,11 @@ struct Visitor
>      void (*start_struct)(Visitor *v, const char *name, void **obj,
>                           size_t size, Error **errp);
>
> +    /* Optional; intended for input visitors.  */
> +    void (*check_struct)(Visitor *v, Error **errp);
> +
>      /* Must be set to visit structs.  */
> -    void (*end_struct)(Visitor *v, Error **errp);
> +    void (*end_struct)(Visitor *v);
>
>      /* Must be set.  */
>      void (*start_list)(Visitor *v, const char *name, Error **errp);
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index 9b5c5b5..21fb167 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -98,7 +98,10 @@ def gen_event_send(name, arg_type):
>          goto out;
>      }
>      visit_type_%(c_name)s_members(v, &param, &err);
> -    visit_end_struct(v, err ? NULL : &err);
> +    if (!err) {
> +        visit_check_struct(v, &err);
> +    }
> +    visit_end_struct(v);
>      if (err) {
>          goto out;
>      }
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index dc8b39c..bfe4a09 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -186,9 +186,10 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
>              break;
>          }
>          visit_type_%(c_type)s_members(v, &(*obj)->u.%(c_name)s, &err);
> -        error_propagate(errp, err);
> -        err = NULL;
> -        visit_end_struct(v, &err);
> +        if (!err) {
> +            visit_check_struct(v, &err);
> +        }
> +        visit_end_struct(v);
>  ''',
>                           c_type=var.type.c_name(),
>                           c_name=c_name(var.name))
> @@ -236,10 +237,12 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
>          goto out_obj;
>      }
>      visit_type_%(c_name)s_members(v, *obj, &err);
> -    error_propagate(errp, err);
> -    err = NULL;
> +    if (err) {
> +        goto out_obj;
> +    }
> +    visit_check_struct(v, &err);
>  out_obj:
> -    visit_end_struct(v, &err);
> +    visit_end_struct(v);
>  out:
>      error_propagate(errp, err);
>  }
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 064b9f1..76ea690 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -30,9 +30,16 @@ void visit_start_struct(Visitor *v, const char *name, void **obj,
>      v->start_struct(v, name, obj, size, errp);
>  }
>
> -void visit_end_struct(Visitor *v, Error **errp)
> +void visit_check_struct(Visitor *v, Error **errp)
>  {
> -    v->end_struct(v, errp);
> +    if (v->check_struct) {
> +        v->check_struct(v, errp);
> +    }
> +}
> +
> +void visit_end_struct(Visitor *v)
> +{
> +    v->end_struct(v);
>  }
>
>  void visit_start_list(Visitor *v, const char *name, Error **errp)
> diff --git a/block/crypto.c b/block/crypto.c
> index 1903e84..2424a4c 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -196,7 +196,6 @@ block_crypto_open_opts_init(QCryptoBlockFormat format,
>      OptsVisitor *ov;
>      QCryptoBlockOpenOptions *ret = NULL;
>      Error *local_err = NULL;
> -    Error *end_err = NULL;
>
>      ret = g_new0(QCryptoBlockOpenOptions, 1);
>      ret->format = format;
> @@ -219,9 +218,11 @@ block_crypto_open_opts_init(QCryptoBlockFormat format,
>          error_setg(&local_err, "Unsupported block format %d", format);
>          break;
>      }
> +    if (!local_err) {
> +        visit_check_struct(opts_get_visitor(ov), &local_err);
> +    }
>
> -    visit_end_struct(opts_get_visitor(ov), &end_err);
> -    error_propagate(&local_err, end_err);
> +    visit_end_struct(opts_get_visitor(ov));
>
>   out:
>      if (local_err) {
> @@ -242,7 +243,6 @@ block_crypto_create_opts_init(QCryptoBlockFormat format,
>      OptsVisitor *ov;
>      QCryptoBlockCreateOptions *ret = NULL;
>      Error *local_err = NULL;
> -    Error *end_err = NULL;
>
>      ret = g_new0(QCryptoBlockCreateOptions, 1);
>      ret->format = format;
> @@ -265,9 +265,11 @@ block_crypto_create_opts_init(QCryptoBlockFormat format,
>          error_setg(&local_err, "Unsupported block format %d", format);
>          break;
>      }
> +    if (!local_err) {
> +        visit_check_struct(opts_get_visitor(ov), &local_err);
> +    }
>
> -    visit_end_struct(opts_get_visitor(ov), &end_err);
> -    error_propagate(&local_err, end_err);
> +    visit_end_struct(opts_get_visitor(ov));
>
>   out:
>      if (local_err) {
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 72d725c..5395c02 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -297,7 +297,8 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
>          case FDT_END_NODE:
>              /* shouldn't ever see an FDT_END_NODE before FDT_BEGIN_NODE */
>              g_assert(fdt_depth > 0);
> -            visit_end_struct(v, &err);
> +            visit_check_struct(v, &err);
> +            visit_end_struct(v);
>              if (err) {
>                  error_propagate(errp, err);
>                  return;
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index c74101e..47f5f5e 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -137,17 +137,18 @@ static void balloon_stats_get_all(Object *obj, Visitor *v, const char *name,
>      for (i = 0; i < VIRTIO_BALLOON_S_NR; i++) {
>          visit_type_uint64(v, balloon_stat_names[i], &s->stats[i], &err);
>          if (err) {
> -            break;
> +            goto out_nested;
>          }
>      }
> -    error_propagate(errp, err);
> -    err = NULL;
> -    visit_end_struct(v, &err);
> +    visit_check_struct(v, &err);
> +out_nested:
> +    visit_end_struct(v);
>
> +    if (!err) {
> +        visit_check_struct(v, &err);
> +    }
>  out_end:
> -    error_propagate(errp, err);
> -    err = NULL;
> -    visit_end_struct(v, &err);
> +    visit_end_struct(v);
>  out:
>      error_propagate(errp, err);
>  }
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index cdb6e42..a08d5a7 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -159,13 +159,13 @@ opts_start_struct(Visitor *v, const char *name, void **obj,
>
>
>  static void
> -opts_end_struct(Visitor *v, Error **errp)
> +opts_check_struct(Visitor *v, Error **errp)
>  {
>      OptsVisitor *ov = to_ov(v);
>      GHashTableIter iter;
>      GQueue *any;
>
> -    if (--ov->depth > 0) {
> +    if (ov->depth > 0) {
>          return;
>      }
>
> @@ -177,6 +177,18 @@ opts_end_struct(Visitor *v, Error **errp)
>          first = g_queue_peek_head(any);
>          error_setg(errp, QERR_INVALID_PARAMETER, first->name);
>      }
> +}
> +
> +
> +static void
> +opts_end_struct(Visitor *v)
> +{
> +    OptsVisitor *ov = to_ov(v);
> +
> +    if (--ov->depth > 0) {
> +        return;
> +    }
> +
>      g_hash_table_destroy(ov->unprocessed_opts);
>      ov->unprocessed_opts = NULL;
>      if (ov->fake_id_opt) {
> @@ -511,6 +523,7 @@ opts_visitor_new(const QemuOpts *opts)
>      ov->visitor.type = VISITOR_INPUT;
>
>      ov->visitor.start_struct = &opts_start_struct;
> +    ov->visitor.check_struct = &opts_check_struct;
>      ov->visitor.end_struct   = &opts_end_struct;
>
>      ov->visitor.start_list = &opts_start_list;
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index 413d525..9005bad 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -67,7 +67,7 @@ static void qapi_dealloc_start_struct(Visitor *v, const char *name, void **obj,
>      qapi_dealloc_push(qov, obj);
>  }
>
> -static void qapi_dealloc_end_struct(Visitor *v, Error **errp)
> +static void qapi_dealloc_end_struct(Visitor *v)
>  {
>      QapiDeallocVisitor *qov = to_qov(v);
>      void **obj = qapi_dealloc_pop(qov);
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index e5b412a..a94cfa9 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -124,8 +124,10 @@ static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj,
>  }
>
>
> -static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
> +static void qmp_input_check_struct(Visitor *v, Error **errp)
>  {
> +    QmpInputVisitor *qiv = to_qiv(v);
> +
>      assert(qiv->nb_stack > 0);
>
>      if (qiv->strict) {
> @@ -138,6 +140,19 @@ static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
>              if (g_hash_table_iter_next(&iter, (void **)&key, NULL)) {
>                  error_setg(errp, QERR_QMP_EXTRA_MEMBER, key);
>              }
> +        }
> +    }
> +}
> +
> +static void qmp_input_pop(Visitor *v)
> +{
> +    QmpInputVisitor *qiv = to_qiv(v);
> +
> +    assert(qiv->nb_stack > 0);
> +
> +    if (qiv->strict) {
> +        GHashTable * const top_ht = qiv->stack[qiv->nb_stack - 1].h;
> +        if (top_ht) {
>              g_hash_table_unref(top_ht);
>          }
>      }
> @@ -172,12 +187,6 @@ static void qmp_input_start_struct(Visitor *v, const char *name, void **obj,
>      }
>  }
>
> -static void qmp_input_end_struct(Visitor *v, Error **errp)
> -{
> -    QmpInputVisitor *qiv = to_qiv(v);
> -
> -    qmp_input_pop(qiv, errp);
> -}
>
>  static void qmp_input_start_list(Visitor *v, const char *name, Error **errp)
>  {
> @@ -217,12 +226,6 @@ static GenericList *qmp_input_next_list(Visitor *v, GenericList **list,
>      return entry;
>  }
>
> -static void qmp_input_end_list(Visitor *v)
> -{
> -    QmpInputVisitor *qiv = to_qiv(v);
> -
> -    qmp_input_pop(qiv, &error_abort);
> -}
>
>  static void qmp_input_start_alternate(Visitor *v, const char *name,
>                                        GenericAlternate **obj, size_t size,
> @@ -382,10 +385,11 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
>
>      v->visitor.type = VISITOR_INPUT;
>      v->visitor.start_struct = qmp_input_start_struct;
> -    v->visitor.end_struct = qmp_input_end_struct;
> +    v->visitor.check_struct = qmp_input_check_struct;
> +    v->visitor.end_struct = qmp_input_pop;
>      v->visitor.start_list = qmp_input_start_list;
>      v->visitor.next_list = qmp_input_next_list;
> -    v->visitor.end_list = qmp_input_end_list;
> +    v->visitor.end_list = qmp_input_pop;
>      v->visitor.start_alternate = qmp_input_start_alternate;
>      v->visitor.type_int64 = qmp_input_type_int64;
>      v->visitor.type_uint64 = qmp_input_type_uint64;
> diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
> index 7c48dfb..ecb2005 100644
> --- a/qapi/qmp-output-visitor.c
> +++ b/qapi/qmp-output-visitor.c
> @@ -113,7 +113,7 @@ static void qmp_output_start_struct(Visitor *v, const char *name, void **obj,
>      qmp_output_push(qov, dict);
>  }
>
> -static void qmp_output_end_struct(Visitor *v, Error **errp)
> +static void qmp_output_end_struct(Visitor *v)
>  {
>      QmpOutputVisitor *qov = to_qov(v);
>      QObject *value = qmp_output_pop(qov);
> diff --git a/qom/object.c b/qom/object.c
> index 8e6e68d..3bc8a00 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -2036,10 +2036,9 @@ static void property_get_tm(Object *obj, Visitor *v, const char *name,
>      if (err) {
>          goto out_end;
>      }
> +    visit_check_struct(v, &err);
>  out_end:
> -    error_propagate(errp, err);
> -    err = NULL;
> -    visit_end_struct(v, errp);
> +    visit_end_struct(v);
>  out:
>      error_propagate(errp, err);
>
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index ab5da35..a0bf6b7 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -42,7 +42,7 @@ Object *user_creatable_add(const QDict *qdict,
>      char *type = NULL;
>      char *id = NULL;
>      Object *obj = NULL;
> -    Error *local_err = NULL, *end_err = NULL;
> +    Error *local_err = NULL;
>      QDict *pdict;
>
>      pdict = qdict_clone_shallow(qdict);
> @@ -69,16 +69,14 @@ Object *user_creatable_add(const QDict *qdict,
>          goto out_visit;
>      }
>
> - out_visit:
> -    visit_end_struct(v, &end_err);
> -    if (end_err) {
> -        error_propagate(&local_err, end_err);
> -        if (obj) {
> -            user_creatable_del(id, NULL);
> -        }
> -        goto out;
> +    visit_check_struct(v, &local_err);
> +    if (local_err) {
> +        user_creatable_del(id, NULL);
>      }
>
> +out_visit:
> +    visit_end_struct(v);
> +
>  out:
>      QDECREF(pdict);
>      g_free(id);

I think this could be simplified further:

       visit_check_struct(v, &local_err);
       if (local_err) {
           goto out_visit;
       }

       obj = user_creatable_add_type(type, id, pdict, v, &local_err);

   out_visit:
       visit_end_struct(v);

   out:


> diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
> index d06383a..ac8ebea 100644
> --- a/tests/test-qmp-input-visitor.c
> +++ b/tests/test-qmp-input-visitor.c
> @@ -290,7 +290,8 @@ static void test_visitor_in_null(TestInputVisitorData *data,
>      v = visitor_input_test_init(data, "{ 'a': null }");
>      visit_start_struct(v, NULL, NULL, 0, &error_abort);
>      visit_type_null(v, "a", &error_abort);
> -    visit_end_struct(v, &error_abort);
> +    visit_check_struct(v, &error_abort);
> +    visit_end_struct(v);
>  }
>
>  static void test_visitor_in_union_flat(TestInputVisitorData *data,
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index 0e4baff..3b2422f 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -899,10 +899,12 @@ Example:
>              goto out_obj;
>          }
>          visit_type_UserDefOne_members(v, *obj, &err);
> -        error_propagate(errp, err);
> -        err = NULL;
> +        if (err) {
> +            goto out_obj;
> +        }
> +        visit_check_struct(v, &err);
>      out_obj:
> -        visit_end_struct(v, &err);
> +        visit_end_struct(v);
>      out:
>          error_propagate(errp, err);
>      }