[Qemu-devel] [RFC PATCH] qapi: split visit_end_struct() into pieces

[Eric Blake <eblake@redhat.com>]

We want to call the various visit_end_*() functions unconditionally,
so that visitors can release resources tied up since the matching
visit_start_*().  But we also have a requirement for detecting when
an input visitor did not consume everything, so the code allowed
visit_end_*() to set an error.  This led to some awkward coding,
since safely setting the error, even if an earlier one had
occurred, while still favoring the earliest error, requires either
multiple error_propagate() calls (and an understanding that unlike
error_setg(), it is safe to propagate onto an already-set error),
or else ternaries such as foo(..., err ? NULL : &err) to ignore
secondary errors if an earlier error is already known.

Splitting the functionality allows slightly cleaner idioms. Now,
clients must call visit_check_struct() if all earlier actions
have been successful for one last chance at flagging excess
input, while visit_end_struct() is unconditional and does not
set any further errors.

As it is an API change, everything must be changed in one commit
(there is no way to break this into smaller pieces): both the
generated code and other existing clients, as well as the various
visitor backends.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
TODO: There is some redundancy in the testsuite, with multiple
files hand-writing their own visit_type_TestStruct() instead
of letting the qapi generators create it.  Should this be
consolidated as a separate patch?

Probably won't apply directly to any published commit, so much
as me posting a part of my worktree early to get feedback on
whether the idea even makes sense, given how large the results
end up being.

v6: new patch, based on discussion of v5 7/46:

On 09/28/2015 03:14 AM, Markus Armbruster wrote:

>> One other potential alternative:  What if we split visit_end_struct()
>> into two visitor functions, one that checks for success, and the other
>> that is called unconditionally to clean up resources.

> I think this split could help with writing safe code: in
> visit_check_struct() you can rely on "no error so far", as usual.  In
> visit_end_struct(), you can't, but it should be a pure cleanup function,
> where that's quite normal.
>
> Looks like we're getting drawn into visitor contract territory again.
>

---
 hmp.c                              |  3 ++-
 hw/virtio/virtio-balloon.c         | 14 ++++++++------
 include/qapi/visitor-impl.h        |  8 +++++---
 include/qapi/visitor.h             | 24 ++++++++++++++++++------
 qapi/opts-visitor.c                | 17 +++++++++++++++--
 qapi/qapi-dealloc-visitor.c        |  6 +++---
 qapi/qapi-visit-core.c             | 19 +++++++++++++------
 qapi/qmp-input-visitor.c           | 36 +++++++++++++++++++++++++-----------
 qapi/qmp-output-visitor.c          |  4 ++--
 qapi/string-input-visitor.c        |  2 +-
 qapi/string-output-visitor.c       |  2 +-
 qom/object.c                       |  5 ++---
 scripts/qapi-event.py              |  5 +++--
 scripts/qapi-visit.py              | 24 ++++++++++++------------
 tests/test-qmp-input-strict.c      |  9 +++++----
 tests/test-qmp-input-visitor.c     |  9 +++++----
 tests/test-qmp-output-visitor.c    | 23 ++++++++++++++++-------
 tests/test-visitor-serialization.c |  9 +++++----
 vl.c                               | 12 +++++++-----
 19 files changed, 148 insertions(+), 83 deletions(-)

diff --git a/hmp.c b/hmp.c
index 5048eee..b7a42dc 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1658,8 +1658,9 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)

     object_add(type, id, pdict, opts_get_visitor(ov), &err);

+    visit_check_struct(opts_get_visitor(ov), &err_end);
 out_end:
-    visit_end_struct(opts_get_visitor(ov), &err_end);
+    visit_end_struct(opts_get_visitor(ov));
     if (!err && err_end) {
         qmp_object_del(id, NULL);
     }
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index c419b17..48867c4 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -132,14 +132,16 @@ static void balloon_stats_get_all(Object *obj, struct Visitor *v,
         visit_type_int64(v, (int64_t *) &s->stats[i], balloon_stat_names[i],
                          &err);
     }
