[Qemu-devel] [PATCH v4 05/28] qapi: Add new visit_free() function

[Eric Blake]

Making each visitor provide its own (awkwardly-named) FOO_cleanup()
is unusual, when we can instead have a polymorphic visit_free()
interface.

The dealloc visitor is the first one converted to completely use
the new entry point, since only generated code and the testsuite
were using it.  Diffs to the generated code look like:

| void qapi_free_ACPIOSTInfo(ACPIOSTInfo *obj)
| {
|-    QapiDeallocVisitor *qdv;
|     Visitor *v;
|
|     if (!obj) {
|         return;
|     }
|
|-    qdv = qapi_dealloc_visitor_new();
|-    v = qapi_dealloc_get_visitor(qdv);
|+    v = qapi_dealloc_visitor_new();
|     visit_type_ACPIOSTInfo(v, NULL, &obj, NULL);
|-    qapi_dealloc_visitor_cleanup(qdv);
|+    visit_free(v);
|}

Signed-off-by: Eric Blake <address@hidden>

---
v4: new patch, inspired by review of v3 7/18
---
 include/qapi/visitor.h             | 37 ++++++++++++++++++++++++++++++++++---
 include/qapi/visitor-impl.h        |  3 +++
 scripts/qapi-commands.py           | 16 ++++++----------
 scripts/qapi-event.py              |  2 +-
 scripts/qapi-types.py              |  6 ++----
 include/qapi/dealloc-visitor.h     |  5 +----
 qapi/qapi-visit-core.c             |  7 +++++++
 qapi/opts-visitor.c                |  9 +++++++++
 qapi/qapi-dealloc-visitor.c        | 12 ++++--------
 qapi/qmp-input-visitor.c           |  7 +++++++
 qapi/qmp-output-visitor.c          |  7 +++++++
 qapi/string-input-visitor.c        |  7 +++++++
 qapi/string-output-visitor.c       |  7 +++++++
 tests/test-visitor-serialization.c |  6 +++---
 docs/qapi-code-gen.txt             | 28 ++++++++++------------------
 15 files changed, 108 insertions(+), 51 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 25d0cc2..2ded852 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -37,6 +37,24 @@
  * implemented by each visitor, and docs/qapi-code-gen.txt for more
  * about the QAPI code generator.
  *
+ * 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 *);
+ *
+ * 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.
+ *
  * All QAPI types have a corresponding function with a signature
  * roughly compatible with this:
  *
@@ -222,6 +240,19 @@ typedef struct GenericAlternate {
     char padding[];
 } GenericAlternate;

+/*** Visitor cleanup ***/
+
+/*
+ * Free @v and any resources it has tied up.
+ *
+ * May be called whether or not the visit has been successfully
+ * completed, but should not be called until a top-level
+ * visit_type_FOO() or visit_start_ITEM() has been performed on the
+ * visitor.  Safe if @v is NULL.
+ */
+void visit_free(Visitor *v);
+
+
 /*** Visiting structures ***/

 /*
@@ -272,7 +303,7 @@ void visit_check_struct(Visitor *v, Error **errp);
  * Must be called after any successful use of visit_start_struct(),
  * even if intermediate processing was skipped due to errors, to allow
  * the backend to release any resources.  Destroying the visitor early
- * behaves as if this was implicitly called.
+ * with visit_free() behaves as if this was implicitly called.
  */
 void visit_end_struct(Visitor *v, void **obj);

@@ -332,7 +363,7 @@ GenericList *visit_next_list(Visitor *v, GenericList *tail, 
size_t size);
  * Must be called after any successful use of visit_start_list(), even
  * if intermediate processing was skipped due to errors, to allow the
  * backend to release any resources.  Destroying the visitor early
- * behaves as if this was implicitly called.
+ * with visit_free() behaves as if this was implicitly called.
  */
 void visit_end_list(Visitor *v, void **list);

