[Qemu-devel] [PATCH v7 19/31] qmp: Tighten output visitor rules

[Eric Blake]

Add a new qmp_output_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 (so
that the accidental return of NULL fixed by commit ab8bf1d7 will
be 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>

---
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..184195b 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_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 78a5ab1..628cab7 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -76,7 +76,7 @@ struct Visitor
     void (*type_any)(Visitor *v, QObject **obj, const char *name,
                      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 06ee19b..28f9854 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, QObject 
**obj, const char *name,
     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_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)
+{
+    qmp_output_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 8e6fc33..53d1372 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, &pu, "unused", &err);
         g_assert(err);
         error_free(err);
+        qmp_output_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);
     arg = qmp_output_get_qobject(data->qov);
     g_assert(qobject_type(arg) == QTYPE_QNULL);
     /* Check that qnull reference counting is sane */
-- 
2.4.3