-    error_propagate(errp, err);
-    err = NULL;
-    visit_end_struct(v, &err);
+    if (!err) {
+        visit_check_struct(v, &err);
+    }
+    visit_end_struct(v);

 out_end:
-    error_propagate(errp, err);
-    err = NULL;
-    visit_end_struct(v, &err);
+    if (!err) {
+        visit_check_struct(v, &err);
+    }
+    visit_end_struct(v);
 out:
     error_propagate(errp, err);
 }
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 090b19b..c53d4d3 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -28,21 +28,23 @@ struct Visitor
      * currently visit structs). */
     void (*start_struct)(Visitor *v, void **obj, const char *kind,
                          const char *name, size_t size, Error **errp);
+    /* May be NULL; most useful for input visitors. */
+    void (*check_struct)(Visitor *v, Error **errp);
     /* Must be provided if start_struct is present. */
-    void (*end_struct)(Visitor *v, Error **errp);
+    void (*end_struct)(Visitor *v);

     /* May be NULL; most useful for input visitors. */
     void (*start_implicit_struct)(Visitor *v, void **obj, size_t size,
                                   Error **errp);
     /* May be NULL */
-    void (*end_implicit_struct)(Visitor *v, Error **errp);
+    void (*end_implicit_struct)(Visitor *v);

     /* Must be set */
     void (*start_list)(Visitor *v, const char *name, Error **errp);
     /* Must be set */
     GenericList *(*next_list)(Visitor *v, GenericList **list, Error **errp);
     /* Must be set */
-    void (*end_list)(Visitor *v, Error **errp);
+    void (*end_list)(Visitor *v);

     /* Must be set, although the helpers input_type_enum() and
      * output_type_enum() can be used.  */
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 6534cca..708f55c 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -56,11 +56,19 @@ typedef struct GenericList
 void visit_start_struct(Visitor *v, void **obj, const char *kind,
                         const char *name, size_t size, Error **errp);
 /**
+ * Check whether completing a struct is safe.
+ * Should be called prior to visit_end_struct() if all other intermediate
+ * visit steps were successful, to allow the caller one last chance to
+ * report errors such as remaining data that was not consumed by the visit.
+ */
+void visit_check_struct(Visitor *v, Error **errp);
+/**
  * Complete a struct visit started earlier.
  * Must be called after any successful use of visit_start_struct(),
- * even if intermediate processing was skipped due to errors.
+ * even if intermediate processing was skipped due to errors, to allow
+ * the backend to release any resources.
  */
-void visit_end_struct(Visitor *v, Error **errp);
+void visit_end_struct(Visitor *v);

 /**
  * Prepare to visit an implicit struct.
@@ -76,9 +84,12 @@ void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
 /**
  * Complete an implicit struct visit started earlier.
  * Must be called after any successful use of visit_start_implicit_struct(),
- * even if intermediate processing was skipped due to errors.
+ * even if intermediate processing was skipped due to errors.  Unlike
+ * visit_end_struct(), there is no need for a prior check call (since
+ * an implicit object is a subset of larger object, checking before ending
+ * the overall struct is sufficient).
  */
-void visit_end_implicit_struct(Visitor *v, Error **errp);
+void visit_end_implicit_struct(Visitor *v);

 /**
  * Prepare to visit an array tied to an object key @name.
@@ -101,9 +112,10 @@ GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp);
 /**
  * Complete the list started earlier.
  * Must be called after any successful use of visit_start_list(),
- * even if intermediate processing was skipped due to errors.
+ * even if intermediate processing was skipped due to errors, to allow
+ * the backend to release any resources.
  */
