Re: [Qemu-devel] [PATCH v8 23/35] qmp: Tighten output visitor rules

[Marc-André Lureau]

Hi

On Mon, Dec 21, 2015 at 6:08 PM, Eric Blake <address@hidden> wrote:
> 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), 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), and that it may only be called once per visit.
>
> To keep the semantics of test_visitor_out_empty, we now have to
> explicitly request a top-level visit of a NULL object, by
> implementing the just-added visitor type_null() callback.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> 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 +
>  include/qapi/visitor-impl.h       |  2 +-
>  qapi/qmp-output-visitor.c         | 37 ++++++++++++++++++++++++-------------
>  tests/test-qmp-output-visitor.c   |  2 ++
>  4 files changed, 28 insertions(+), 14 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/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index d301901..ed2934b 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -76,7 +76,7 @@ struct Visitor
>      void (*type_any)(Visitor *v, const char *name, QObject **obj,
>                       Error **errp);
>      /* Must be provided to visit explicit null values (right now, only the
> -     * dealloc visitor supports this).  */
> +     * dealloc and qmp-output visitors support this).  */
>      void (*type_null)(Visitor *v, const char *name, Error **errp);
>
>      /* May be NULL; most useful for input visitors. */
> diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
> index df22999..f2c39c5 100644
> --- a/qapi/qmp-output-visitor.c
> +++ b/qapi/qmp-output-visitor.c
> @@ -1,6 +1,7 @@
>  /*
>   * Core Definitions for QAPI/QMP Command Registry
>   *
> + * Copyright (C) 2015 Red Hat, Inc.
>   * Copyright IBM, Corp. 2011
>   *
>   * Authors:
> @@ -88,9 +89,8 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, const 
> char *name,
>      cur = qmp_output_last(qov);
>
>      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)) {
> @@ -202,18 +202,22 @@ static void qmp_output_type_any(Visitor *v, const char 
> *name, QObject **obj,
>      qmp_output_add_obj(qov, name, *obj);
>  }
>
> +static void qmp_output_type_null(Visitor *v, const char *name, Error **errp)
> +{
> +    QmpOutputVisitor *qov = to_qov(v);
> +    qmp_output_add_obj(qov, name, qnull());
> +}
> +
>  /* Finish building, and return the root object. Will not be NULL. */
>  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;
> +    QObject *root;
> +
> +    assert(qov->root);              /* A visit must have occurred...  */
> +    assert(!qmp_output_last(qov));  /* ...with each start paired with end.  
> */
> +    root = qov->root;
> +    qov->root = NULL;
> +    return root;
>  }
>
>  Visitor *qmp_output_get_visitor(QmpOutputVisitor *v)
> @@ -221,7 +225,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 +235,12 @@ void qmp_output_visitor_cleanup(QmpOutputVisitor *v)
>      }
>
>      qobject_decref(v->root);
> +    v->root = NULL;
> +}
> +
> +void qmp_output_visitor_cleanup(QmpOutputVisitor *v)

It would make sense to call it _free() imho..

> +{
> +    qmp_output_visitor_reset(v);
>      g_free(v);
>  }
>
> @@ -252,6 +262,7 @@ QmpOutputVisitor *qmp_output_visitor_new(void)
>      v->visitor.type_str = qmp_output_type_str;
>      v->visitor.type_number = qmp_output_type_number;
>      v->visitor.type_any = qmp_output_type_any;
> +    v->visitor.type_null = qmp_output_type_null;
>
>      QTAILQ_INIT(&v->stack);
>
> diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
> index 26dc752..74d0ac4 100644
> --- a/tests/test-qmp-output-visitor.c
> +++ b/tests/test-qmp-output-visitor.c
> @@ -260,6 +260,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);
>      }
>  }
>
> @@ -459,6 +460,7 @@ static void test_visitor_out_empty(TestOutputVisitorData 
> *data,
>  {
>      QObject *arg;
>
> +    visit_type_null(data->ov, NULL, &error_abort);

I guess this is going to be used outside of just this artificial case.
Otherwise, I would have suggested to get rid of it.

>      arg = qmp_output_get_qobject(data->qov);
>      g_assert(qobject_type(arg) == QTYPE_QNULL);
>      /* Check that qnull reference counting is sane */
> --
> 2.4.3
>
>

Reviewed-by: Marc-André Lureau <address@hidden>



-- 
Marc-André Lureau