[Qemu-devel] [PATCH 01/16] qapi: fix error propagation

[Laszlo Ersek <lersek@redhat.com>]

From: Paolo Bonzini <pbonzini@redhat.com>

Don't overwrite / leak previously set errors.
Don't try to end a container that could not be started.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 error.h                        |    4 +-
 error.c                        |    4 +-
 qapi/qapi-visit-core.c         |   10 +--
 tests/test-qmp-input-visitor.c |   24 +++++---
 docs/qapi-code-gen.txt         |    2 +
 scripts/qapi-visit.py          |  129 +++++++++++++++++++++++----------------
 6 files changed, 102 insertions(+), 71 deletions(-)

diff --git a/error.h b/error.h
index 45ff6c1..6898f84 100644
--- a/error.h
+++ b/error.h
@@ -24,7 +24,7 @@ typedef struct Error Error;
 /**
  * Set an indirect pointer to an error given a printf-style format parameter.
  * Currently, qerror.h defines these error formats.  This function is not
- * meant to be used outside of QEMU.
+ * meant to be used outside of QEMU.  Errors after the first are discarded.
  */
 void error_set(Error **err, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
 
@@ -57,7 +57,7 @@ void error_set_field(Error *err, const char *field, const char *value);
 /**
  * Propagate an error to an indirect pointer to an error.  This function will
  * always transfer ownership of the error reference and handles the case where
- * dst_err is NULL correctly.
+ * dst_err is NULL correctly.  Errors after the first are discarded.
  */
 void error_propagate(Error **dst_err, Error *local_err);
 
diff --git a/error.c b/error.c
index a52b771..0177972 100644
--- a/error.c
+++ b/error.c
@@ -29,7 +29,7 @@ void error_set(Error **errp, const char *fmt, ...)
     Error *err;
     va_list ap;
 
-    if (errp == NULL) {
+    if (errp == NULL || *errp != NULL) {
         return;
     }
 
@@ -132,7 +132,7 @@ bool error_is_type(Error *err, const char *fmt)
 
 void error_propagate(Error **dst_err, Error *local_err)
 {
-    if (dst_err) {
+    if (dst_err && !*dst_err) {
         *dst_err = local_err;
     } else if (local_err) {
         error_free(local_err);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index a4e088c..df1ed5c 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -39,9 +39,8 @@ void visit_start_struct(Visitor *v, void **obj, const char *kind,
 
 void visit_end_struct(Visitor *v, Error **errp)
 {
-    if (!error_is_set(errp)) {
-        v->end_struct(v, errp);
-    }
+    assert(!error_is_set(errp));
+    v->end_struct(v, errp);
 }
 
 void visit_start_list(Visitor *v, const char *name, Error **errp)
@@ -62,9 +61,8 @@ GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp)
 
 void visit_end_list(Visitor *v, Error **errp)
 {
-    if (!error_is_set(errp)) {
-        v->end_list(v, errp);
-    }
+    assert(!error_is_set(errp));
+    v->end_list(v, errp);
 }
 
 void visit_start_optional(Visitor *v, bool *present, const char *name,
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index c30fdc4..8f5a509 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -151,14 +151,22 @@ typedef struct TestStruct
 static void visit_type_TestStruct(Visitor *v, TestStruct **obj,
                                   const char *name, Error **errp)
 {
-    visit_start_struct(v, (void **)obj, "TestStruct", name, sizeof(TestStruct),
-                       errp);
-
-    visit_type_int(v, &(*obj)->integer, "integer", errp);
-    visit_type_bool(v, &(*obj)->boolean, "boolean", errp);
-    visit_type_str(v, &(*obj)->string, "string", errp);
-
-    visit_end_struct(v, errp);
+    Error *err = NULL;
+    if (!error_is_set(errp)) {
+        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);
+        }
+        error_propagate(errp, err);
+    }
 }
 
 static void test_visitor_in_struct(TestInputVisitorData *data,
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index ad11767..cccb11e 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -220,6 +220,8 @@ Example:
     #endif
     mdroth@illuin:~/w/qemu2.git$
 
+(The actual structure of the visit_type_* functions is a bit more complex
+in order to propagate errors correctly and avoid leaking memory).
 
 === scripts/qapi-commands.py ===
 
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 8d4e94a..61cf586 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -17,14 +17,37 @@ import os
 import getopt
 import errno
 
-def generate_visit_struct_body(field_prefix, members):
-    ret = ""
+def generate_visit_struct_body(field_prefix, name, members):
+    ret = mcgen('''
+if (!error_is_set(errp)) {
+''')
+    push_indent()
+
     if len(field_prefix):
         field_prefix = field_prefix + "."
+        ret += mcgen('''
+Error **errp = &err; /* from outer scope */
+Error *err = NULL;
+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);
+''',
+                name=name)
+
+    ret += mcgen('''
+if (!err) {
+    assert(!obj || *obj);
+''')
+
+    push_indent()
     for argname, argentry, optional, structured in parse_args(members):
         if optional:
             ret += mcgen('''
-visit_start_optional(m, (obj && *obj) ? &(*obj)->%(c_prefix)shas_%(c_name)s : NULL, "%(name)s", errp);
+visit_start_optional(m, obj ? &(*obj)->%(c_prefix)shas_%(c_name)s : NULL, "%(name)s", &err);
 if ((*obj)->%(prefix)shas_%(c_name)s) {
 ''',
                          c_prefix=c_var(field_prefix), prefix=field_prefix,
@@ -32,17 +55,10 @@ if ((*obj)->%(prefix)shas_%(c_name)s) {
             push_indent()
 
         if structured:
-            ret += mcgen('''
-visit_start_struct(m, NULL, "", "%(name)s", 0, errp);
-''',
-                         name=argname)
-            ret += generate_visit_struct_body(field_prefix + argname, argentry)
-            ret += mcgen('''
-visit_end_struct(m, errp);
-''')
+            ret += generate_visit_struct_body(field_prefix + argname, argname, argentry)
         else:
             ret += mcgen('''
-visit_type_%(type)s(m, (obj && *obj) ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s", errp);
+visit_type_%(type)s(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s", &err);
 ''',
                          c_prefix=c_var(field_prefix), prefix=field_prefix,
                          type=type_name(argentry), c_name=c_var(argname),
@@ -52,7 +68,19 @@ visit_type_%(type)s(m, (obj && *obj) ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "
             pop_indent()
             ret += mcgen('''
 }
-visit_end_optional(m, errp);
+visit_end_optional(m, &err);
+''')
+
+    pop_indent()
+    pop_indent()
+    ret += mcgen('''
+        /* Always call end_struct if start_struct succeeded.  */
+        error_propagate(errp, err);
+        err = NULL;
+        visit_end_struct(m, &err);
+    }
+    error_propagate(errp, err);
+}
 ''')
     return ret
 
@@ -61,22 +89,14 @@ def generate_visit_struct(name, members):
 
 void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
 {
-    if (error_is_set(errp)) {
-        return;
-    }
-    visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), errp);
-    if (obj && !*obj) {
-        goto end;
-    }
 ''',
                 name=name)
+
     push_indent()
-    ret += generate_visit_struct_body("", members)
+    ret += generate_visit_struct_body("", name, members)
     pop_indent()
 
     ret += mcgen('''
-end:
-    visit_end_struct(m, errp);
 }
 ''')
     return ret
@@ -87,18 +107,22 @@ 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;
 
-    if (error_is_set(errp)) {
-        return;
-    }
-    visit_start_list(m, name, errp);
-
-    for (; (i = visit_next_list(m, prev, errp)) != NULL; prev = &i) {
-        %(name)sList *native_i = (%(name)sList *)i;
-        visit_type_%(name)s(m, &native_i->value, NULL, errp);
+    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);
+            }
+            /* Always call end_list if start_list succeeded.  */
+            error_propagate(errp, err);
+            err = NULL;
+            visit_end_list(m, &err);
+        }
+        error_propagate(errp, err);
     }
-
-    visit_end_list(m, errp);
 }
 ''',
                 name=name)
@@ -122,27 +146,21 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
 {
     Error *err = NULL;
 
-    if (error_is_set(errp)) {
-        return;
-    }
-    visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
-    if (obj && !*obj) {
-        goto end;
-    }
-    visit_type_%(name)sKind(m, &(*obj)->kind, "type", &err);
-    if (err) {
-        error_propagate(errp, err);
-        goto end;
-    }
-    switch ((*obj)->kind) {
+    if (!error_is_set(errp)) {
+        visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
+        if (!err) {
+            visit_type_%(name)sKind(m, &(*obj)->kind, "type", &err);
+        }
+        if (!err) {
+            switch ((*obj)->kind) {
 ''',
                  name=name)
 
     for key in members:
         ret += mcgen('''
-    case %(abbrev)s_KIND_%(enum)s:
-        visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", errp);
-        break;
+            case %(abbrev)s_KIND_%(enum)s:
+                visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", &err);
+                break;
 ''',
                 abbrev = de_camel_case(name).upper(),
                 enum = c_fun(de_camel_case(key)).upper(),
@@ -150,11 +168,16 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
                 c_name=c_fun(key))
 
     ret += mcgen('''
-    default:
-        abort();
+            default:
+                abort();
+            }
+        }
+        /* Always call end_struct if start_struct succeeded.  */
+        error_propagate(errp, err);
+        err = NULL;
+        visit_end_struct(m, &err);
     }
-end:
-    visit_end_struct(m, errp);
+    error_propagate(errp, err);
 }
 ''')
 
-- 
1.7.1