-void visit_end_list(Visitor *v, Error **errp);
+void visit_end_list(Visitor *v);

 /**
  * Check if an optional member @name of an object needs visiting.
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index cd10392..39dc431 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -158,7 +158,7 @@ ghr_true(gpointer ign_key, gpointer ign_value, gpointer ign_user_data)


 static void
-opts_end_struct(Visitor *v, Error **errp)
+opts_check_struct(Visitor *v, Error **errp)
 {
     OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
     GQueue *any;
@@ -175,6 +175,18 @@ opts_end_struct(Visitor *v, Error **errp)
         first = g_queue_peek_head(any);
         error_setg(errp, QERR_INVALID_PARAMETER, first->name);
     }
+}
+
+
+static void
+opts_end_struct(Visitor *v)
+{
+    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+
+    if (--ov->depth > 0) {
+        return;
+    }
+
     g_hash_table_destroy(ov->unprocessed_opts);
     ov->unprocessed_opts = NULL;
     if (ov->fake_id_opt) {
@@ -263,7 +275,7 @@ opts_next_list(Visitor *v, GenericList **list, Error **errp)


 static void
-opts_end_list(Visitor *v, Error **errp)
+opts_end_list(Visitor *v)
 {
     OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);

@@ -506,6 +518,7 @@ opts_visitor_new(const QemuOpts *opts)
     ov = g_malloc0(sizeof *ov);

     ov->visitor.start_struct = &opts_start_struct;
+    ov->visitor.check_struct = &opts_check_struct;
     ov->visitor.end_struct   = &opts_end_struct;

     ov->visitor.start_list = &opts_start_list;
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 737deab..a2139dd 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -67,7 +67,7 @@ static void qapi_dealloc_start_struct(Visitor *v, void **obj, const char *kind,
     qapi_dealloc_push(qov, obj);
 }

-static void qapi_dealloc_end_struct(Visitor *v, Error **errp)
+static void qapi_dealloc_end_struct(Visitor *v)
 {
     QapiDeallocVisitor *qov = to_qov(v);
     void **obj = qapi_dealloc_pop(qov);
@@ -85,7 +85,7 @@ static void qapi_dealloc_start_implicit_struct(Visitor *v,
     qapi_dealloc_push(qov, obj);
 }

-static void qapi_dealloc_end_implicit_struct(Visitor *v, Error **errp)
+static void qapi_dealloc_end_implicit_struct(Visitor *v)
 {
     QapiDeallocVisitor *qov = to_qov(v);
     void **obj = qapi_dealloc_pop(qov);
@@ -121,7 +121,7 @@ static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList **listp,
     return NULL;
 }

-static void qapi_dealloc_end_list(Visitor *v, Error **errp)
+static void qapi_dealloc_end_list(Visitor *v)
 {
     QapiDeallocVisitor *qov = to_qov(v);
     void *obj = qapi_dealloc_pop(qov);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index cbf7780..82d54af 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -23,9 +23,16 @@ void visit_start_struct(Visitor *v, void **obj, const char *kind,
     v->start_struct(v, obj, kind, name, size, errp);
 }

-void visit_end_struct(Visitor *v, Error **errp)
+void visit_check_struct(Visitor *v, Error **errp)
 {
-    v->end_struct(v, errp);
+    if (v->check_struct) {
+        v->check_struct(v, errp);
+    }
+}
+
+void visit_end_struct(Visitor *v)
+{
+    v->end_struct(v);
 }

 void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
@@ -36,10 +43,10 @@ void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
     }
 }

-void visit_end_implicit_struct(Visitor *v, Error **errp)
+void visit_end_implicit_struct(Visitor *v)
 {
     if (v->end_implicit_struct) {
-        v->end_implicit_struct(v, errp);
+        v->end_implicit_struct(v);
     }
 }

@@ -53,9 +60,9 @@ GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp)
     return v->next_list(v, list, errp);
 }

-void visit_end_list(Visitor *v, Error **errp)
+void visit_end_list(Visitor *v)
 {
-    v->end_list(v, errp);
+    v->end_list(v);
 }

 bool visit_start_union(Visitor *v, bool data_present, Error **errp)
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 5310db5..f10f74f 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -88,14 +88,14 @@ static void qmp_input_push(QmpInputVisitor *qiv, QObject *obj, Error **errp)
     qiv->nb_stack++;
 }

-/** Only for qmp_input_pop. */
+/** Only for qmp_input_check. */
 static gboolean always_true(gpointer key, gpointer val, gpointer user_pkey)
 {
     *(const char **)user_pkey = (const char *)key;
     return TRUE;
 }