@@ -368,7 +399,7 @@ void visit_start_alternate(Visitor *v, const char *name,
  * Must be called after any successful use of visit_start_alternate(),
  * even if intermediate processing was skipped due to errors, to allow
  * the backend to release any resources.  Destroying the visitor early
- * behaves as if this was implicitly called.
+ * with visit_free() behaves as if this was implicitly called.
  *
  */
 void visit_end_alternate(Visitor *v, void **obj);
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index a495bf0..525b068 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -104,6 +104,9 @@ struct Visitor

     /* Must be set */
     VisitorType type;
+
+    /* Must be set */
+    void (*free)(Visitor *v);
 };

 #endif
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 971dc4e..77ecd47 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -62,7 +62,6 @@ static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, 
QObject **ret_out,
 {
     Error *err = NULL;
     QmpOutputVisitor *qov = qmp_output_visitor_new();
-    QapiDeallocVisitor *qdv;
     Visitor *v;

     v = qmp_output_get_visitor(qov);
@@ -74,11 +73,10 @@ static void qmp_marshal_output_%(c_name)s(%(c_type)s 
ret_in, QObject **ret_out,

 out:
     error_propagate(errp, err);
-    qmp_output_visitor_cleanup(qov);
-    qdv = qapi_dealloc_visitor_new();
-    v = qapi_dealloc_get_visitor(qdv);
+    visit_free(v);
+    v = qapi_dealloc_visitor_new();
     visit_type_%(c_name)s(v, "unused", &ret_in, NULL);
-    qapi_dealloc_visitor_cleanup(qdv);
+    visit_free(v);
 }
 ''',
                  c_type=ret_type.c_type(), c_name=ret_type.c_name())
@@ -116,7 +114,6 @@ def gen_marshal(name, arg_type, ret_type):
     if arg_type and arg_type.members:
         ret += mcgen('''
     QmpInputVisitor *qiv = qmp_input_visitor_new(QOBJECT(args), true);
-    QapiDeallocVisitor *qdv;
     Visitor *v;
     %(c_name)s arg = {0};

@@ -155,13 +152,12 @@ out:
 ''')
     if arg_type and arg_type.members:
         ret += mcgen('''
-    qmp_input_visitor_cleanup(qiv);
-    qdv = qapi_dealloc_visitor_new();
-    v = qapi_dealloc_get_visitor(qdv);
+    visit_free(v);
+    v = qapi_dealloc_visitor_new();
     visit_start_struct(v, NULL, NULL, 0, NULL);
     visit_type_%(c_name)s_members(v, &arg, NULL);
     visit_end_struct(v, NULL);
-    qapi_dealloc_visitor_cleanup(qdv);
+    visit_free(v);
 ''',
                      c_name=arg_type.c_name())

diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 084c40a..909e8d6 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -119,7 +119,7 @@ def gen_event_send(name, arg_type):
     if arg_type and arg_type.members:
         ret += mcgen('''
 out:
-    qmp_output_visitor_cleanup(qov);
+    visit_free(v);
 ''')
     ret += mcgen('''
     error_propagate(errp, err);
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 437cf6c..5ace2cf 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -150,17 +150,15 @@ def gen_type_cleanup(name):

 void qapi_free_%(c_name)s(%(c_name)s *obj)
 {
-    QapiDeallocVisitor *qdv;
     Visitor *v;

     if (!obj) {
         return;
     }

-    qdv = qapi_dealloc_visitor_new();
-    v = qapi_dealloc_get_visitor(qdv);
+    v = qapi_dealloc_visitor_new();
     visit_type_%(c_name)s(v, NULL, &obj, NULL);
-    qapi_dealloc_visitor_cleanup(qdv);
+    visit_free(v);
 }
 ''',
                 c_name=c_name(name))
diff --git a/include/qapi/dealloc-visitor.h b/include/qapi/dealloc-visitor.h
index 45b06b2..b3e5c85 100644
--- a/include/qapi/dealloc-visitor.h
+++ b/include/qapi/dealloc-visitor.h
@@ -23,9 +23,6 @@ typedef struct QapiDeallocVisitor QapiDeallocVisitor;
  * qapi_free_FOO() functions, and is the only visitor designed to work
  * correctly in the face of a partially-constructed QAPI tree.
  */
-QapiDeallocVisitor *qapi_dealloc_visitor_new(void);
-void qapi_dealloc_visitor_cleanup(QapiDeallocVisitor *d);
-
-Visitor *qapi_dealloc_get_visitor(QapiDeallocVisitor *v);
+Visitor *qapi_dealloc_visitor_new(void);

 #endif
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index dba11c6..5f68c25 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -20,6 +20,13 @@
 #include "qapi/visitor.h"
 #include "qapi/visitor-impl.h"

+void visit_free(Visitor *v)
+{
+    if (v) {
+        v->free(v);
+    }
+}
+
 void visit_start_struct(Visitor *v, const char *name, void **obj,
                         size_t size, Error **errp)
 {
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index dcfbf92..28d2203 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -513,6 +513,14 @@ opts_optional(Visitor *v, const char *name, bool *present)
 }


+static void
+opts_free(Visitor *v)
+{
+    OptsVisitor *ov = to_ov(v);
+    opts_visitor_cleanup(ov);
+}
+
+
 OptsVisitor *
 opts_visitor_new(const QemuOpts *opts)
 {
@@ -540,6 +548,7 @@ opts_visitor_new(const QemuOpts *opts)
      * skip some mandatory methods... */

     ov->visitor.optional = &opts_optional;
+    ov->visitor.free = opts_free;

     ov->opts_root = opts;

diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 9391dea..235e8a1 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -107,17 +107,12 @@ static void qapi_dealloc_type_null(Visitor *v, const char 
*name, Error **errp)
 {
 }

-Visitor *qapi_dealloc_get_visitor(QapiDeallocVisitor *v)
-{
-    return &v->visitor;
-}
-
-void qapi_dealloc_visitor_cleanup(QapiDeallocVisitor *v)
+static void qapi_dealloc_free(Visitor *v)
 {
     g_free(v);
 }

-QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
+Visitor *qapi_dealloc_visitor_new(void)
 {
     QapiDeallocVisitor *v;

@@ -138,6 +133,7 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void)
     v->visitor.type_number = qapi_dealloc_type_number;
     v->visitor.type_any = qapi_dealloc_type_anything;
     v->visitor.type_null = qapi_dealloc_type_null;
+    v->visitor.free = qapi_dealloc_free;

-    return v;
+    return &v->visitor;
 }
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 84f32fc..3ca192e 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -375,6 +375,12 @@ Visitor *qmp_input_get_visitor(QmpInputVisitor *v)
     return &v->visitor;
 }

+static void qmp_input_free(Visitor *v)
+{
+    QmpInputVisitor *qiv = to_qiv(v);
+    qmp_input_visitor_cleanup(qiv);
+}
+
 void qmp_input_visitor_cleanup(QmpInputVisitor *v)
 {
     qobject_decref(v->root);
@@ -403,6 +409,7 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj, bool 
strict)
     v->visitor.type_any = qmp_input_type_any;
     v->visitor.type_null = qmp_input_type_null;
     v->visitor.optional = qmp_input_optional;
+    v->visitor.free = qmp_input_free;
     v->strict = strict;

     v->root = obj;
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index 8f44d23..7c6a601 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -209,6 +209,12 @@ Visitor *qmp_output_get_visitor(QmpOutputVisitor *v)
     return &v->visitor;
 }

+static void qmp_output_free(Visitor *v)
+{
+    QmpOutputVisitor *qov = to_qov(v);
+    qmp_output_visitor_cleanup(qov);
+}
+
 void qmp_output_visitor_cleanup(QmpOutputVisitor *v)
 {
     QStackEntry *e, *tmp;
@@ -241,6 +247,7 @@ 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.free = qmp_output_free;

     QTAILQ_INIT(&v->stack);

diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 3b0b788..dce5b42 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -330,6 +330,12 @@ Visitor *string_input_get_visitor(StringInputVisitor *v)
     return &v->visitor;
 }

+static void string_input_free(Visitor *v)
+{
+    StringInputVisitor *siv = to_siv(v);
+    string_input_visitor_cleanup(siv);
+}
+
 void string_input_visitor_cleanup(StringInputVisitor *v)
 {
     g_list_foreach(v->ranges, free_range, NULL);
@@ -354,6 +360,7 @@ StringInputVisitor *string_input_visitor_new(const char 
*str)
     v->visitor.next_list = next_list;
     v->visitor.end_list = end_list;
     v->visitor.optional = parse_optional;
+    v->visitor.free = string_input_free;

     v->string = str;
     return v;
diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
index cec3e76..8923898 100644
--- a/qapi/string-output-visitor.c
+++ b/qapi/string-output-visitor.c
@@ -319,6 +319,12 @@ static void free_range(void *range, void *dummy)
     g_free(range);
 }

+static void string_output_free(Visitor *v)
+{
+    StringOutputVisitor *sov = to_sov(v);
+    string_output_visitor_cleanup(sov);
+}
+
 void string_output_visitor_cleanup(StringOutputVisitor *sov)
 {
     if (sov->string) {
@@ -348,6 +354,7 @@ 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.free = string_output_free;

     return v;
 }
diff --git a/tests/test-visitor-serialization.c 
b/tests/test-visitor-serialization.c
index d102fa6..f83d019 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -90,11 +90,11 @@ typedef void (*VisitorFunc)(Visitor *v, void **native, 
Error **errp);

 static void dealloc_helper(void *native_in, VisitorFunc visit, Error **errp)
 {
-    QapiDeallocVisitor *qdv = qapi_dealloc_visitor_new();
+    Visitor *v = qapi_dealloc_visitor_new();

-    visit(qapi_dealloc_get_visitor(qdv), &native_in, errp);
+    visit(v, &native_in, errp);

-    qapi_dealloc_visitor_cleanup(qdv);
+    visit_free(v);
 }

 static void visit_primitive_type(Visitor *v, void **native, Error **errp)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 79bfdf5..2830e02 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -802,32 +802,28 @@ Example:

     void qapi_free_UserDefOne(UserDefOne *obj)
     {
-        QapiDeallocVisitor *qdv;
         Visitor *v;

         if (!obj) {
             return;
         }

-        qdv = qapi_dealloc_visitor_new();
-        v = qapi_dealloc_get_visitor(qdv);
+        v = qapi_dealloc_visitor_new();
         visit_type_UserDefOne(v, NULL, &obj, NULL);
-        qapi_dealloc_visitor_cleanup(qdv);
+        visit_free(v);
     }

     void qapi_free_UserDefOneList(UserDefOneList *obj)
     {
-        QapiDeallocVisitor *qdv;
         Visitor *v;

         if (!obj) {
             return;
         }

-        qdv = qapi_dealloc_visitor_new();
-        v = qapi_dealloc_get_visitor(qdv);
+        v = qapi_dealloc_visitor_new();
         visit_type_UserDefOneList(v, NULL, &obj, NULL);
-        qapi_dealloc_visitor_cleanup(qdv);
+        visit_free(v);
     }

 === scripts/qapi-visit.py ===
@@ -985,7 +981,6 @@ Example:
     {
         Error *err = NULL;
         QmpOutputVisitor *qov = qmp_output_visitor_new();
-        QapiDeallocVisitor *qdv;
         Visitor *v;

         v = qmp_output_get_visitor(qov);
@@ -997,11 +992,10 @@ Example:

     out:
         error_propagate(errp, err);
-        qmp_output_visitor_cleanup(qov);
-        qdv = qapi_dealloc_visitor_new();
-        v = qapi_dealloc_get_visitor(qdv);
+        visit_free(v);
+        v = qapi_dealloc_visitor_new();
         visit_type_UserDefOne(v, "unused", &ret_in, NULL);
-        qapi_dealloc_visitor_cleanup(qdv);
+        visit_free(v);
     }

     static void qmp_marshal_my_command(QDict *args, QObject **ret, Error 
**errp)
@@ -1009,7 +1003,6 @@ Example:
         Error *err = NULL;
         UserDefOne *retval;
         QmpInputVisitor *qiv = qmp_input_visitor_new(QOBJECT(args), true);
-        QapiDeallocVisitor *qdv;
         Visitor *v;
         UserDefOneList *arg1 = NULL;

@@ -1036,13 +1029,12 @@ Example:

     out:
         error_propagate(errp, err);
-        qmp_input_visitor_cleanup(qiv);
-        qdv = qapi_dealloc_visitor_new();
-        v = qapi_dealloc_get_visitor(qdv);
+        visit_free(v);
+        v = qapi_dealloc_visitor_new();
         visit_start_struct(v, NULL, NULL, 0, NULL);
         visit_type_UserDefOneList(v, "arg1", &arg1, NULL);
         visit_end_struct(v, NULL);
-        qapi_dealloc_visitor_cleanup(qdv);
+        visit_free(v);
     }

     static void qmp_init_marshal(void)
-- 
2.5.5