[Qemu-devel] [PATCH 12/13] qapi: Replace uncommon use of the error API by the common one

[Markus Armbruster <armbru@redhat.com>]

We commonly use the error API like this:

    err = NULL;
    foo(..., &err);
    if (err) {
        goto out;
    }
    bar(..., &err);

Every error source is checked separately.  The second function is only
called when the first one succeeds.  Both functions are free to pass
their argument to error_set().  Because error_set() asserts no error
has been set, this effectively means they must not be called with an
error set.

The qapi-generated code uses the error API differently:

    // *errp was initialized to NULL somewhere up the call chain
    frob(..., errp);
    gnat(..., errp);

Errors accumulate in *errp: first error wins, subsequent errors get
dropped.  To make this work, the second function does nothing when
called with an error set.  Requires non-null errp, or else the second
function can't see the first one fail.

This usage has also bled into visitor tests, and two device model
object property getters rtc_get_date() and balloon_stats_get_all().

With the "accumulate" technique, you need fewer error checks in
callers, and buy that with an error check in every callee.  Can be
nice.

However, mixing the two techniques is confusing.  You can't use the
"accumulate" technique with functions designed for the "check
separately" technique.  You can use the "check separately" technique
with functions designed for the "accumulate" technique, but then
error_set() can't catch you setting an error more than once.

Standardize on the "check separately" technique for now, because it's
overwhelmingly prevalent.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/qapi-code-gen.txt             |  87 ++++++++------
 hw/timer/mc146818rtc.c             |  24 +++-
 hw/virtio/virtio-balloon.c         |  12 +-
 qapi/qapi-visit-core.c             | 231 ++++++++++++++++---------------------
 scripts/qapi-commands.py           |  55 ++++++---
 scripts/qapi-visit.py              | 169 +++++++++++++--------------
 tests/test-qmp-input-strict.c      |  10 +-
 tests/test-qmp-input-visitor.c     |  26 +++--
 tests/test-qmp-output-visitor.c    |  10 +-
 tests/test-visitor-serialization.c |  12 +-
 10 files changed, 350 insertions(+), 286 deletions(-)

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index ac951ef..eb32981 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -240,7 +240,6 @@ Example:
         qapi_dealloc_visitor_cleanup(md);
     }
 