-static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
+static void qmp_input_check(QmpInputVisitor *qiv, Error **errp)
 {
     assert(qiv->nb_stack > 0);

@@ -107,6 +107,17 @@ static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
                 g_hash_table_find(top_ht, always_true, &key);
                 error_setg(errp, QERR_QMP_EXTRA_MEMBER, key);
             }
+        }
+    }
+}
+
+static void qmp_input_pop(QmpInputVisitor *qiv)
+{
+    assert(qiv->nb_stack > 0);
+
+    if (qiv->strict) {
+        GHashTable * const top_ht = qiv->stack[qiv->nb_stack - 1].h;
+        if (top_ht) {
             g_hash_table_unref(top_ht);
         }
     }
@@ -138,11 +149,18 @@ static void qmp_input_start_struct(Visitor *v, void **obj, const char *kind,
     }
 }

-static void qmp_input_end_struct(Visitor *v, Error **errp)
+static void qmp_input_check_struct(Visitor *v, Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);

-    qmp_input_pop(qiv, errp);
+    qmp_input_check(qiv, errp);
+}
+
+static void qmp_input_end_struct(Visitor *v)
+{
+    QmpInputVisitor *qiv = to_qiv(v);
+
+    qmp_input_pop(qiv);
 }

 static void qmp_input_start_implicit_struct(Visitor *v, void **obj,
@@ -153,10 +171,6 @@ static void qmp_input_start_implicit_struct(Visitor *v, void **obj,
     }
 }

