Re: [Qemu-devel] [PATCH v4 12/28] qapi: Add new visit_complete() function

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> Making each output visitor provide its own output collection
> function was the only remaining reason for exposing visitor
> sub-types to the rest of the code base.  Add a polymorphic
> visit_complete() function which is a no-op for input visitors,
> and which populates an opaque pointer for output visitors.  For
> maximum type-safety, also add a parameter to the output visitor
> constructors with a type-correct version of the output pointer,
> and assert that the two uses match.
>
> This approach was considered superior to either passing the
> output parameter only during construction (action at a distance
> during visit_free() feels awkward) or only during visit_complete()
> (defeating type safety makes it easier to use incorrectly).

Reasonable.

> Most callers were function-local, and therefore a mechanical
> conversion; the testsuite was a bit trickier, but the previous
> cleanup patch minimized the churn here.
>
> Generated code is simplified as follows for events:
>
> |@@ -26,7 +26,7 @@ void qapi_event_send_acpi_device_ost(ACP
> |     QDict *qmp;
> |     Error *err = NULL;
> |     QMPEventFuncEmit emit;
> |-    QmpOutputVisitor *qov;
> |+    QObject *obj;
> |     Visitor *v;
> |     q_obj_ACPI_DEVICE_OST_arg param = {
> |         info
> |@@ -39,8 +39,7 @@ void qapi_event_send_acpi_device_ost(ACP
> |
> |     qmp = qmp_event_build_dict("ACPI_DEVICE_OST");
> |
> |-    qov = qmp_output_visitor_new();
> |-    v = qmp_output_get_visitor(qov);
> |+    v = qmp_output_visitor_new(&obj);
> |
> |     visit_start_struct(v, "ACPI_DEVICE_OST", NULL, 0, &err);
> |     if (err) {
> |@@ -55,7 +54,8 @@ void qapi_event_send_acpi_device_ost(ACP
> |         goto out;
> |     }
> |
> |-    qdict_put_obj(qmp, "data", qmp_output_get_qobject(qov));
> |+    visit_complete(v, &obj);
> |+    qdict_put_obj(qmp, "data", obj);
> |     emit(QAPI_EVENT_ACPI_DEVICE_OST, qmp, &err);
>
> and for commands:
>
> | {
> |     Error *err = NULL;
> |-    QmpOutputVisitor *qov = qmp_output_visitor_new();
> |     Visitor *v;
> |
> |-    v = qmp_output_get_visitor(qov);
> |+    v = qmp_output_visitor_new(ret_out);
> |     visit_type_AddfdInfo(v, "unused", &ret_in, &err);
> |-    if (err) {
> |-        goto out;
> |+    if (!err) {
> |+        visit_complete(v, ret_out);
> |     }
> |-    *ret_out = qmp_output_get_qobject(qov);
> |-
> |-out:
> |     error_propagate(errp, err);
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v4: new patch, inspired by review of v3 7/18
> ---
>  include/qapi/visitor.h               | 52 +++++++++++++++++++++---------------
>  include/qapi/visitor-impl.h          |  3 +++
>  scripts/qapi-commands.py             | 10 +++----
>  scripts/qapi-event.py                |  8 +++---
>  include/qapi/qmp-output-visitor.h    | 11 +++++---
>  include/qapi/string-output-visitor.h | 10 ++++---
>  qapi/qapi-visit-core.c               | 10 +++++++
>  block/qapi.c                         |  9 +++----
>  blockdev.c                           |  9 +++----
>  hmp.c                                | 11 ++++----
>  net/net.c                            | 11 ++++----
>  qapi/qmp-output-visitor.c            | 21 ++++++++-------
>  qapi/string-output-visitor.c         | 22 ++++++++-------
>  qemu-img.c                           | 30 ++++++++++-----------
>  qom/object.c                         | 28 +++++++++----------
>  qom/qom-qobject.c                    | 10 +++----
>  replay/replay-input.c                |  6 ++---
>  tests/check-qnull.c                  | 17 ++++++------
>  tests/test-qmp-output-visitor.c      | 11 +++-----
>  tests/test-string-output-visitor.c   |  8 ++----
>  tests/test-visitor-serialization.c   | 22 ++++++++-------
>  util/qemu-sockets.c                  |  6 ++---
>  22 files changed, 167 insertions(+), 158 deletions(-)
>
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index 2ded852..b3bd97c 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -39,21 +39,15 @@
>   *
>   * All of the visitors are created via:
>   *
> - * Type *subtype_visitor_new(parameters...);
> - *
> - * where Type is either directly 'Visitor *', or is a subtype that can
> - * be trivially upcast to Visitor * via another function:
> - *
> - * Visitor *subtype_get_visitor(SubtypeVisitor *);
> + * Visitor *subtype_visitor_new(parameters...);
>   *
>   * A visitor should be used for exactly one top-level visit_type_FOO()
> - * or virtual walk, then passed to visit_free() to clean up resources.
> - * It is okay to free the visitor without completing the visit, if
> - * some other error is detected in the meantime.  Output visitors
> - * provide an additional function, for collecting the final results of
> - * a successful visit: string_output_get_string() and
> - * qmp_output_get_qobject(); this collection function should not be
> - * called if any errors were reported during the visit.
> + * or virtual walk; if that is successful, the caller can optionally
> + * call visit_complete() (most useful for output visits).  Then,

Well, it's mostly useless for any other visits, isn't it?

> + * regardless of success or failure, the user should call visit_free()
> + * to clean up resources.  It is okay to free the visitor without
> + * completing the visit, if some other error is detected in the
> + * meantime.
>   *
>   * All QAPI types have a corresponding function with a signature
>   * roughly compatible with this:
> @@ -123,14 +117,14 @@
>   *  Error *err = NULL;
>   *  Visitor *v;
>   *
> - *  v = ...obtain input visitor...
> + *  v = FOO_visitor_new(...);
>   *  visit_type_Foo(v, NULL, &f, &err);
>   *  if (err) {
>   *      ...handle error...
>   *  } else {
>   *      ...use f...
>   *  }
> - *  ...clean up v...
> + *  visit_free(v);
>   *  qapi_free_Foo(f);
>   * </example>
>   *
> @@ -140,7 +134,7 @@
>   *  Error *err = NULL;
>   *  Visitor *v;
>   *
> - *  v = ...obtain input visitor...
> + *  v = FOO_visitor_new(...);
>   *  visit_type_FooList(v, NULL, &l, &err);
>   *  if (err) {
>   *      ...handle error...
> @@ -149,7 +143,7 @@
>   *          ...use l->value...
>   *      }
>   *  }
> - *  ...clean up v...
> + *  visit_free(v);
>   *  qapi_free_FooList(l);
>   * </example>
>   *
> @@ -159,13 +153,17 @@
>   *  Foo *f = ...obtain populated object...
>   *  Error *err = NULL;
>   *  Visitor *v;
> + *  Type *result;
>   *
> - *  v = ...obtain output visitor...
> + *  v = FOO_visitor_new(..., &result);
>   *  visit_type_Foo(v, NULL, &f, &err);
>   *  if (err) {
>   *      ...handle error...
> + *  } else {
> + *      visit_complete(v, &result);
> + *      ...use result...
>   *  }
> - *  ...clean up v...
> + *  visit_free(v);
>   * </example>
>   *
>   * When visiting a real QAPI struct, this file provides several
> @@ -191,7 +189,7 @@
>   *  Error *err = NULL;
>   *  int value;
>   *
> - *  v = ...obtain visitor...
> + *  v = FOO_visitor_new(...);
>   *  visit_start_struct(v, NULL, NULL, 0, &err);
>   *  if (err) {
>   *      goto out;
> @@ -219,7 +217,7 @@
>   *  visit_end_struct(v, NULL);
>   * out:
>   *  error_propagate(errp, err);
> - *  ...clean up v...
> + *  visit_free(v);
>   * </example>
>   */
>
> @@ -243,6 +241,18 @@ typedef struct GenericAlternate {
>  /*** Visitor cleanup ***/
>
>  /*
> + * Complete the visit, collecting any output.
> + *
> + * May only be called after a successful top-level visit_type_FOO() or
> + * visit_end_ITEM(), and marks the end of the visit.  The @opaque
> + * pointer should match the output parameter passed to the
> + * subtype_visitor_new() used to create an output visitor, or NULL for
> + * any other visitor.  Needed for output visitors, but may also be
> + * called with other visitors.
> + */
> +void visit_complete(Visitor *v, void *opaque);
> +
> +/*
>   * Free @v and any resources it has tied up.
>   *
>   * May be called whether or not the visit has been successfully
[...]
> diff --git a/include/qapi/qmp-output-visitor.h b/include/qapi/qmp-output-visitor.h
> index 29c9a2e..040fdda 100644
> --- a/include/qapi/qmp-output-visitor.h
> +++ b/include/qapi/qmp-output-visitor.h
> @@ -19,9 +19,12 @@
>
>  typedef struct QmpOutputVisitor QmpOutputVisitor;
>
> -QmpOutputVisitor *qmp_output_visitor_new(void);
> -
> -QObject *qmp_output_get_qobject(QmpOutputVisitor *v);
> -Visitor *qmp_output_get_visitor(QmpOutputVisitor *v);
> +/*
> + * Create a new QMP output visitor.
> + *
> + * If everything else succeeds, pass @result to visit_complete() to
> + * collect the result of the visit.
> + */
> +Visitor *qmp_output_visitor_new(QObject **result);
>
>  #endif

One qmp_output_get_visitor() left behind in docs/qapi-code-gen.txt.

> diff --git a/include/qapi/string-output-visitor.h b/include/qapi/string-output-visitor.h
> index 03e377e..a31054e 100644
> --- a/include/qapi/string-output-visitor.h
> +++ b/include/qapi/string-output-visitor.h
> @@ -18,13 +18,15 @@
>  typedef struct StringOutputVisitor StringOutputVisitor;
>
>  /*
> + * Create a new string output visitor.
> + *
> + * If everything else succeeds, pass @result to visit_complete() to
> + * collect the result of the visit.
> + *

Document @human while we're here?

>   * The string output visitor does not implement support for visiting
>   * QAPI structs, alternates, null, or arbitrary QTypes.  It also
>   * requires a non-null list argument to visit_start_list().
>   */
> -StringOutputVisitor *string_output_visitor_new(bool human);
> -
> -char *string_output_get_string(StringOutputVisitor *v);
> -Visitor *string_output_get_visitor(StringOutputVisitor *v);
> +Visitor *string_output_visitor_new(bool human, char **result);
>
>  #endif
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 5f68c25..279ea8e 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -20,6 +20,16 @@
>  #include "qapi/visitor.h"
>  #include "qapi/visitor-impl.h"
>
> +void visit_complete(Visitor *v, void *opaque)
> +{
> +    if (v->type == VISITOR_OUTPUT) {
> +        assert(v->complete);
> +    }

       assert(v->type != VISITOR_OUTPUT || v->complete);

> +    if (v->complete) {
> +        v->complete(v, opaque);
> +    }
> +}
> +
>  void visit_free(Visitor *v)
>  {
>      if (v) {
[...]
> diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
> index 80bc682..2dac302 100644
> --- a/qapi/qmp-output-visitor.c
> +++ b/qapi/qmp-output-visitor.c
> @@ -32,6 +32,7 @@ struct QmpOutputVisitor
>      Visitor visitor;
>      QStack stack; /* Stack of containers that haven't yet been finished */
>      QObject *root; /* Root of the output visit */
> +    QObject **result; /* User's storage location for result */
>  };
>
>  #define qmp_output_add(qov, name, value) \
> @@ -195,18 +196,17 @@ static void qmp_output_type_null(Visitor *v, const char *name, Error **errp)
>  /* Finish building, and return the root object.
>   * The root object is never null. The caller becomes the object's
>   * owner, and should use qobject_decref() when done with it.  */
> -QObject *qmp_output_get_qobject(QmpOutputVisitor *qov)
> +static void qmp_output_complete(Visitor *v, void *opaque)
>  {
> +    QmpOutputVisitor *qov = to_qov(v);
> +
>      /* A visit must have occurred, with each start paired with end.  */
>      assert(qov->root && QTAILQ_EMPTY(&qov->stack));
> +    assert(opaque == qov->result);
>
>      qobject_incref(qov->root);
> -    return qov->root;
> -}
> -
> -Visitor *qmp_output_get_visitor(QmpOutputVisitor *v)
> -{
> -    return &v->visitor;
> +    *qov->result = qov->root;
> +    qov->result = NULL;

Why zap qov->result?

If we want to do that, we better spell out that visit_complete() may be
called at most once.

I like idempotent functions... but see string_output_complete() below.

>  }
>
>  static void qmp_output_free(Visitor *v)
> @@ -223,7 +223,7 @@ static void qmp_output_free(Visitor *v)
>      g_free(qov);
>  }
>
> -QmpOutputVisitor *qmp_output_visitor_new(void)
> +Visitor *qmp_output_visitor_new(QObject **result)
>  {
>      QmpOutputVisitor *v;
>
> @@ -242,9 +242,12 @@ QmpOutputVisitor *qmp_output_visitor_new(void)
>      v->visitor.type_number = qmp_output_type_number;
>      v->visitor.type_any = qmp_output_type_any;
>      v->visitor.type_null = qmp_output_type_null;
> +    v->visitor.complete = qmp_output_complete;
>      v->visitor.free = qmp_output_free;
>
>      QTAILQ_INIT(&v->stack);
> +    *result = NULL;
> +    v->result = result;
>
> -    return v;
> +    return &v->visitor;
>  }
> diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> index 91ee694..c1c5942 100644
> --- a/qapi/string-output-visitor.c
> +++ b/qapi/string-output-visitor.c
> @@ -58,6 +58,7 @@ struct StringOutputVisitor
>      Visitor visitor;
>      bool human;
>      GString *string;
> +    char **result;
>      ListMode list_mode;
>      union {
>          int64_t s;
> @@ -302,16 +303,13 @@ static void end_list(Visitor *v, void **obj)
>      sov->list_mode = LM_NONE;
>  }
>
> -char *string_output_get_string(StringOutputVisitor *sov)
> +static void string_output_complete(Visitor *v, void *opaque)
>  {
> -    char *string = g_string_free(sov->string, false);
> +    StringOutputVisitor *sov = to_sov(v);
> +
> +    assert(opaque == sov->result);
> +    *sov->result = g_string_free(sov->string, false);
>      sov->string = NULL;

Why zap sov->string?

I guess it lets you transfer ownership of the string inside the GString
without copying it.  An idempotent string_output_complete() would have
to copy.  Okay, I can buy that argument for making it non-idempotent.

> -    return string;
> -}
> -
> -Visitor *string_output_get_visitor(StringOutputVisitor *sov)
> -{
> -    return &sov->visitor;
>  }
>
>  static void free_range(void *range, void *dummy)
> @@ -332,7 +330,7 @@ static void string_output_free(Visitor *v)
>      g_free(sov);
>  }
>
> -StringOutputVisitor *string_output_visitor_new(bool human)
> +Visitor *string_output_visitor_new(bool human, char **result)
>  {
>      StringOutputVisitor *v;
>
> @@ -340,6 +338,9 @@ StringOutputVisitor *string_output_visitor_new(bool human)
>
>      v->string = g_string_new(NULL);
>      v->human = human;
> +    v->result = result;
> +    *result = NULL;
> +
>      v->visitor.type = VISITOR_OUTPUT;
>      v->visitor.type_int64 = print_type_int64;
>      v->visitor.type_uint64 = print_type_uint64;
> @@ -350,7 +351,8 @@ StringOutputVisitor *string_output_visitor_new(bool human)
>      v->visitor.start_list = start_list;
>      v->visitor.next_list = next_list;
>      v->visitor.end_list = end_list;
> +    v->visitor.complete = string_output_complete;
>      v->visitor.free = string_output_free;
>
> -    return v;
> +    return &v->visitor;
>  }
[...]

I rather like how PATCH 05-12 reduce boilerplate in visitor usage.