-
     void qapi_free_UserDefOne(UserDefOne * obj)
     {
         QapiDeallocVisitor *md;
@@ -317,49 +316,54 @@ Example:
     {
         Error *err = NULL;
         visit_type_int(m, &(*obj)->integer, "integer", &err);
+        if (err) {
+            goto out;
+        }
         visit_type_str(m, &(*obj)->string, "string", &err);
+        if (err) {
+            goto out;
+        }
 
+    out:
         error_propagate(errp, err);
     }
 
     void visit_type_UserDefOne(Visitor *m, UserDefOne ** obj, const char *name, Error **errp)
     {
-        if (!error_is_set(errp)) {
-            Error *err = NULL;
-            visit_start_struct(m, (void **)obj, "UserDefOne", name, sizeof(UserDefOne), &err);
-            if (!err) {
-                if (*obj) {
-                    visit_type_UserDefOne_fields(m, obj, &err);
-                    error_propagate(errp, err);
-                    err = NULL;
-                }
-                /* Always call end_struct if start_struct succeeded.  */
-                visit_end_struct(m, &err);
+        Error *err = NULL;
+
+        visit_start_struct(m, (void **)obj, "UserDefOne", name, sizeof(UserDefOne), &err);
+        if (!err) {
+            if (*obj) {
+                visit_type_UserDefOne_fields(m, obj, errp);
             }
-            error_propagate(errp, err);
+            visit_end_struct(m, &err);
         }
+        error_propagate(errp, err);
     }
 
     void visit_type_UserDefOneList(Visitor *m, UserDefOneList ** obj, const char *name, Error **errp)
     {
-        GenericList *i, **prev = (GenericList **)obj;
         Error *err = NULL;
+        GenericList *i, **prev;
 
-        if (!error_is_set(errp)) {
-            visit_start_list(m, name, &err);
-            if (!err) {
-                for (; (i = visit_next_list(m, prev, &err)) != NULL; prev = &i) {
-                    UserDefOneList *native_i = (UserDefOneList *)i;
-                    visit_type_UserDefOne(m, &native_i->value, NULL, &err);
-                }
-                error_propagate(errp, err);
-                err = NULL;
-
-                /* Always call end_list if start_list succeeded.  */
-                visit_end_list(m, &err);
-            }
-            error_propagate(errp, err);
+        visit_start_list(m, name, &err);
+        if (err) {
+            goto out;
+        }
+
+        for (prev = (GenericList **)obj;
+             !err && (i = visit_next_list(m, prev, &err)) != NULL;
+             prev = &i) {
+            UserDefOneList *native_i = (UserDefOneList *)i;
+            visit_type_UserDefOne(m, &native_i->value, NULL, &err);
         }
+
+        error_propagate(errp, err);
+        err = NULL;
+        visit_end_list(m, &err);
+    out:
+        error_propagate(errp, err);
     }
     mdroth@illuin:~/w/qemu2.git$ cat qapi-generated/example-qapi-visit.h
 [Uninteresting stuff omitted...]
@@ -399,15 +403,20 @@ Example:
 
     static void qmp_marshal_output_my_command(UserDefOne * ret_in, QObject **ret_out, Error **errp)
     {
+        Error *local_err = NULL;
         QmpOutputVisitor *mo = qmp_output_visitor_new();
         QapiDeallocVisitor *md;
         Visitor *v;
 
         v = qmp_output_get_visitor(mo);
-        visit_type_UserDefOne(v, &ret_in, "unused", errp);
-        if (!error_is_set(errp)) {
-            *ret_out = qmp_output_get_qobject(mo);
+        visit_type_UserDefOne(v, &ret_in, "unused", &local_err);
+        if (local_err) {
+            goto out;
         }
+        *ret_out = qmp_output_get_qobject(mo);
+
+    out:
+        error_propagate(errp, local_err);
         qmp_output_visitor_cleanup(mo);
         md = qapi_dealloc_visitor_new();
         v = qapi_dealloc_get_visitor(md);
@@ -417,6 +426,7 @@ Example:
 
     static void qmp_marshal_input_my_command(QDict *args, QObject **ret, Error **errp)
     {
+        Error *local_err = NULL;
         UserDefOne * retval = NULL;
         QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
         QapiDeallocVisitor *md;
@@ -424,17 +434,20 @@ Example:
         UserDefOne * arg1 = NULL;
 
         v = qmp_input_get_visitor(mi);
-        visit_type_UserDefOne(v, &arg1, "arg1", errp);
-
-        if (error_is_set(errp)) {
+        visit_type_UserDefOne(v, &arg1, "arg1", &local_err);
+        if (local_err) {
             goto out;
         }
-        retval = qmp_my_command(arg1, errp);
-        if (!error_is_set(errp)) {
-            qmp_marshal_output_my_command(retval, ret, errp);
+
+        retval = qmp_my_command(arg1, &local_err);
+        if (local_err) {
+            goto out;
         }
 
+        qmp_marshal_output_my_command(retval, ret, &local_err);
+
     out:
+        error_propagate(errp, local_err);
         qmp_input_visitor_cleanup(mi);
         md = qapi_dealloc_visitor_new();
         v = qapi_dealloc_get_visitor(md);
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 6c3e3b6..df54546 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -804,13 +804,33 @@ static void rtc_get_date(Object *obj, Visitor *v, void *opaque,
         goto out;
     }
     visit_type_int32(v, &current_tm.tm_year, "tm_year", &err);
+    if (err) {
+        goto out_end;
+    }
     visit_type_int32(v, &current_tm.tm_mon, "tm_mon", &err);
+    if (err) {
+        goto out_end;
+    }
     visit_type_int32(v, &current_tm.tm_mday, "tm_mday", &err);
+    if (err) {
+        goto out_end;
+    }
     visit_type_int32(v, &current_tm.tm_hour, "tm_hour", &err);
+    if (err) {
+        goto out_end;
+    }
     visit_type_int32(v, &current_tm.tm_min, "tm_min", &err);
+    if (err) {
+        goto out_end;
+    }
     visit_type_int32(v, &current_tm.tm_sec, "tm_sec", &err);
-    visit_end_struct(v, &err);
-
+    if (err) {
+        goto out_end;
+    }
+out_end:
+    error_propagate(errp, err);
+    err = NULL;
+    visit_end_struct(v, errp);
 out:
     error_propagate(errp, err);
 }
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 65d05c8..178d133 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -121,23 +121,27 @@ static void balloon_stats_get_all(Object *obj, struct Visitor *v,
     if (err) {
         goto out;
     }
-
     visit_type_int(v, &s->stats_last_update, "last-update", &err);
+    if (err) {
+        goto out_end;
+    }
 
     visit_start_struct(v, NULL, NULL, "stats", 0, &err);
     if (err) {
         goto out_end;
     }
-        
-    for (i = 0; i < VIRTIO_BALLOON_S_NR; i++) {
+    for (i = 0; err && i < VIRTIO_BALLOON_S_NR; i++) {
         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);
 
 out_end:
+    error_propagate(errp, err);
+    err = NULL;
     visit_end_struct(v, &err);
-
 out:
     error_propagate(errp, err);
 }
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index ffd7637..55f8d40 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -20,28 +20,24 @@
 void visit_start_struct(Visitor *v, void **obj, const char *kind,
                         const char *name, size_t size, Error **errp)
 {
-    if (!error_is_set(errp)) {
-        v->start_struct(v, obj, kind, name, size, errp);
-    }
+    v->start_struct(v, obj, kind, name, size, errp);
 }
 
 void visit_end_struct(Visitor *v, Error **errp)
 {
-    assert(!error_is_set(errp));
     v->end_struct(v, errp);
 }
 
 void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
                                  Error **errp)
 {
-    if (!error_is_set(errp) && v->start_implicit_struct) {
+    if (v->start_implicit_struct) {
         v->start_implicit_struct(v, obj, size, errp);
     }
 }
 
 void visit_end_implicit_struct(Visitor *v, Error **errp)
 {
-    assert(!error_is_set(errp));
     if (v->end_implicit_struct) {
         v->end_implicit_struct(v, errp);
     }
@@ -49,30 +45,23 @@ void visit_end_implicit_struct(Visitor *v, Error **errp)
 
 void visit_start_list(Visitor *v, const char *name, Error **errp)
 {
-    if (!error_is_set(errp)) {
-        v->start_list(v, name, errp);
-    }
+    v->start_list(v, name, errp);
 }
 
 GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp)
 {
-    if (!error_is_set(errp)) {
-        return v->next_list(v, list, errp);
-    }
-
-    return 0;
+    return v->next_list(v, list, errp);
 }
 
 void visit_end_list(Visitor *v, Error **errp)
 {
-    assert(!error_is_set(errp));
     v->end_list(v, errp);
 }
 
 void visit_optional(Visitor *v, bool *present, const char *name,
                     Error **errp)
 {
-    if (!error_is_set(errp) && v->optional) {
+    if (v->optional) {
         v->optional(v, present, name, errp);
     }
 }
@@ -80,7 +69,7 @@ void visit_optional(Visitor *v, bool *present, const char *name,
 void visit_get_next_type(Visitor *v, int *obj, const int *qtypes,
                          const char *name, Error **errp)
 {
-    if (!error_is_set(errp) && v->get_next_type) {
+    if (v->get_next_type) {
         v->get_next_type(v, obj, qtypes, name, errp);
     }
 }
@@ -88,192 +77,172 @@ void visit_get_next_type(Visitor *v, int *obj, const int *qtypes,
 void visit_type_enum(Visitor *v, int *obj, const char *strings[],
                      const char *kind, const char *name, Error **errp)
 {
-    if (!error_is_set(errp)) {
-        v->type_enum(v, obj, strings, kind, name, errp);
-    }
+    v->type_enum(v, obj, strings, kind, name, errp);
 }
 
 void visit_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp)
 {
-    if (!error_is_set(errp)) {
-        v->type_int(v, obj, name, errp);
-    }
+    v->type_int(v, obj, name, errp);
 }
 
 void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error **errp)
 {
     int64_t value;
-    if (!error_is_set(errp)) {
-        if (v->type_uint8) {
-            v->type_uint8(v, obj, name, errp);
-        } else {
-            value = *obj;
-            v->type_int(v, &value, name, errp);
-            if (value < 0 || value > UINT8_MAX) {
-                error_set(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
-                          "uint8_t");
-                return;
-            }
-            *obj = value;
+
+    if (v->type_uint8) {
+        v->type_uint8(v, obj, name, errp);
+    } else {
+        value = *obj;
+        v->type_int(v, &value, name, errp);
+        if (value < 0 || value > UINT8_MAX) {
+            error_set(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
+                      "uint8_t");
+            return;
         }
+        *obj = value;
     }
 }
 
 void visit_type_uint16(Visitor *v, uint16_t *obj, const char *name, Error **errp)
 {
     int64_t value;
-    if (!error_is_set(errp)) {
-        if (v->type_uint16) {
-            v->type_uint16(v, obj, name, errp);
-        } else {
-            value = *obj;
-            v->type_int(v, &value, name, errp);
-            if (value < 0 || value > UINT16_MAX) {
-                error_set(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
-                          "uint16_t");
-                return;
-            }
-            *obj = value;
+
+    if (v->type_uint16) {
+        v->type_uint16(v, obj, name, errp);
+    } else {
+        value = *obj;
+        v->type_int(v, &value, name, errp);
+        if (value < 0 || value > UINT16_MAX) {
+            error_set(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
+                      "uint16_t");
+            return;
         }
+        *obj = value;
     }
 }
 
 void visit_type_uint32(Visitor *v, uint32_t *obj, const char *name, Error **errp)
 {
     int64_t value;
-    if (!error_is_set(errp)) {
-        if (v->type_uint32) {
-            v->type_uint32(v, obj, name, errp);
-        } else {
-            value = *obj;
-            v->type_int(v, &value, name, errp);
-            if (value < 0 || value > UINT32_MAX) {
-                error_set(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
-                          "uint32_t");
-                return;
-            }
-            *obj = value;
+
+    if (v->type_uint32) {
+        v->type_uint32(v, obj, name, errp);
+    } else {
+        value = *obj;
+        v->type_int(v, &value, name, errp);
+        if (value < 0 || value > UINT32_MAX) {
+            error_set(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
+                      "uint32_t");
+            return;
         }
+        *obj = value;
     }
 }
 
 void visit_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
 {
     int64_t value;
-    if (!error_is_set(errp)) {
-        if (v->type_uint64) {
-            v->type_uint64(v, obj, name, errp);
-        } else {
-            value = *obj;
-            v->type_int(v, &value, name, errp);
-            *obj = value;
-        }
+
+    if (v->type_uint64) {
+        v->type_uint64(v, obj, name, errp);
+    } else {
+        value = *obj;
+        v->type_int(v, &value, name, errp);
+        *obj = value;
     }
 }
 
 void visit_type_int8(Visitor *v, int8_t *obj, const char *name, Error **errp)
 {
     int64_t value;
-    if (!error_is_set(errp)) {
-        if (v->type_int8) {
-            v->type_int8(v, obj, name, errp);
-        } else {
-            value = *obj;
-            v->type_int(v, &value, name, errp);
-            if (value < INT8_MIN || value > INT8_MAX) {
-                error_set(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
-                          "int8_t");
-                return;
-            }
-            *obj = value;
+
+    if (v->type_int8) {
+        v->type_int8(v, obj, name, errp);
+    } else {
+        value = *obj;
+        v->type_int(v, &value, name, errp);
+        if (value < INT8_MIN || value > INT8_MAX) {
+            error_set(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
+                      "int8_t");
+            return;
         }
+        *obj = value;
     }
 }
 
 void visit_type_int16(Visitor *v, int16_t *obj, const char *name, Error **errp)
 {
     int64_t value;
-    if (!error_is_set(errp)) {
-        if (v->type_int16) {
-            v->type_int16(v, obj, name, errp);
-        } else {
-            value = *obj;
-            v->type_int(v, &value, name, errp);
-            if (value < INT16_MIN || value > INT16_MAX) {
-                error_set(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
-                          "int16_t");
-                return;
-            }
-            *obj = value;
+
+    if (v->type_int16) {
+        v->type_int16(v, obj, name, errp);
+    } else {
+        value = *obj;
+        v->type_int(v, &value, name, errp);
+        if (value < INT16_MIN || value > INT16_MAX) {
+            error_set(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
+                      "int16_t");
+            return;
         }
+        *obj = value;
     }
 }
 
 void visit_type_int32(Visitor *v, int32_t *obj, const char *name, Error **errp)
 {
     int64_t value;
-    if (!error_is_set(errp)) {
-        if (v->type_int32) {
-            v->type_int32(v, obj, name, errp);
-        } else {
-            value = *obj;
-            v->type_int(v, &value, name, errp);
-            if (value < INT32_MIN || value > INT32_MAX) {
-                error_set(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
-                          "int32_t");
-                return;
-            }
-            *obj = value;
+
+    if (v->type_int32) {
+        v->type_int32(v, obj, name, errp);
+    } else {
+        value = *obj;
+        v->type_int(v, &value, name, errp);
+        if (value < INT32_MIN || value > INT32_MAX) {
+            error_set(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
+                      "int32_t");
+            return;
         }
+        *obj = value;
     }
 }
 
 void visit_type_int64(Visitor *v, int64_t *obj, const char *name, Error **errp)
 {
-    if (!error_is_set(errp)) {
-        if (v->type_int64) {
-            v->type_int64(v, obj, name, errp);
-        } else {
-            v->type_int(v, obj, name, errp);
-        }
+    if (v->type_int64) {
+        v->type_int64(v, obj, name, errp);
+    } else {
+        v->type_int(v, obj, name, errp);
     }
 }
 
 void visit_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp)
 {
     int64_t value;
-    if (!error_is_set(errp)) {
-        if (v->type_size) {
-            v->type_size(v, obj, name, errp);
-        } else if (v->type_uint64) {
-            v->type_uint64(v, obj, name, errp);
-        } else {
-            value = *obj;
-            v->type_int(v, &value, name, errp);
-            *obj = value;
-        }
+
+    if (v->type_size) {
+        v->type_size(v, obj, name, errp);
+    } else if (v->type_uint64) {
+        v->type_uint64(v, obj, name, errp);
+    } else {
+        value = *obj;
+        v->type_int(v, &value, name, errp);
+        *obj = value;
     }
 }
 
 void visit_type_bool(Visitor *v, bool *obj, const char *name, Error **errp)
 {
-    if (!error_is_set(errp)) {
-        v->type_bool(v, obj, name, errp);
-    }
+    v->type_bool(v, obj, name, errp);
 }
 
 void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp)
 {
-    if (!error_is_set(errp)) {
-        v->type_str(v, obj, name, errp);
-    }
+    v->type_str(v, obj, name, errp);
 }
 
 void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp)
 {
-    if (!error_is_set(errp)) {
-        v->type_number(v, obj, name, errp);
-    }
+    v->type_number(v, obj, name, errp);
 }
 
 void output_type_enum(Visitor *v, int *obj, const char *strings[],
@@ -299,13 +268,15 @@ void input_type_enum(Visitor *v, int *obj, const char *strings[],
                      const char *kind, const char *name,
                      Error **errp)
 {
+    Error *local_err = NULL;
     int64_t value = 0;
     char *enum_str;
 
     assert(strings);
 
-    visit_type_str(v, &enum_str, name, errp);
-    if (error_is_set(errp)) {
+    visit_type_str(v, &enum_str, name, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
         return;
     }
 
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index dabd6c7..e7e8a30 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -12,6 +12,7 @@
 
 from ordereddict import OrderedDict
 from qapi import *
+import re
 import sys
 import os
 import getopt
@@ -37,6 +38,15 @@ def generate_command_decl(name, args, ret_type):
 ''',
                  ret_type=c_type(ret_type), name=c_fun(name), args=arglist).strip()
 
+def gen_err_check(errvar):
+    if errvar:
+        return mcgen('''
+if (local_err) {
+    goto out;
+}
+''')
+    return ''
+
 def gen_sync_call(name, args, ret_type, indent=0):
     ret = ""
     arglist=""
@@ -49,15 +59,14 @@ def gen_sync_call(name, args, ret_type, indent=0):
         arglist += "%s, " % (c_var(argname))
     push_indent(indent)
     ret = mcgen('''
-%(retval)sqmp_%(name)s(%(args)serrp);
+%(retval)sqmp_%(name)s(%(args)s&local_err);
 
 ''',
                 name=c_fun(name), args=arglist, retval=retval).rstrip()
     if ret_type:
+        ret += "\n" + gen_err_check('local_err')
         ret += "\n" + mcgen(''''
-if (!error_is_set(errp)) {
-    %(marshal_output_call)s
-}
+%(marshal_output_call)s
 ''',
                             marshal_output_call=gen_marshal_output_call(name, ret_type)).rstrip()
     pop_indent(indent)
@@ -67,7 +76,7 @@ if (!error_is_set(errp)) {
 def gen_marshal_output_call(name, ret_type):
     if not ret_type:
         return ""
-    return "qmp_marshal_output_%s(retval, ret, errp);" % c_fun(name)
+    return "qmp_marshal_output_%s(retval, ret, &local_err);" % c_fun(name)
 
 def gen_visitor_input_containers_decl(args, obj):
     ret = ""
@@ -109,7 +118,8 @@ bool has_%(argname)s = false;
 
 def gen_visitor_input_block(args, dealloc=False):
     ret = ""
-    errparg = 'errp'
+    errparg = '&local_err'
+    errarg = 'local_err'
 
     if len(args) == 0:
         return ret
@@ -118,6 +128,7 @@ def gen_visitor_input_block(args, dealloc=False):
 
     if dealloc:
         errparg = 'NULL'
+        errarg = None;
         ret += mcgen('''
 qmp_input_visitor_cleanup(mi);
 md = qapi_dealloc_visitor_new();
@@ -132,15 +143,20 @@ v = qmp_input_get_visitor(mi);
         if optional:
             ret += mcgen('''
 visit_optional(v, &has_%(c_name)s, "%(name)s", %(errp)s);
-if (has_%(c_name)s) {
 ''',
                          c_name=c_var(argname), name=argname, errp=errparg)
+            ret += gen_err_check(errarg)
+            ret += mcgen('''
+if (has_%(c_name)s) {
+''',
+                         c_name=c_var(argname))
             push_indent()
         ret += mcgen('''
 %(visitor)s(v, &%(c_name)s, "%(name)s", %(errp)s);
 ''',
                      c_name=c_var(argname), name=argname, argtype=argtype,
                      visitor=type_visitor(argtype), errp=errparg)
+        ret += gen_err_check(errarg)
         if optional:
             pop_indent()
             ret += mcgen('''
@@ -161,15 +177,20 @@ def gen_marshal_output(name, args, ret_type, middle_mode):
     ret = mcgen('''
 static void qmp_marshal_output_%(c_name)s(%(c_ret_type)s ret_in, QObject **ret_out, Error **errp)
 {
+    Error *local_err = NULL;
     QmpOutputVisitor *mo = qmp_output_visitor_new();
     QapiDeallocVisitor *md;
     Visitor *v;
 
     v = qmp_output_get_visitor(mo);
-    %(visitor)s(v, &ret_in, "unused", errp);
-    if (!error_is_set(errp)) {
-        *ret_out = qmp_output_get_qobject(mo);
+    %(visitor)s(v, &ret_in, "unused", &local_err);
+    if (local_err) {
+        goto out;
     }
+    *ret_out = qmp_output_get_qobject(mo);
+
+out:
+    error_propagate(errp, local_err);
     qmp_output_visitor_cleanup(mo);
     md = qapi_dealloc_visitor_new();
     v = qapi_dealloc_get_visitor(md);
@@ -196,13 +217,12 @@ def gen_marshal_input(name, args, ret_type, middle_mode):
     ret = mcgen('''
 %(header)s
 {
+    Error *local_err = NULL;
 ''',
                 header=hdr)
 
     if middle_mode:
         ret += mcgen('''
-    Error *local_err = NULL;
-    Error **errp = &local_err;
     QDict *args = (QDict *)qdict;
 ''')
 
@@ -229,20 +249,23 @@ def gen_marshal_input(name, args, ret_type, middle_mode):
                      visitor_input_block=gen_visitor_input_block(args))
     else:
         ret += mcgen('''
+
     (void)args;
 ''')
 
     ret += mcgen('''
-    if (error_is_set(errp)) {
-        goto out;
-    }
 %(sync_call)s
 ''',
                  sync_call=gen_sync_call(name, args, ret_type, indent=4))
-    ret += mcgen('''
+    if re.search('^ *goto out\\;', ret, re.MULTILINE):
+        ret += mcgen('''
 
 out:
 ''')
+    if not middle_mode:
+        ret += mcgen('''
+    error_propagate(errp, local_err);
+''')
     ret += mcgen('''
 %(visitor_input_block_cleanup)s
 ''',
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 3e161bf..a64f1f6 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -12,6 +12,7 @@
 
 from ordereddict import OrderedDict
 from qapi import *
+import re
 import sys
 import os
 import getopt
@@ -32,9 +33,7 @@ static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error *
 
     visit_start_implicit_struct(m, (void **)obj, sizeof(%(c_type)s), &err);
     if (!err) {
-        visit_type_%(c_type)s_fields(m, obj, &err);
-        error_propagate(errp, err);
-        err = NULL;
+        visit_type_%(c_type)s_fields(m, obj, errp);
         visit_end_implicit_struct(m, &err);
     }
     error_propagate(errp, err);
@@ -64,12 +63,9 @@ def generate_visit_struct_fields(name, field_prefix, fn_prefix, members, base =
 
 static void visit_type_%(full_name)s_field_%(c_name)s(Visitor *m, %(name)s **obj, Error **errp)
 {
-    Error *err = NULL;
 ''',
                          name=name, full_name=full_name, c_name=c_var(argname))
-            push_indent()
             ret += generate_visit_struct_body(full_name, argname, argentry)
-            pop_indent()
             ret += mcgen('''
 }
 ''')
@@ -89,6 +85,9 @@ static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error *
     if base:
         ret += mcgen('''
 visit_type_implicit_%(type)s(m, &(*obj)->%(c_prefix)s%(c_name)s, &err);
+if (err) {
+    goto out;
+}
 ''',
                      c_prefix=c_var(field_prefix),
                      type=type_name(base), c_name=c_var('base'))
@@ -97,7 +96,7 @@ visit_type_implicit_%(type)s(m, &(*obj)->%(c_prefix)s%(c_name)s, &err);
         if optional:
             ret += mcgen('''
 visit_optional(m, &(*obj)->%(c_prefix)shas_%(c_name)s, "%(name)s", &err);
-if ((*obj)->%(prefix)shas_%(c_name)s) {
+if (!err && (*obj)->%(prefix)shas_%(c_name)s) {
 ''',
                          c_prefix=c_var(field_prefix), prefix=field_prefix,
                          c_name=c_var(argname), name=argname)
@@ -121,10 +120,19 @@ visit_type_%(type)s(m, &(*obj)->%(c_prefix)s%(c_name)s, "%(name)s", &err);
             ret += mcgen('''
 }
 ''')
+        ret += mcgen('''
+if (err) {
+    goto out;
+}
+''')
 
     pop_indent()
-    ret += mcgen('''
+    if re.search('^ *goto out\\;', ret, re.MULTILINE):
+        ret += mcgen('''
 
+out:
+''')
+    ret += mcgen('''
     error_propagate(errp, err);
 }
 ''')
@@ -133,9 +141,9 @@ visit_type_%(type)s(m, &(*obj)->%(c_prefix)s%(c_name)s, "%(name)s", &err);
 
 def generate_visit_struct_body(field_prefix, name, members):
     ret = mcgen('''
-if (!error_is_set(errp)) {
+    Error *err = NULL;
+
 ''')
-    push_indent()
 
     if not field_prefix:
         full_name = name
@@ -144,36 +152,26 @@ if (!error_is_set(errp)) {
 
     if len(field_prefix):
         ret += mcgen('''
-visit_start_struct(m, NULL, "", "%(name)s", 0, &err);
+    visit_start_struct(m, NULL, "", "%(name)s", 0, &err);
 ''',
                 name=name)
     else:
         ret += mcgen('''
-Error *err = NULL;
-visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
+    visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
 ''',
                 name=name)
 
     ret += mcgen('''
-if (!err) {
-    if (*obj) {
-        visit_type_%(name)s_fields(m, obj, &err);
-        error_propagate(errp, err);
-        err = NULL;
+    if (!err) {
+        if (*obj) {
+            visit_type_%(name)s_fields(m, obj, errp);
+        }
+        visit_end_struct(m, &err);
     }
+    error_propagate(errp, err);
 ''',
         name=full_name)
 
-    ret += mcgen('''
-    /* Always call end_struct if start_struct succeeded.  */
-    visit_end_struct(m, &err);
-}
-error_propagate(errp, err);
-''')
-    pop_indent()
-    ret += mcgen('''
-}
-''')
     return ret
 
 def generate_visit_struct(expr):
@@ -191,9 +189,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
 ''',
                 name=name)
 
-    push_indent()
     ret += generate_visit_struct_body("", name, members)
-    pop_indent()
 
     ret += mcgen('''
 }
@@ -205,24 +201,26 @@ def generate_visit_list(name, members):
 
 void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp)
 {
-    GenericList *i, **prev = (GenericList **)obj;
     Error *err = NULL;
+    GenericList *i, **prev;
 
-    if (!error_is_set(errp)) {
-        visit_start_list(m, name, &err);
-        if (!err) {
-            for (; (i = visit_next_list(m, prev, &err)) != NULL; prev = &i) {
-                %(name)sList *native_i = (%(name)sList *)i;
-                visit_type_%(name)s(m, &native_i->value, NULL, &err);
-            }
-            error_propagate(errp, err);
-            err = NULL;
-
-            /* Always call end_list if start_list succeeded.  */
-            visit_end_list(m, &err);
-        }
-        error_propagate(errp, err);
+    visit_start_list(m, name, &err);
+    if (err) {
+        goto out;
+    }
+
+    for (prev = (GenericList **)obj;
+         !err && (i = visit_next_list(m, prev, &err)) != NULL;
+         prev = &i) {
+        %(name)sList *native_i = (%(name)sList *)i;
+        visit_type_%(name)s(m, &native_i->value, NULL, &err);
     }
+
+    error_propagate(errp, err);
+    err = NULL;
+    visit_end_list(m, &err);
+out:
+    error_propagate(errp, err);
 }
 ''',
                 name=name)
@@ -244,10 +242,15 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
 {
     Error *err = NULL;
 
-    if (!error_is_set(errp)) {
-        visit_start_implicit_struct(m, (void**) obj, sizeof(%(name)s), &err);
-        visit_get_next_type(m, (int*) &(*obj)->kind, %(name)s_qtypes, name, &err);
-        switch ((*obj)->kind) {
+    visit_start_implicit_struct(m, (void**) obj, sizeof(%(name)s), &err);
+    if (err) {
+        goto out;
+    }
+    visit_get_next_type(m, (int*) &(*obj)->kind, %(name)s_qtypes, name, &err);
+    if (err) {
+        goto out_end;
+    }
+    switch ((*obj)->kind) {
 ''',
     name=name)
 
@@ -262,22 +265,24 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
 
         enum_full_value = generate_enum_full_value(disc_type, key)
         ret += mcgen('''
-        case %(enum_full_value)s:
-            visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, name, &err);
-            break;
+    case %(enum_full_value)s:
+        visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, name, &err);
+        break;
 ''',
                 enum_full_value = enum_full_value,
                 c_type = type_name(members[key]),
                 c_name = c_fun(key))
 
     ret += mcgen('''
-        default:
-            abort();
-        }
-        error_propagate(errp, err);
-        err = NULL;
-        visit_end_implicit_struct(m, &err);
+    default:
+        abort();
     }
+out_end:
+    error_propagate(errp, err);
+    err = NULL;
+    visit_end_implicit_struct(m, &err);
+out:
+    error_propagate(errp, err);
 }
 ''')
 
@@ -324,19 +329,20 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
 {
     Error *err = NULL;
 
-    if (!error_is_set(errp)) {
-        visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
-        if (!err) {
-            if (*obj) {
+    visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
+    if (err) {
+        goto out;
+    }
+    if (*obj) {
 ''',
                  name=name)
 
-    push_indent()
-    push_indent()
-
     if base:
         ret += mcgen('''
         visit_type_%(name)s_fields(m, obj, &err);
+        if (err) {
+            goto out_obj;
+        }
 ''',
             name=name)
 
@@ -346,8 +352,10 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
         disc_key = discriminator
     ret += mcgen('''
         visit_type_%(disc_type)s(m, &(*obj)->kind, "%(disc_key)s", &err);
-        if (!err) {
-            switch ((*obj)->kind) {
+        if (err) {
+            goto out_obj;
+        }
+        switch ((*obj)->kind) {
 ''',
                  disc_type = disc_type,
                  disc_key = disc_key)
@@ -360,32 +368,25 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
 
         enum_full_value = generate_enum_full_value(disc_type, key)
         ret += mcgen('''
-            case %(enum_full_value)s:
-                ''' + fmt + '''
-                break;
+        case %(enum_full_value)s:
+            ''' + fmt + '''
+            break;
 ''',
                 enum_full_value = enum_full_value,
                 c_type=type_name(members[key]),
                 c_name=c_fun(key))
 
     ret += mcgen('''
-            default:
-                abort();
-            }
+        default:
+            abort();
         }
+out_obj:
         error_propagate(errp, err);
         err = NULL;
-''')
-    pop_indent()
-    pop_indent()
-
-    ret += mcgen('''
-            }
-            /* Always call end_struct if start_struct succeeded.  */
-            visit_end_struct(m, &err);
-        }
-        error_propagate(errp, err);
     }
+    visit_end_struct(m, &err);
+out:
+    error_propagate(errp, err);
 }
 ''')
 
@@ -502,7 +503,7 @@ fdecl.write(mcgen('''
 /* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */
 
 /*
- * schema-defined QAPI visitor function
+ * schema-defined QAPI visitor functions
  *
  * Copyright IBM, Corp. 2011
  *
diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
index ec798c2..0f77003 100644
--- a/tests/test-qmp-input-strict.c
+++ b/tests/test-qmp-input-strict.c
@@ -81,11 +81,19 @@ static void visit_type_TestStruct(Visitor *v, TestStruct **obj,
     }
 
     visit_type_int(v, &(*obj)->integer, "integer", &err);
+    if (err) {
+        goto out_end;
+    }
     visit_type_bool(v, &(*obj)->boolean, "boolean", &err);
+    if (err) {
+        goto out_end;
+    }
     visit_type_str(v, &(*obj)->string, "string", &err);
 
+out_end:
+    error_propagate(errp, err);
+    err = NULL;
     visit_end_struct(v, &err);
-
 out:
     error_propagate(errp, err);
 }
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index a58a3e6..1c8e872 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -199,16 +199,24 @@ static void visit_type_TestStruct(Visitor *v, TestStruct **obj,
 
     visit_start_struct(v, (void **)obj, "TestStruct", name, sizeof(TestStruct),
                        &err);
-    if (!err) {
-        visit_type_int(v, &(*obj)->integer, "integer", &err);
-        visit_type_bool(v, &(*obj)->boolean, "boolean", &err);
-        visit_type_str(v, &(*obj)->string, "string", &err);
-
-        /* Always call end_struct if start_struct succeeded.  */
-        error_propagate(errp, err);
-        err = NULL;
-        visit_end_struct(v, &err);
+    if (err) {
+        goto out;
     }
+    visit_type_int(v, &(*obj)->integer, "integer", &err);
+    if (err) {
+        goto out_end;
+    }
+    visit_type_bool(v, &(*obj)->boolean, "boolean", &err);
+    if (err) {
+        goto out_end;
+    }
+    visit_type_str(v, &(*obj)->string, "string", &err);
+
+out_end:
+    error_propagate(errp, err);
+    err = NULL;
+    visit_end_struct(v, &err);
+out:
     error_propagate(errp, err);
 }
 
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index dfd597c..9c15458 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -185,11 +185,19 @@ static void visit_type_TestStruct(Visitor *v, TestStruct **obj,
     }
 
     visit_type_int(v, &(*obj)->integer, "integer", &err);
+    if (err) {
+        goto out_end;
+    }
     visit_type_bool(v, &(*obj)->boolean, "boolean", &err);
+    if (err) {
+        goto out_end;
+    }
     visit_type_str(v, &(*obj)->string, "string", &err);
 
+out_end:
+    error_propagate(errp, err);
+    err = NULL;
     visit_end_struct(v, &err);
-
 out:
     error_propagate(errp, err);
 }
diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c
index 85170e5..74d6481 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -195,7 +195,7 @@ typedef struct TestStruct
 static void visit_type_TestStruct(Visitor *v, TestStruct **obj,
                                   const char *name, Error **errp)
 {
-    Error *err= NULL;
+    Error *err = NULL;
 
     visit_start_struct(v, (void **)obj, NULL, name, sizeof(TestStruct), &err);
     if (err) {
@@ -203,11 +203,19 @@ static void visit_type_TestStruct(Visitor *v, TestStruct **obj,
     }
 
     visit_type_int(v, &(*obj)->integer, "integer", &err);
+    if (err) {
+        goto out_end;
+    }
     visit_type_bool(v, &(*obj)->boolean, "boolean", &err);
+    if (err) {
+        goto out_end;
+    }
     visit_type_str(v, &(*obj)->string, "string", &err);
 
+out_end:
+    error_propagate(errp, err);
+    err = NULL;
     visit_end_struct(v, &err);
-
 out:
     error_propagate(errp, err);
 }
-- 
1.8.1.4