Eric Blake <ebl...@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 <ebl...@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()?
