Re: [Qemu-devel] [PATCH v14 13/19] qmp: Tighten output visitor rules

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> Add a new qmp_output_visitor_reset(), which must be called before
> reusing an exising QmpOutputVisitor on a new root object.  Tighten
> assertions to require that qmp_output_get_qobject() can only be
> called after pairing a visit_end_* for every visit_start_* (rather
> than allowing it to return a partially built object), and that it
> must not be called unless at least one visit_type_* or visit_start/
> visit_end pair has occurred since creation/reset (the accidental
> return of NULL fixed by commit ab8bf1d7 would have been much
> easier to diagnose).
>
> Also, check that we are encountering the expected object or list
> type (both during visit_end*, and also by validating whether 'name'
> was NULL - although the latter may need to change later if we
> improve error messages by always passing in a sensible name).
> This provides protection against mismatched push(struct)/pop(list)
> or push(list)/pop(struct), similar to the qmp-input protection
> added in commit bdd8e6b5.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>

As written, the commit message makes me wonder why we add
qmp_output_visitor_reset() in the same commit.  I think the reason is
the tightened rules make it necessary.  The commit message could make
that clearer by explaining the rule changes first, then point out we
need a reset to comply with the rules.

>
> ---
> v14: no change
> v13: no change
> v12: rebase to latest, move type_null() into earlier patches,
> don't change signature of pop, don't auto-reset after a single
> get_qobject
> [no v10, v11]
> v9: rebase to added patch, squash in more sanity checks, drop
> Marc-Andre's R-b
> v8: rename qmp_output_reset to qmp_output_visitor_reset
> v7: new patch, based on discussion about spapr_drc.c
> ---
>  include/qapi/qmp-output-visitor.h |  1 +
>  qapi/qmp-output-visitor.c         | 39 +++++++++++++++++++++++----------------
>  tests/test-qmp-output-visitor.c   |  6 ++++++
>  3 files changed, 30 insertions(+), 16 deletions(-)
>
> diff --git a/include/qapi/qmp-output-visitor.h b/include/qapi/qmp-output-visitor.h
> index 2266770..5093f0d 100644
> --- a/include/qapi/qmp-output-visitor.h
> +++ b/include/qapi/qmp-output-visitor.h
> @@ -21,6 +21,7 @@ typedef struct QmpOutputVisitor QmpOutputVisitor;
>
>  QmpOutputVisitor *qmp_output_visitor_new(void);
>  void qmp_output_visitor_cleanup(QmpOutputVisitor *v);
> +void qmp_output_visitor_reset(QmpOutputVisitor *v);
>
>  QObject *qmp_output_get_qobject(QmpOutputVisitor *v);
>  Visitor *qmp_output_get_visitor(QmpOutputVisitor *v);
> diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
> index 5681ad3..7c48dfb 100644
> --- a/qapi/qmp-output-visitor.c
> +++ b/qapi/qmp-output-visitor.c
> @@ -82,9 +82,8 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name,
>      QObject *cur = e ? e->value : NULL;
>
>      if (!cur) {
> -        /* FIXME we should require the user to reset the visitor, rather
> -         * than throwing away the previous root */
> -        qobject_decref(qov->root);
> +        /* Don't allow reuse of visitor on more than one root */
> +        assert(!qov->root);
>          qov->root = value;
>      } else {
>          switch (qobject_type(cur)) {
> @@ -93,6 +92,9 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name,
>              qdict_put_obj(qobject_to_qdict(cur), name, value);
>              break;
>          case QTYPE_QLIST:
> +            /* FIXME: assertion needs adjustment if we fix visit-core
> +             * to pass "name.0" style name during lists.  */

visit-core merely passes through whatever name it gets from the client.
Thus, saying we 'fix visit-core to pass "name.0"' is a bit misleading.
What we'd do is change it to require "name.0", then update its users to
comply.

Moreover, this is a note, not a FIXME: nothing is broken here.  The
closest we get to "broken" are the bad error messages, but they're
elsewhere.

I'd simply drop the comment.

> +            assert(!name);

PATCH 08 made this part of the contract.  It also added a bunch of
contract-checking assertions.  Should this one be in PATCH 08, too?

>              qlist_append_obj(qobject_to_qlist(cur), value);
>              break;
>          default:
> @@ -114,7 +116,8 @@ static void qmp_output_start_struct(Visitor *v, const char *name, void **obj,
>  static void qmp_output_end_struct(Visitor *v, Error **errp)
>  {
>      QmpOutputVisitor *qov = to_qov(v);
> -    qmp_output_pop(qov);
> +    QObject *value = qmp_output_pop(qov);
> +    assert(qobject_type(value) == QTYPE_QDICT);
>  }
>
>  static void qmp_output_start_list(Visitor *v, const char *name, Error **errp)
> @@ -145,7 +148,8 @@ static GenericList *qmp_output_next_list(Visitor *v, GenericList **listp,
>  static void qmp_output_end_list(Visitor *v)
>  {
>      QmpOutputVisitor *qov = to_qov(v);
> -    qmp_output_pop(qov);
> +    QObject *value = qmp_output_pop(qov);
> +    assert(qobject_type(value) == QTYPE_QLIST);
>  }
>
>  static void qmp_output_type_int64(Visitor *v, const char *name, int64_t *obj,
> @@ -202,18 +206,15 @@ static void qmp_output_type_null(Visitor *v, const char *name, Error **errp)
>      qmp_output_add_obj(qov, name, qnull());
>  }
>
> -/* Finish building, and return the root object. Will not be NULL. */
> +/* Finish building, and return the root object. Will not be NULL.
> + * Caller must use qobject_decref() on the result.  */

Well, it must only if it wants the object freed, but I know what you
mean.

Perhaps:

   /*
    * Finish building, and return the root object.
    * The root object is never null.
    * The caller becomes the object's owner.  It should free it with
    * qobject_decref().
    */
  
>  QObject *qmp_output_get_qobject(QmpOutputVisitor *qov)
>  {
> -    /* FIXME: we should require that a visit occurred, and that it is
> -     * complete (no starts without a matching end) */
> -    QObject *obj = qov->root;
> -    if (obj) {
> -        qobject_incref(obj);
> -    } else {
> -        obj = qnull();
> -    }
> -    return obj;
> +    /* A visit must have occurred, with each start paired with end.  */
> +    assert(qov->root && !QTAILQ_FIRST(&qov->stack));

&& QTAILQ_EMPTY(&qov-stack) would be clearer, wouldn't it?

> +
> +    qobject_incref(qov->root);
> +    return qov->root;
>  }
>
>  Visitor *qmp_output_get_visitor(QmpOutputVisitor *v)
> @@ -221,7 +222,7 @@ Visitor *qmp_output_get_visitor(QmpOutputVisitor *v)
>      return &v->visitor;
>  }
>
> -void qmp_output_visitor_cleanup(QmpOutputVisitor *v)
> +void qmp_output_visitor_reset(QmpOutputVisitor *v)
>  {
>      QStackEntry *e, *tmp;
>
> @@ -231,6 +232,12 @@ void qmp_output_visitor_cleanup(QmpOutputVisitor *v)
>      }
>
>      qobject_decref(v->root);
> +    v->root = NULL;
> +}
> +
> +void qmp_output_visitor_cleanup(QmpOutputVisitor *v)
> +{
> +    qmp_output_visitor_reset(v);
>      g_free(v);
>  }
>
> diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
> index e656d99..7be33ba 100644
> --- a/tests/test-qmp-output-visitor.c
> +++ b/tests/test-qmp-output-visitor.c
> @@ -139,6 +139,7 @@ static void test_visitor_out_enum(TestOutputVisitorData *data,
>          g_assert_cmpstr(qstring_get_str(qobject_to_qstring(obj)), ==,
>                          EnumOne_lookup[i]);
>          qobject_decref(obj);
> +        qmp_output_visitor_reset(data->qov);
>      }
>  }
>
> @@ -153,6 +154,7 @@ static void test_visitor_out_enum_errors(TestOutputVisitorData *data,
>          visit_type_EnumOne(data->ov, "unused", &bad_values[i], &err);
>          g_assert(err);
>          error_free(err);
> +        qmp_output_visitor_reset(data->qov);
>      }
>  }
>
> @@ -262,6 +264,7 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
>          visit_type_UserDefOne(data->ov, "unused", &pu, &err);
>          g_assert(err);
>          error_free(err);
> +        qmp_output_visitor_reset(data->qov);
>      }
>  }
>
> @@ -366,6 +369,7 @@ static void test_visitor_out_any(TestOutputVisitorData *data,
>      qobject_decref(obj);
>      qobject_decref(qobj);
>
> +    qmp_output_visitor_reset(data->qov);
>      qdict = qdict_new();
>      qdict_put(qdict, "integer", qint_from_int(-42));
>      qdict_put(qdict, "boolean", qbool_from_bool(true));
> @@ -442,6 +446,7 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
>      qapi_free_UserDefAlternate(tmp);
>      qobject_decref(arg);
>
> +    qmp_output_visitor_reset(data->qov);
>      tmp = g_new0(UserDefAlternate, 1);
>      tmp->type = QTYPE_QSTRING;
>      tmp->u.s = g_strdup("hello");
> @@ -455,6 +460,7 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
>      qapi_free_UserDefAlternate(tmp);
>      qobject_decref(arg);
>
> +    qmp_output_visitor_reset(data->qov);
>      tmp = g_new0(UserDefAlternate, 1);
>      tmp->type = QTYPE_QDICT;
>      tmp->u.udfu.integer = 1;

How did you find the places that now need qmp_output_visitor_reset()?