-static void qmp_input_end_implicit_struct(Visitor *v, Error **errp)
-{
-}
-
 static void qmp_input_start_list(Visitor *v, const char *name, Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
@@ -201,11 +215,11 @@ static GenericList *qmp_input_next_list(Visitor *v, GenericList **list,
     return entry;
 }

-static void qmp_input_end_list(Visitor *v, Error **errp)
+static void qmp_input_end_list(Visitor *v)
 {
     QmpInputVisitor *qiv = to_qiv(v);

-    qmp_input_pop(qiv, errp);
+    qmp_input_pop(qiv);
 }

 static void qmp_input_get_next_type(Visitor *v, qtype_code *type,
@@ -332,9 +346,9 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
     v = g_malloc0(sizeof(*v));

     v->visitor.start_struct = qmp_input_start_struct;
+    v->visitor.check_struct = qmp_input_check_struct;
     v->visitor.end_struct = qmp_input_end_struct;
     v->visitor.start_implicit_struct = qmp_input_start_implicit_struct;
-    v->visitor.end_implicit_struct = qmp_input_end_implicit_struct;
     v->visitor.start_list = qmp_input_start_list;
     v->visitor.next_list = qmp_input_next_list;
     v->visitor.end_list = qmp_input_end_list;
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index f891e72..b9fba7b 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -122,7 +122,7 @@ static void qmp_output_start_struct(Visitor *v, void **obj, const char *kind,
     qmp_output_push(qov, dict);
 }

-static void qmp_output_end_struct(Visitor *v, Error **errp)
+static void qmp_output_end_struct(Visitor *v)
 {
     QmpOutputVisitor *qov = to_qov(v);
     qmp_output_pop(qov);
@@ -153,7 +153,7 @@ static GenericList *qmp_output_next_list(Visitor *v, GenericList **listp,
     return list ? list->next : NULL;
 }

-static void qmp_output_end_list(Visitor *v, Error **errp)
+static void qmp_output_end_list(Visitor *v)
 {
     QmpOutputVisitor *qov = to_qov(v);
     qmp_output_pop(qov);
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index bbd6a54..97e00c8 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -173,7 +173,7 @@ next_list(Visitor *v, GenericList **list, Error **errp)
 }

 static void
-end_list(Visitor *v, Error **errp)
+end_list(Visitor *v)
 {
     StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
     siv->head = true;
diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
index b86ce2c..b269f39 100644
--- a/qapi/string-output-visitor.c
+++ b/qapi/string-output-visitor.c
@@ -290,7 +290,7 @@ next_list(Visitor *v, GenericList **list, Error **errp)
 }

 static void
-end_list(Visitor *v, Error **errp)
+end_list(Visitor *v)
 {
     StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);

diff --git a/qom/object.c b/qom/object.c
index 11cd86b..6d92e01 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1844,10 +1844,9 @@ static void property_get_tm(Object *obj, Visitor *v, void *opaque,
     if (err) {
         goto out_end;
     }
+    visit_check_struct(v, &err);
 out_end:
-    error_propagate(errp, err);
-    err = NULL;
-    visit_end_struct(v, errp);
+    visit_end_struct(v);
 out:
     error_propagate(errp, err);

diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 720486f..250e753 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -73,13 +73,14 @@ def gen_event_send(name, arg_type):
         ret += gen_err_check()
         ret += gen_visit_fields(arg_type.members, need_cast=True)
         ret += mcgen('''
-    visit_end_struct(v, &err);
+    visit_check_struct(v, &err);
     if (err) {
         goto out;
     }
+    visit_end_struct(v);

     obj = qmp_output_get_qobject(qov);
-    g_assert(obj != NULL);
+    g_assert(obj);

     qdict_put_obj(qmp, "data", obj);
 ''')
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 1ac5350..c5a32cf 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -52,7 +52,7 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error *
     visit_start_implicit_struct(v, (void **)obj, sizeof(%(c_type)s), &err);
     if (!err) {
         visit_type_%(c_type)s_fields(v, obj, errp);
-        visit_end_implicit_struct(v, &err);
+        visit_end_implicit_struct(v);
     }
     error_propagate(errp, err);
 }
@@ -115,9 +115,12 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
     visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
     if (!err) {
         if (*obj) {
-            visit_type_%(c_name)s_fields(v, obj, errp);
+            visit_type_%(c_name)s_fields(v, obj, &err);
         }
-        visit_end_struct(v, &err);
+        if (!err) {
+            visit_check_struct(v, &err);
+        }
+        visit_end_struct(v);
     }
     error_propagate(errp, err);
 }
@@ -147,9 +150,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
         visit_type_%(c_elt_type)s(v, &native_i->value, NULL, &err);
     }

-    error_propagate(errp, err);
-    err = NULL;
-    visit_end_list(v, &err);
+    visit_end_list(v);
 out:
     error_propagate(errp, err);
 }
@@ -208,9 +209,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
                    "%(name)s");
     }
 out_obj:
-    error_propagate(errp, err);
-    err = NULL;
-    visit_end_implicit_struct(v, &err);
+    visit_end_implicit_struct(v);
 out:
     error_propagate(errp, err);
 }
@@ -297,13 +296,14 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
     default:
         abort();
     }
+    if (!err) {
+        visit_check_struct(v, &err);
+    }
 out_obj:
     error_propagate(errp, err);
     err = NULL;
     visit_end_union(v, !!(*obj)->data, &err);
-    error_propagate(errp, err);
-    err = NULL;
-    visit_end_struct(v, &err);
+    visit_end_struct(v);
 out:
     error_propagate(errp, err);
 }
diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
index 1c13c8f..da9dafd 100644
--- a/tests/test-qmp-input-strict.c
+++ b/tests/test-qmp-input-strict.c
@@ -118,11 +118,12 @@ static void visit_type_TestStruct(Visitor *v, TestStruct **obj,
         goto out_end;
     }
     visit_type_str(v, &(*obj)->string, "string", &err);
-
+    if (err) {
+        goto out_end;
+    }
+    visit_check_struct(v, &err);
 out_end:
-    error_propagate(errp, err);
-    err = NULL;
-    visit_end_struct(v, &err);
+    visit_end_struct(v);
 out:
     error_propagate(errp, err);
 }
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index ef5871b..38b1436 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -200,11 +200,12 @@ static void visit_type_TestStruct(Visitor *v, TestStruct **obj,
         goto out_end;
     }
     visit_type_str(v, &(*obj)->string, "string", &err);
-
+    if (err) {
+        goto out_end;
+    }
+    visit_check_struct(v, &err);
 out_end:
-    error_propagate(errp, err);
-    err = NULL;
-    visit_end_struct(v, &err);
+    visit_end_struct(v);
 out:
     error_propagate(errp, err);
 }
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 11996d0..f836588 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -194,10 +194,11 @@ static void visit_type_TestStruct(Visitor *v, TestStruct **obj,
     }
     visit_type_str(v, &(*obj)->string, "string", &err);

+    if (!err) {
+        visit_check_struct(v, &err);
+    }
 out_end:
-    error_propagate(errp, err);
-    err = NULL;
-    visit_end_struct(v, &err);
+    visit_end_struct(v);
 out:
     error_propagate(errp, err);
 }
@@ -329,15 +330,23 @@ static void visit_type_TestStructList(Visitor *v, TestStructList **obj,
                                       const char *name, Error **errp)
 {
     GenericList *i, **head = (GenericList **)obj;
+    Error *err = NULL;

-    visit_start_list(v, name, errp);
+    visit_start_list(v, name, &err);
+    if (err) {
+        goto out;
+    }

-    for (*head = i = visit_next_list(v, head, errp); i; i = visit_next_list(v, &i, errp)) {
+    for (*head = i = visit_next_list(v, head, &err);
+         !err && i;
+         i = visit_next_list(v, &i, &err)) {
         TestStructList *native_i = (TestStructList *)i;
-        visit_type_TestStruct(v, &native_i->value, NULL, errp);
+        visit_type_TestStruct(v, &native_i->value, NULL, &err);
     }

-    visit_end_list(v, errp);
+    visit_end_list(v);
+out:
+    error_propagate(errp, err);
 }

 static void test_visitor_out_list(TestOutputVisitorData *data,
diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c
index fa86cae..dd50d69 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -212,11 +212,12 @@ static void visit_type_TestStruct(Visitor *v, TestStruct **obj,
         goto out_end;
     }
     visit_type_str(v, &(*obj)->string, "string", &err);
-
+    if (err) {
+        goto out_end;
+    }
+    visit_check_struct(v, &err);
 out_end:
-    error_propagate(errp, err);
-    err = NULL;
-    visit_end_struct(v, &err);
+    visit_end_struct(v);
 out:
     error_propagate(errp, err);
 }
diff --git a/vl.c b/vl.c
index 8d1846c..8b944aa 100644
--- a/vl.c
+++ b/vl.c
@@ -2796,23 +2796,25 @@ static int object_create(void *opaque, QemuOpts *opts, Error **errp)
     qdict_del(pdict, "qom-type");
     visit_type_str(opts_get_visitor(ov), &type, "qom-type", &err);
     if (err) {
-        goto out;
+        goto obj_out;
     }
     if (!type_predicate(type)) {
-        goto out;
+        goto obj_out;
     }

     qdict_del(pdict, "id");
     visit_type_str(opts_get_visitor(ov), &id, "id", &err);
     if (err) {
-        goto out;
+        goto obj_out;
     }

     object_add(type, id, pdict, opts_get_visitor(ov), &err);
     if (err) {
-        goto out;
+        goto obj_out;
     }
-    visit_end_struct(opts_get_visitor(ov), &err);
+    visit_check_struct(opts_get_visitor(ov), &err);
+obj_out:
+    visit_end_struct(opts_get_visitor(ov));
     if (err) {
         qmp_object_del(id, NULL);
     }
-- 
2.4.3