[Qemu-devel] [PATCH 00/13] qapi: Purge error_is_set()

[Qemu-devel] [PATCH 00/13] qapi: Purge error_is_set()

[Markus Armbruster]

This is the sixth and final part, covering QAPI and its users.

PATCH 01-08 are preparatory cleanups.

PATCH 09-11 fix a misuse of the visitor API in hand-written code.
Generated code uses the API correctly.

PATCH 12 converts QAPI and its users to the common use of the error
API, purging error_is_set() along the way.

PATCH 13 drops error_is_set().  This depends on all five prior parts
of the purge, of which only the first two have been committed already.

If you get undefined references to error_is_set() with this series
applied, either prior parts of the purge haven't been applied, or new
uses have crept in.  Drop just the last patch then.

My series conflicts with Lluís's "qapi: Allow modularization of QAPI
schema files" and Amos's "qapi: fix coding style in generated code",
but the conflicts are trivial, and 3-way merge can take care of them.

Luiz, would you be willing to take this through your tree as well?

Markus Armbruster (13):
  qapi: Update qapi-code-gen.txt example to match current code
  qapi: Normalize marshalling's visitor initialization and cleanup
  qapi: Remove unused Visitor callbacks start_handle(), end_handle()
  qapi: Replace start_optional()/end_optional() by optional()
  qapi-visit.py: Clean up confusing push_indent() / pop_indent() use
  qapi: Clean up shadowing of parameters and locals in inner scopes
  qapi-visit.py: Clean up a sloppy use of field prefix
  qapi: Un-inline visit of implicit struct
  hmp: Call visit_end_struct() after visit_start_struct() succeeds
  hw: Don't call visit_end_struct() after visit_start_struct() fails
  tests: Don't call visit_end_struct() after visit_start_struct() fails
  qapi: Replace uncommon use of the error API by the common one
  error: error_is_set() is finally unused; remove

 docs/qapi-code-gen.txt             | 165 ++++++++++++++---------
 hmp.c                              |  16 +--
 hw/timer/mc146818rtc.c             |  41 +++++-
 hw/virtio/virtio-balloon.c         |  33 +++--
 include/qapi/error.h               |   6 -
 include/qapi/visitor-impl.h        |   8 +-
 include/qapi/visitor.h             |   5 +-
 qapi/opts-visitor.c                |   5 +-
 qapi/qapi-visit-core.c             | 259 +++++++++++++++----------------------
 qapi/qmp-input-visitor.c           |   6 +-
 qapi/string-input-visitor.c        |   6 +-
 scripts/qapi-commands.py           |  87 ++++++++-----
 scripts/qapi-visit.py              | 230 ++++++++++++++++++--------------
 tests/test-qmp-input-strict.c      |  28 +++-
 tests/test-qmp-input-visitor.c     |  26 ++--
 tests/test-qmp-output-visitor.c    |  28 +++-
 tests/test-visitor-serialization.c |  26 +++-
 util/error.c                       |   5 -
 18 files changed, 554 insertions(+), 426 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 01/13] qapi: Update qapi-code-gen.txt example to match current code

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/qapi-code-gen.txt | 146 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 90 insertions(+), 56 deletions(-)

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index d78921f..923565e 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -223,11 +223,23 @@ Example:
     mdroth@illuin:~/w/qemu2.git$ python scripts/qapi-types.py \
       --output-dir="qapi-generated" --prefix="example-" < example-schema.json
     mdroth@illuin:~/w/qemu2.git$ cat qapi-generated/example-qapi-types.c
-    /* AUTOMATICALLY GENERATED, DO NOT MODIFY */
+[Uninteresting stuff omitted...]
+
+    void qapi_free_UserDefOneList(UserDefOneList * obj)
+    {
+        QapiDeallocVisitor *md;
+        Visitor *v;
+
+        if (!obj) {
+            return;
+        }
+
+        md = qapi_dealloc_visitor_new();
+        v = qapi_dealloc_get_visitor(md);
+        visit_type_UserDefOneList(v, &obj, NULL, NULL);
+        qapi_dealloc_visitor_cleanup(md);
+    }
 
-    #include "qapi/qapi-dealloc-visitor.h"
-    #include "example-qapi-types.h"
-    #include "example-qapi-visit.h"
 
     void qapi_free_UserDefOne(UserDefOne * obj)
     {
@@ -245,31 +257,37 @@ Example:
     }
 
     mdroth@illuin:~/w/qemu2.git$ cat qapi-generated/example-qapi-types.h
-    /* AUTOMATICALLY GENERATED, DO NOT MODIFY */
-    #ifndef QAPI_GENERATED_EXAMPLE_QAPI_TYPES
-    #define QAPI_GENERATED_EXAMPLE_QAPI_TYPES
+[Uninteresting stuff omitted...]
+
+    #ifndef EXAMPLE_QAPI_TYPES_H
+    #define EXAMPLE_QAPI_TYPES_H
 
-    #include "qapi/qapi-types-core.h"
+[Builtin types omitted...]
 
     typedef struct UserDefOne UserDefOne;
 
     typedef struct UserDefOneList
     {
-        UserDefOne *value;
+        union {
+            UserDefOne *value;
+            uint64_t padding;
+        };
         struct UserDefOneList *next;
     } UserDefOneList;
 
+[Functions on builtin types omitted...]
+
     struct UserDefOne
     {
         int64_t integer;
         char * string;
     };
 
+    void qapi_free_UserDefOneList(UserDefOneList * obj);
     void qapi_free_UserDefOne(UserDefOne * obj);
 
     #endif
 
-
 === scripts/qapi-visit.py ===
 
 Used to generate the visitor functions used to walk through and convert
@@ -293,39 +311,63 @@ Example:
     mdroth@illuin:~/w/qemu2.git$ python scripts/qapi-visit.py \
         --output-dir="qapi-generated" --prefix="example-" < example-schema.json
     mdroth@illuin:~/w/qemu2.git$ cat qapi-generated/example-qapi-visit.c
-    /* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */
+[Uninteresting stuff omitted...]
 
-    #include "example-qapi-visit.h"
+    static void visit_type_UserDefOne_fields(Visitor *m, UserDefOne ** obj, Error **errp)
+    {
+        Error *err = NULL;
+        visit_type_int(m, &(*obj)->integer, "integer", &err);
+        visit_type_str(m, &(*obj)->string, "string", &err);
+
+        error_propagate(errp, err);
+    }
 
     void visit_type_UserDefOne(Visitor *m, UserDefOne ** obj, const char *name, Error **errp)
     {
-        visit_start_struct(m, (void **)obj, "UserDefOne", name, sizeof(UserDefOne), errp);
-        visit_type_int(m, (obj && *obj) ? &(*obj)->integer : NULL, "integer", errp);
-        visit_type_str(m, (obj && *obj) ? &(*obj)->string : NULL, "string", errp);
-        visit_end_struct(m, 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_propagate(errp, err);
+        }
     }
 
     void visit_type_UserDefOneList(Visitor *m, UserDefOneList ** obj, const char *name, Error **errp)
     {
         GenericList *i, **prev = (GenericList **)obj;
-
-        visit_start_list(m, name, errp);
-
-        for (; (i = visit_next_list(m, prev, errp)) != NULL; prev = &i) {
-            UserDefOneList *native_i = (UserDefOneList *)i;
-            visit_type_UserDefOne(m, &native_i->value, NULL, errp);
+        Error *err = NULL;
+
+        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_end_list(m, errp);
     }
     mdroth@illuin:~/w/qemu2.git$ cat qapi-generated/example-qapi-visit.h
-    /* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */
+[Uninteresting stuff omitted...]
 
-    #ifndef QAPI_GENERATED_EXAMPLE_QAPI_VISIT
-    #define QAPI_GENERATED_EXAMPLE_QAPI_VISIT
+    #ifndef EXAMPLE_QAPI_VISIT_H
+    #define EXAMPLE_QAPI_VISIT_H
 
-    #include "qapi/qapi-visit-core.h"
-    #include "example-qapi-types.h"
+[Visitors for builtin types omitted...]
 
     void visit_type_UserDefOne(Visitor *m, UserDefOne ** obj, const char *name, Error **errp);
     void visit_type_UserDefOneList(Visitor *m, UserDefOneList ** obj, const char *name, Error **errp);
@@ -333,9 +375,6 @@ 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 ===
 
 Used to generate the marshaling/dispatch functions for the commands defined
@@ -356,18 +395,8 @@ $(prefix)qmp-commands.h: Function prototypes for the QMP commands
 Example:
 
     mdroth@illuin:~/w/qemu2.git$ cat qapi-generated/example-qmp-marshal.c
-    /* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */
-
-    #include "qemu-objects.h"
-    #include "qapi/qmp-core.h"
-    #include "qapi/qapi-visit-core.h"
-    #include "qapi/qmp-output-visitor.h"
-    #include "qapi/qmp-input-visitor.h"
-    #include "qapi/qapi-dealloc-visitor.h"
-    #include "example-qapi-types.h"
-    #include "example-qapi-visit.h"
+[Uninteresting stuff omitted...]
 
-    #include "example-qmp-commands.h"
     static void qmp_marshal_output_my_command(UserDefOne * ret_in, QObject **ret_out, Error **errp)
     {
         QapiDeallocVisitor *md = qapi_dealloc_visitor_new();
@@ -376,15 +405,16 @@ Example:
 
         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);
+        }
+        qmp_output_visitor_cleanup(mo);
         v = qapi_dealloc_get_visitor(md);
-        visit_type_UserDefOne(v, &ret_in, "unused", errp);
+        visit_type_UserDefOne(v, &ret_in, "unused", NULL);
         qapi_dealloc_visitor_cleanup(md);
-
-
-        *ret_out = qmp_output_get_qobject(mo);
     }
 
-    static void qmp_marshal_input_my_command(QmpState *qmp__sess, QDict *args, QObject **ret, Error **errp)
+    static void qmp_marshal_input_my_command(QDict *args, QObject **ret, Error **errp)
     {
         UserDefOne * retval = NULL;
         QmpInputVisitor *mi;
@@ -392,38 +422,42 @@ Example:
         Visitor *v;
         UserDefOne * arg1 = NULL;
 
-        mi = qmp_input_visitor_new(QOBJECT(args));
+        mi = qmp_input_visitor_new_strict(QOBJECT(args));
         v = qmp_input_get_visitor(mi);
         visit_type_UserDefOne(v, &arg1, "arg1", errp);
+        qmp_input_visitor_cleanup(mi);
 
         if (error_is_set(errp)) {
             goto out;
         }
         retval = qmp_my_command(arg1, errp);
-        qmp_marshal_output_my_command(retval, ret, errp);
+        if (!error_is_set(errp)) {
+            qmp_marshal_output_my_command(retval, ret, errp);
+        }
 
     out:
         md = qapi_dealloc_visitor_new();
         v = qapi_dealloc_get_visitor(md);
-        visit_type_UserDefOne(v, &arg1, "arg1", errp);
+        visit_type_UserDefOne(v, &arg1, "arg1", NULL);
         qapi_dealloc_visitor_cleanup(md);
         return;
     }
 
     static void qmp_init_marshal(void)
     {
-        qmp_register_command("my-command", qmp_marshal_input_my_command);
+        qmp_register_command("my-command", qmp_marshal_input_my_command, QCO_NO_OPTIONS);
     }
 
     qapi_init(qmp_init_marshal);
     mdroth@illuin:~/w/qemu2.git$ cat qapi-generated/example-qmp-commands.h
-    /* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */
+[Uninteresting stuff omitted...]
 
-    #ifndef QAPI_GENERATED_EXAMPLE_QMP_COMMANDS
-    #define QAPI_GENERATED_EXAMPLE_QMP_COMMANDS
+    #ifndef EXAMPLE_QMP_COMMANDS_H
+    #define EXAMPLE_QMP_COMMANDS_H
 
     #include "example-qapi-types.h"
-    #include "error.h"
+    #include "qapi/qmp/qdict.h"
+    #include "qapi/error.h"
 
     UserDefOne * qmp_my_command(UserDefOne * arg1, Error **errp);
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 01/13] qapi: Update qapi-code-gen.txt example to match current code

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1130 bytes --]

On 05/02/2014 06:44 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  docs/qapi-code-gen.txt | 146 ++++++++++++++++++++++++++++++-------------------
>  1 file changed, 90 insertions(+), 56 deletions(-)

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

I checked that this matches the current reality, and serves as a good
metric for what the rest of the series cleans up.

>  === scripts/qapi-commands.py ===
>  
>  Used to generate the marshaling/dispatch functions for the commands defined
> @@ -356,18 +395,8 @@ $(prefix)qmp-commands.h: Function prototypes for the QMP commands
>  Example:
>  
>      mdroth@illuin:~/w/qemu2.git$ cat qapi-generated/example-qmp-marshal.c

The file was missing the example command line that generated this file;
I figured it out:

    mdroth@illuin:~/w/qemu2.git$ python scripts/qapi-commands.py \
        --output-dir="qapi-generated" --prefix="example-" <
example-schema.json

if you want to add that to this patch.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 01/13] qapi: Update qapi-code-gen.txt example to match current code

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/02/2014 06:44 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  docs/qapi-code-gen.txt | 146
>> ++++++++++++++++++++++++++++++-------------------
>>  1 file changed, 90 insertions(+), 56 deletions(-)
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> I checked that this matches the current reality, and serves as a good
> metric for what the rest of the series cleans up.
>
>>  === scripts/qapi-commands.py ===
>>  
>>  Used to generate the marshaling/dispatch functions for the commands defined
>> @@ -356,18 +395,8 @@ $(prefix)qmp-commands.h: Function prototypes for the QMP commands
>>  Example:
>>  
>>      mdroth@illuin:~/w/qemu2.git$ cat qapi-generated/example-qmp-marshal.c
>
> The file was missing the example command line that generated this file;
> I figured it out:
>
>     mdroth@illuin:~/w/qemu2.git$ python scripts/qapi-commands.py \
>         --output-dir="qapi-generated" --prefix="example-" <
> example-schema.json
>
> if you want to add that to this patch.

Sure, why not, but if there's nothing else to correct in this series,
I'd prefer a separate commit to a respin.

Re: [Qemu-devel] [PATCH 01/13] qapi: Update qapi-code-gen.txt example to match current code

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 852 bytes --]

On 05/05/2014 12:49 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 

>>>  Example:
>>>  
>>>      mdroth@illuin:~/w/qemu2.git$ cat qapi-generated/example-qmp-marshal.c
>>
>> The file was missing the example command line that generated this file;
>> I figured it out:
>>
>>     mdroth@illuin:~/w/qemu2.git$ python scripts/qapi-commands.py \
>>         --output-dir="qapi-generated" --prefix="example-" <
>> example-schema.json
>>
>> if you want to add that to this patch.
> 
> Sure, why not, but if there's nothing else to correct in this series,
> I'd prefer a separate commit to a respin.

Followup commit is just fine, if I don't turn up anything else while
reviewing the rest of the series.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 01/13] qapi: Update qapi-code-gen.txt example to match current code

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/05/2014 12:49 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>
>>>>  Example:
>>>>  
>>>>      mdroth@illuin:~/w/qemu2.git$ cat qapi-generated/example-qmp-marshal.c
>>>
>>> The file was missing the example command line that generated this file;
>>> I figured it out:
>>>
>>>     mdroth@illuin:~/w/qemu2.git$ python scripts/qapi-commands.py \
>>>         --output-dir="qapi-generated" --prefix="example-" <
>>> example-schema.json
>>>
>>> if you want to add that to this patch.
>> 
>> Sure, why not, but if there's nothing else to correct in this series,
>> I'd prefer a separate commit to a respin.
>
> Followup commit is just fine, if I don't turn up anything else while
> reviewing the rest of the series.

Followup commit. because my fix conflicts with a patch in Luiz's queue.
I'll wait for that to land, then post my fix on top.

[Qemu-devel] [PATCH 02/13] qapi: Normalize marshalling's visitor initialization and cleanup

[Markus Armbruster]

Input and output marshalling functions do it differently.  Change them
to work the same: initialize the I/O visitor, use it, clean it up,
initialize the dealloc visitor, use it, clean it up.

This delays dealloc visitor initialization in output marshalling
functions, and input visitor cleanup in input marshalling functions.
No functional change, but the latter will be convenient when I change
the error handling.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/qapi-code-gen.txt   |  8 ++++----
 scripts/qapi-commands.py | 27 ++++++++++++---------------
 2 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 923565e..ac951ef 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -399,8 +399,8 @@ Example:
 
     static void qmp_marshal_output_my_command(UserDefOne * ret_in, QObject **ret_out, Error **errp)
     {
-        QapiDeallocVisitor *md = qapi_dealloc_visitor_new();
         QmpOutputVisitor *mo = qmp_output_visitor_new();
+        QapiDeallocVisitor *md;
         Visitor *v;
 
         v = qmp_output_get_visitor(mo);
@@ -409,6 +409,7 @@ Example:
             *ret_out = qmp_output_get_qobject(mo);
         }
         qmp_output_visitor_cleanup(mo);
+        md = qapi_dealloc_visitor_new();
         v = qapi_dealloc_get_visitor(md);
         visit_type_UserDefOne(v, &ret_in, "unused", NULL);
         qapi_dealloc_visitor_cleanup(md);
@@ -417,15 +418,13 @@ Example:
     static void qmp_marshal_input_my_command(QDict *args, QObject **ret, Error **errp)
     {
         UserDefOne * retval = NULL;
-        QmpInputVisitor *mi;
+        QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
         QapiDeallocVisitor *md;
         Visitor *v;
         UserDefOne * arg1 = NULL;
 
-        mi = qmp_input_visitor_new_strict(QOBJECT(args));
         v = qmp_input_get_visitor(mi);
         visit_type_UserDefOne(v, &arg1, "arg1", errp);
-        qmp_input_visitor_cleanup(mi);
 
         if (error_is_set(errp)) {
             goto out;
@@ -436,6 +435,7 @@ Example:
         }
 
     out:
+        qmp_input_visitor_cleanup(mi);
         md = qapi_dealloc_visitor_new();
         v = qapi_dealloc_get_visitor(md);
         visit_type_UserDefOne(v, &arg1, "arg1", NULL);
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 9734ab0..f56cc1c 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -69,16 +69,17 @@ def gen_marshal_output_call(name, ret_type):
         return ""
     return "qmp_marshal_output_%s(retval, ret, errp);" % c_fun(name)
 
-def gen_visitor_input_containers_decl(args):
+def gen_visitor_input_containers_decl(args, obj):
     ret = ""
 
     push_indent()
     if len(args) > 0:
         ret += mcgen('''
-QmpInputVisitor *mi;
+QmpInputVisitor *mi = qmp_input_visitor_new_strict(%(obj)s);
 QapiDeallocVisitor *md;
 Visitor *v;
-''')
+''',
+                     obj=obj)
     pop_indent()
 
     return ret.rstrip()
@@ -106,7 +107,7 @@ bool has_%(argname)s = false;
     pop_indent()
     return ret.rstrip()
 
-def gen_visitor_input_block(args, obj, dealloc=False):
+def gen_visitor_input_block(args, dealloc=False):
     ret = ""
     errparg = 'errp'
 
@@ -118,15 +119,14 @@ def gen_visitor_input_block(args, obj, dealloc=False):
     if dealloc:
         errparg = 'NULL'
         ret += mcgen('''
+qmp_input_visitor_cleanup(mi);
 md = qapi_dealloc_visitor_new();
 v = qapi_dealloc_get_visitor(md);
 ''')
     else:
         ret += mcgen('''
-mi = qmp_input_visitor_new_strict(%(obj)s);
 v = qmp_input_get_visitor(mi);
-''',
-                     obj=obj)
+''')
 
     for argname, argtype, optional, structured in parse_args(args):
         if optional:
@@ -152,10 +152,6 @@ visit_end_optional(v, %(errp)s);
         ret += mcgen('''
 qapi_dealloc_visitor_cleanup(md);
 ''')
-    else:
-        ret += mcgen('''
-qmp_input_visitor_cleanup(mi);
-''')
     pop_indent()
     return ret.rstrip()
 
@@ -166,8 +162,8 @@ 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)
 {
-    QapiDeallocVisitor *md = qapi_dealloc_visitor_new();
     QmpOutputVisitor *mo = qmp_output_visitor_new();
+    QapiDeallocVisitor *md;
     Visitor *v;
 
     v = qmp_output_get_visitor(mo);
@@ -176,6 +172,7 @@ static void qmp_marshal_output_%(c_name)s(%(c_ret_type)s ret_in, QObject **ret_o
         *ret_out = qmp_output_get_qobject(mo);
     }
     qmp_output_visitor_cleanup(mo);
+    md = qapi_dealloc_visitor_new();
     v = qapi_dealloc_get_visitor(md);
     %(visitor)s(v, &ret_in, "unused", NULL);
     qapi_dealloc_visitor_cleanup(md);
@@ -228,9 +225,9 @@ def gen_marshal_input(name, args, ret_type, middle_mode):
 %(visitor_input_block)s
 
 ''',
-                     visitor_input_containers_decl=gen_visitor_input_containers_decl(args),
+                     visitor_input_containers_decl=gen_visitor_input_containers_decl(args, "QOBJECT(args)"),
                      visitor_input_vars_decl=gen_visitor_input_vars_decl(args),
-                     visitor_input_block=gen_visitor_input_block(args, "QOBJECT(args)"))
+                     visitor_input_block=gen_visitor_input_block(args))
     else:
         ret += mcgen('''
     (void)args;
@@ -250,7 +247,7 @@ out:
     ret += mcgen('''
 %(visitor_input_block_cleanup)s
 ''',
-                 visitor_input_block_cleanup=gen_visitor_input_block(args, None,
+                 visitor_input_block_cleanup=gen_visitor_input_block(args,
                                                                      dealloc=True))
 
     if middle_mode:
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 02/13] qapi: Normalize marshalling's visitor initialization and cleanup

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 969 bytes --]

On 05/02/2014 06:44 AM, Markus Armbruster wrote:
> Input and output marshalling functions do it differently.  Change them
> to work the same: initialize the I/O visitor, use it, clean it up,
> initialize the dealloc visitor, use it, clean it up.
> 
> This delays dealloc visitor initialization in output marshalling
> functions, and input visitor cleanup in input marshalling functions.
> No functional change, but the latter will be convenient when I change
> the error handling.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  docs/qapi-code-gen.txt   |  8 ++++----
>  scripts/qapi-commands.py | 27 ++++++++++++---------------
>  2 files changed, 16 insertions(+), 19 deletions(-)

Tracking what the changes do in the examples certainly made it easier to
review.  Thanks.

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 03/13] qapi: Remove unused Visitor callbacks start_handle(), end_handle()

[Markus Armbruster]

These have never been called or implemented by anything, and their
intended use is undocumented, like all of the visitor API.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/visitor-impl.h |  3 ---
 qapi/qapi-visit-core.c      | 15 ---------------
 2 files changed, 18 deletions(-)

diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index f3fa420..166aadd 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -46,9 +46,6 @@ struct Visitor
                            Error **errp);
     void (*end_optional)(Visitor *v, Error **errp);
 
-    void (*start_handle)(Visitor *v, void **obj, const char *kind,
-                         const char *name, Error **errp);
-    void (*end_handle)(Visitor *v, Error **errp);
     void (*type_uint8)(Visitor *v, uint8_t *obj, const char *name, Error **errp);
     void (*type_uint16)(Visitor *v, uint16_t *obj, const char *name, Error **errp);
     void (*type_uint32)(Visitor *v, uint32_t *obj, const char *name, Error **errp);
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 6451a21..1f7475c 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -17,21 +17,6 @@
 #include "qapi/visitor.h"
 #include "qapi/visitor-impl.h"
 
-void visit_start_handle(Visitor *v, void **obj, const char *kind,
-                        const char *name, Error **errp)
-{
-    if (!error_is_set(errp) && v->start_handle) {
-        v->start_handle(v, obj, kind, name, errp);
-    }
-}
-
-void visit_end_handle(Visitor *v, Error **errp)
-{
-    if (!error_is_set(errp) && v->end_handle) {
-        v->end_handle(v, errp);
-    }
-}
-
 void visit_start_struct(Visitor *v, void **obj, const char *kind,
                         const char *name, size_t size, Error **errp)
 {
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 03/13] qapi: Remove unused Visitor callbacks start_handle(), end_handle()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 542 bytes --]

On 05/02/2014 06:44 AM, Markus Armbruster wrote:
> These have never been called or implemented by anything, and their
> intended use is undocumented, like all of the visitor API.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qapi/visitor-impl.h |  3 ---
>  qapi/qapi-visit-core.c      | 15 ---------------
>  2 files changed, 18 deletions(-)
> 

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 04/13] qapi: Replace start_optional()/end_optional() by optional()

[Markus Armbruster]

Semantics of end_optional() differ subtly from the other end_FOO()
callbacks: when start_FOO() succeeds, the matching end_FOO() gets
called regardless of what happens in between.  end_optional() gets
called only when everything in between succeeds as well.  Entirely
undocumented, like all of the visitor API.

The only user of Visitor Callback end_optional() never did anything,
and was removed in commit 9f9ab46.

I'm about to clean up error handling in the generated visitor code,
and end_optional() is in my way.  No users mean no test cases, and
making non-trivial cleanup transformations without test cases doesn't
strike me as a good idea.

Drop end_optional(), and rename start_optional() to optional().  We
can always go back to a pair of callbacks when we have an actual need.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/visitor-impl.h |  5 ++---
 include/qapi/visitor.h      |  5 ++---
 qapi/opts-visitor.c         |  5 ++---
 qapi/qapi-visit-core.c      | 15 ++++-----------
 qapi/qmp-input-visitor.c    |  6 +++---
 qapi/string-input-visitor.c |  6 +++---
 scripts/qapi-commands.py    |  5 ++---
 scripts/qapi-visit.py       |  3 +--
 8 files changed, 19 insertions(+), 31 deletions(-)

diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 166aadd..ecc0183 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -42,9 +42,8 @@ struct Visitor
                         Error **errp);
 
     /* May be NULL */
-    void (*start_optional)(Visitor *v, bool *present, const char *name,
-                           Error **errp);
-    void (*end_optional)(Visitor *v, Error **errp);
+    void (*optional)(Visitor *v, bool *present, const char *name,
+                     Error **errp);
 
     void (*type_uint8)(Visitor *v, uint8_t *obj, const char *name, Error **errp);
     void (*type_uint16)(Visitor *v, uint16_t *obj, const char *name, Error **errp);
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 29da211..4a0178f 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -39,9 +39,8 @@ void visit_end_implicit_struct(Visitor *v, Error **errp);
 void visit_start_list(Visitor *v, const char *name, Error **errp);
 GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp);
 void visit_end_list(Visitor *v, Error **errp);
-void visit_start_optional(Visitor *v, bool *present, const char *name,
-                          Error **errp);
-void visit_end_optional(Visitor *v, Error **errp);
+void visit_optional(Visitor *v, bool *present, const char *name,
+                    Error **errp);
 void visit_get_next_type(Visitor *v, int *obj, const int *qtypes,
                          const char *name, Error **errp);
 void visit_type_enum(Visitor *v, int *obj, const char *strings[],
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 5d830a2..1632c54 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -483,8 +483,7 @@ opts_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp)
 
 
 static void
-opts_start_optional(Visitor *v, bool *present, const char *name,
-                       Error **errp)
+opts_optional(Visitor *v, bool *present, const char *name, Error **errp)
 {
     OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
 
@@ -527,7 +526,7 @@ opts_visitor_new(const QemuOpts *opts)
     /* type_number() is not filled in, but this is not the first visitor to
      * skip some mandatory methods... */
 
-    ov->visitor.start_optional = &opts_start_optional;
+    ov->visitor.optional = &opts_optional;
 
     ov->opts_root = opts;
 
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 1f7475c..ffd7637 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -69,18 +69,11 @@ void visit_end_list(Visitor *v, Error **errp)
     v->end_list(v, errp);
 }
 
-void visit_start_optional(Visitor *v, bool *present, const char *name,
-                          Error **errp)
+void visit_optional(Visitor *v, bool *present, const char *name,
+                    Error **errp)
 {
-    if (!error_is_set(errp) && v->start_optional) {
-        v->start_optional(v, present, name, errp);
-    }
-}
-
-void visit_end_optional(Visitor *v, Error **errp)
-{
-    if (!error_is_set(errp) && v->end_optional) {
-        v->end_optional(v, errp);
+    if (!error_is_set(errp) && v->optional) {
+        v->optional(v, present, name, errp);
     }
 }
 
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index a2bed1e..d861206 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -286,8 +286,8 @@ static void qmp_input_type_number(Visitor *v, double *obj, const char *name,
     }
 }
 
-static void qmp_input_start_optional(Visitor *v, bool *present,
-                                     const char *name, Error **errp)
+static void qmp_input_optional(Visitor *v, bool *present, const char *name,
+                               Error **errp)
 {
     QmpInputVisitor *qiv = to_qiv(v);
     QObject *qobj = qmp_input_get_object(qiv, name, true);
@@ -329,7 +329,7 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
     v->visitor.type_bool = qmp_input_type_bool;
     v->visitor.type_str = qmp_input_type_str;
     v->visitor.type_number = qmp_input_type_number;
-    v->visitor.start_optional = qmp_input_start_optional;
+    v->visitor.optional = qmp_input_optional;
     v->visitor.get_next_type = qmp_input_get_next_type;
 
     qmp_input_push(v, obj, NULL);
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 793548a..5780944 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -120,8 +120,8 @@ static void parse_type_number(Visitor *v, double *obj, const char *name,
     *obj = val;
 }
 
-static void parse_start_optional(Visitor *v, bool *present,
-                                 const char *name, Error **errp)
+static void parse_optional(Visitor *v, bool *present, const char *name,
+                           Error **errp)
 {
     StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
 
@@ -155,7 +155,7 @@ StringInputVisitor *string_input_visitor_new(const char *str)
     v->visitor.type_bool = parse_type_bool;
     v->visitor.type_str = parse_type_str;
     v->visitor.type_number = parse_type_number;
-    v->visitor.start_optional = parse_start_optional;
+    v->visitor.optional = parse_optional;
 
     v->string = str;
     return v;
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index f56cc1c..dabd6c7 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -131,7 +131,7 @@ v = qmp_input_get_visitor(mi);
     for argname, argtype, optional, structured in parse_args(args):
         if optional:
             ret += mcgen('''
-visit_start_optional(v, &has_%(c_name)s, "%(name)s", %(errp)s);
+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)
@@ -145,8 +145,7 @@ if (has_%(c_name)s) {
             pop_indent()
             ret += mcgen('''
 }
-visit_end_optional(v, %(errp)s);
-''', errp=errparg)
+''')
 
     if dealloc:
         ret += mcgen('''
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 45ce3a9..b38d62e 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -61,7 +61,7 @@ if (!err) {
     for argname, argentry, optional, structured in parse_args(members):
         if optional:
             ret += mcgen('''
-visit_start_optional(m, &(*obj)->%(c_prefix)shas_%(c_name)s, "%(name)s", &err);
+visit_optional(m, &(*obj)->%(c_prefix)shas_%(c_name)s, "%(name)s", &err);
 if ((*obj)->%(prefix)shas_%(c_name)s) {
 ''',
                          c_prefix=c_var(field_prefix), prefix=field_prefix,
@@ -82,7 +82,6 @@ visit_type_%(type)s(m, &(*obj)->%(c_prefix)s%(c_name)s, "%(name)s", &err);
             pop_indent()
             ret += mcgen('''
 }
-visit_end_optional(m, &err);
 ''')
 
     pop_indent()
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 04/13] qapi: Replace start_optional()/end_optional() by optional()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1916 bytes --]

On 05/02/2014 06:44 AM, Markus Armbruster wrote:
> Semantics of end_optional() differ subtly from the other end_FOO()
> callbacks: when start_FOO() succeeds, the matching end_FOO() gets
> called regardless of what happens in between.  end_optional() gets
> called only when everything in between succeeds as well.  Entirely
> undocumented, like all of the visitor API.
> 
> The only user of Visitor Callback end_optional() never did anything,
> and was removed in commit 9f9ab46.
> 
> I'm about to clean up error handling in the generated visitor code,
> and end_optional() is in my way.  No users mean no test cases, and
> making non-trivial cleanup transformations without test cases doesn't
> strike me as a good idea.
> 
> Drop end_optional(), and rename start_optional() to optional().  We
> can always go back to a pair of callbacks when we have an actual need.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qapi/visitor-impl.h |  5 ++---
>  include/qapi/visitor.h      |  5 ++---
>  qapi/opts-visitor.c         |  5 ++---
>  qapi/qapi-visit-core.c      | 15 ++++-----------
>  qapi/qmp-input-visitor.c    |  6 +++---
>  qapi/string-input-visitor.c |  6 +++---
>  scripts/qapi-commands.py    |  5 ++---
>  scripts/qapi-visit.py       |  3 +--
>  8 files changed, 19 insertions(+), 31 deletions(-)

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

> +++ b/qapi/opts-visitor.c
> @@ -483,8 +483,7 @@ opts_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp)
>  
>  
>  static void
> -opts_start_optional(Visitor *v, bool *present, const char *name,
> -                       Error **errp)
> +opts_optional(Visitor *v, bool *present, const char *name, Error **errp)

Managed to fix an indentation problem while at it :)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 05/13] qapi-visit.py: Clean up confusing push_indent() / pop_indent() use

[Markus Armbruster]

Changing implicit indentation in the middle of generating a block
makes following the code being generated unnecessarily hard.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-visit.py | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index b38d62e..3eeb435 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -128,12 +128,14 @@ if (!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('''
-        /* Always call end_struct if start_struct succeeded.  */
-        visit_end_struct(m, &err);
-    }
-    error_propagate(errp, err);
 }
 ''')
     return ret
@@ -289,19 +291,15 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
 ''',
                  name=name)
 
-
-    push_indent()
     push_indent()
     push_indent()
 
     if base:
         ret += mcgen('''
-    visit_type_%(name)s_fields(m, obj, &err);
+        visit_type_%(name)s_fields(m, obj, &err);
 ''',
             name=name)
 
-    pop_indent()
-
     if not discriminator:
         disc_key = "type"
     else:
@@ -343,19 +341,17 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
         }
         error_propagate(errp, err);
         err = NULL;
-    }
 ''')
     pop_indent()
-    ret += mcgen('''
-        /* Always call end_struct if start_struct succeeded.  */
-        visit_end_struct(m, &err);
-    }
-    error_propagate(errp, err);
-}
-''')
+    pop_indent()
 
-    pop_indent();
     ret += mcgen('''
+            }
+            /* Always call end_struct if start_struct succeeded.  */
+            visit_end_struct(m, &err);
+        }
+        error_propagate(errp, err);
+    }
 }
 ''')
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 05/13] qapi-visit.py: Clean up confusing push_indent() / pop_indent() use

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 526 bytes --]

On 05/02/2014 06:44 AM, Markus Armbruster wrote:
> Changing implicit indentation in the middle of generating a block
> makes following the code being generated unnecessarily hard.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-visit.py | 32 ++++++++++++++------------------
>  1 file changed, 14 insertions(+), 18 deletions(-)

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 06/13] qapi: Clean up shadowing of parameters and locals in inner scopes

[Markus Armbruster]

By un-inlining the visit of nested complex types.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-visit.py | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 3eeb435..222ce62 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -35,6 +35,19 @@ def generate_visit_struct_fields(name, field_prefix, fn_prefix, members, base =
             nested_field_prefix = "%s%s." % (field_prefix, argname)
             ret += generate_visit_struct_fields(name, nested_field_prefix,
                                                 nested_fn_prefix, argentry)
+            ret += mcgen('''
+
+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('''
+}
+''')
 
     ret += mcgen('''
 
@@ -69,7 +82,10 @@ if ((*obj)->%(prefix)shas_%(c_name)s) {
             push_indent()
 
         if structured:
-            ret += generate_visit_struct_body(full_name, argname, argentry)
+            ret += mcgen('''
+visit_type_%(full_name)s_field_%(c_name)s(m, obj, &err);
+''',
+                         full_name=full_name, c_name=c_var(argname))
         else:
             ret += mcgen('''
 visit_type_%(type)s(m, &(*obj)->%(c_prefix)s%(c_name)s, "%(name)s", &err);
@@ -106,8 +122,6 @@ if (!error_is_set(errp)) {
 
     if len(field_prefix):
         ret += mcgen('''
-Error **errp = &err; /* from outer scope */
-Error *err = NULL;
 visit_start_struct(m, NULL, "", "%(name)s", 0, &err);
 ''',
                 name=name)
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 06/13] qapi: Clean up shadowing of parameters and locals in inner scopes

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 665 bytes --]

On 05/02/2014 06:44 AM, Markus Armbruster wrote:
> By un-inlining the visit of nested complex types.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-visit.py | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 

> @@ -106,8 +122,6 @@ if (!error_is_set(errp)) {
>  
>      if len(field_prefix):
>          ret += mcgen('''
> -Error **errp = &err; /* from outer scope */
> -Error *err = NULL;

Eww.  Yeah, that's worth cleaning up.

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 07/13] qapi-visit.py: Clean up a sloppy use of field prefix

[Markus Armbruster]

generate_visit_struct_fields() generates the base type's struct member
name both with and without the field prefix.  Harmless, because the
field prefix is always empty there: only unboxed complex members have
a prefix, and those can't have a base type.

Clean it up anyway.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-visit.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 222ce62..23ae5f2 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -60,7 +60,7 @@ static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error *
 
     if base:
         ret += mcgen('''
-visit_start_implicit_struct(m, (void**) &(*obj)->%(c_name)s, sizeof(%(type)s), &err);
+visit_start_implicit_struct(m, (void**) &(*obj)->%(c_prefix)s%(c_name)s, sizeof(%(type)s), &err);
 if (!err) {
     visit_type_%(type)s_fields(m, &(*obj)->%(c_prefix)s%(c_name)s, &err);
     error_propagate(errp, err);
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 07/13] qapi-visit.py: Clean up a sloppy use of field prefix

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 650 bytes --]

On 05/02/2014 06:44 AM, Markus Armbruster wrote:
> generate_visit_struct_fields() generates the base type's struct member
> name both with and without the field prefix.  Harmless, because the
> field prefix is always empty there: only unboxed complex members have
> a prefix, and those can't have a base type.
> 
> Clean it up anyway.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-visit.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 08/13] qapi: Un-inline visit of implicit struct

[Markus Armbruster]

In preparation of error handling changes.  Bonus: generates less
duplicated code.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-visit.py | 48 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 34 insertions(+), 14 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 23ae5f2..3e161bf 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -17,6 +17,31 @@ import os
 import getopt
 import errno
 
+implicit_structs = []
+
+def generate_visit_implicit_struct(type):
+    global implicit_structs
+    if type in implicit_structs:
+        return ''
+    implicit_structs.append(type)
+    return mcgen('''
+
+static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error **errp)
+{
+    Error *err = NULL;
+
+    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_end_implicit_struct(m, &err);
+    }
+    error_propagate(errp, err);
+}
+''',
+                 c_type=type_name(type))
+
 def generate_visit_struct_fields(name, field_prefix, fn_prefix, members, base = None):
     substructs = []
     ret = ''
@@ -49,6 +74,9 @@ static void visit_type_%(full_name)s_field_%(c_name)s(Visitor *m, %(name)s **obj
 }
 ''')
 
+    if base:
+        ret += generate_visit_implicit_struct(base)
+
     ret += mcgen('''
 
 static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error **errp)
@@ -60,13 +88,7 @@ static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error *
 
     if base:
         ret += mcgen('''
-visit_start_implicit_struct(m, (void**) &(*obj)->%(c_prefix)s%(c_name)s, sizeof(%(type)s), &err);
-if (!err) {
-    visit_type_%(type)s_fields(m, &(*obj)->%(c_prefix)s%(c_name)s, &err);
-    error_propagate(errp, err);
-    err = NULL;
-    visit_end_implicit_struct(m, &err);
-}
+visit_type_implicit_%(type)s(m, &(*obj)->%(c_prefix)s%(c_name)s, &err);
 ''',
                      c_prefix=c_var(field_prefix),
                      type=type_name(base), c_name=c_var('base'))
@@ -292,6 +314,10 @@ def generate_visit_union(expr):
             del base_fields[discriminator]
         ret += generate_visit_struct_fields(name, "", "", base_fields)
 
+    if discriminator:
+        for key in members:
+            ret += generate_visit_implicit_struct(members[key])
+
     ret += mcgen('''
 
 void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
@@ -330,13 +356,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
         if not discriminator:
             fmt = 'visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", &err);'
         else:
-            fmt = '''visit_start_implicit_struct(m, (void**) &(*obj)->%(c_name)s, sizeof(%(c_type)s), &err);
-                if (!err) {
-                    visit_type_%(c_type)s_fields(m, &(*obj)->%(c_name)s, &err);
-                    error_propagate(errp, err);
-                    err = NULL;
-                    visit_end_implicit_struct(m, &err);
-                }'''
+            fmt = 'visit_type_implicit_%(c_type)s(m, &(*obj)->%(c_name)s, &err);'
 
         enum_full_value = generate_enum_full_value(disc_type, key)
         ret += mcgen('''
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 08/13] qapi: Un-inline visit of implicit struct

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 600 bytes --]

On 05/02/2014 06:44 AM, Markus Armbruster wrote:
> In preparation of error handling changes.  Bonus: generates less
> duplicated code.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-visit.py | 48 ++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 34 insertions(+), 14 deletions(-)

Too bad the example txt file didn't cover this case, to show the
difference in the generated code.

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 08/13] qapi: Un-inline visit of implicit struct

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/02/2014 06:44 AM, Markus Armbruster wrote:
>> In preparation of error handling changes.  Bonus: generates less
>> duplicated code.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  scripts/qapi-visit.py | 48 ++++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 34 insertions(+), 14 deletions(-)
>
> Too bad the example txt file didn't cover this case, to show the
> difference in the generated code.

Yes.  I considered extending the example in docs/qapi-code-gen.txt, but
I'm afraid our schema language has become too big for complete coverage
in that example.

tests/qapi-schema-test.json should use every schema language feature.
The code generated for it isn't in git, though.  To see diffs, so you
have to apply patches, make check, and compare tests/test-q*.[ch] by
hand.

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

Thanks!

[Qemu-devel] [PATCH 09/13] hmp: Call visit_end_struct() after visit_start_struct() succeeds

[Markus Armbruster]

When visit_start_struct() succeeds, visit_end_struct() must be called.
hmp_object_add() doesn't when a member visit fails.  As far as I can
tell, the opts visitor copes okay with the misuse.  Fix it anyway.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hmp.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/hmp.c b/hmp.c
index ad31ceb..a7cd59e 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1384,6 +1384,7 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
 void hmp_object_add(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
+    Error *err_end = NULL;
     QemuOpts *opts;
     char *type = NULL;
     char *id = NULL;
@@ -1407,24 +1408,23 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
     qdict_del(pdict, "qom-type");
     visit_type_str(opts_get_visitor(ov), &type, "qom-type", &err);
     if (err) {
-        goto out_clean;
+        goto out_end;
     }
 
     qdict_del(pdict, "id");
     visit_type_str(opts_get_visitor(ov), &id, "id", &err);
     if (err) {
-        goto out_clean;
+        goto out_end;
     }
 
     object_add(type, id, pdict, opts_get_visitor(ov), &err);
-    if (err) {
-        goto out_clean;
-    }
-    visit_end_struct(opts_get_visitor(ov), &err);
-    if (err) {
+
+out_end:
+    visit_end_struct(opts_get_visitor(ov), &err_end);
+    if (!err && err_end) {
         qmp_object_del(id, NULL);
     }
-
+    error_propagate(&err, err_end);
 out_clean:
     opts_visitor_cleanup(ov);
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 09/13] hmp: Call visit_end_struct() after visit_start_struct() succeeds

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 579 bytes --]

On 05/02/2014 06:44 AM, Markus Armbruster wrote:
> When visit_start_struct() succeeds, visit_end_struct() must be called.
> hmp_object_add() doesn't when a member visit fails.  As far as I can
> tell, the opts visitor copes okay with the misuse.  Fix it anyway.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hmp.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 10/13] hw: Don't call visit_end_struct() after visit_start_struct() fails

[Markus Armbruster]

When visit_start_struct() succeeds, visit_end_struct() must not be
called.  rtc_get_date() and balloon_stats_all() call it anyway.  As
far as I can tell, they're only used with the string output visitor,
which doesn't care.  Fix them anyway.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/timer/mc146818rtc.c     | 23 +++++++++++++++--------
 hw/virtio/virtio-balloon.c | 25 +++++++++++++++++++------
 2 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 8509309..6c3e3b6 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -793,19 +793,26 @@ static const MemoryRegionOps cmos_ops = {
 static void rtc_get_date(Object *obj, Visitor *v, void *opaque,
                          const char *name, Error **errp)
 {
+    Error *err = NULL;
     RTCState *s = MC146818_RTC(obj);
     struct tm current_tm;
 
     rtc_update_time(s);
     rtc_get_time(s, &current_tm);
-    visit_start_struct(v, NULL, "struct tm", name, 0, errp);
-    visit_type_int32(v, &current_tm.tm_year, "tm_year", errp);
-    visit_type_int32(v, &current_tm.tm_mon, "tm_mon", errp);
-    visit_type_int32(v, &current_tm.tm_mday, "tm_mday", errp);
-    visit_type_int32(v, &current_tm.tm_hour, "tm_hour", errp);
-    visit_type_int32(v, &current_tm.tm_min, "tm_min", errp);
-    visit_type_int32(v, &current_tm.tm_sec, "tm_sec", errp);
-    visit_end_struct(v, errp);
+    visit_start_struct(v, NULL, "struct tm", name, 0, &err);
+    if (err) {
+        goto out;
+    }
+    visit_type_int32(v, &current_tm.tm_year, "tm_year", &err);
+    visit_type_int32(v, &current_tm.tm_mon, "tm_mon", &err);
+    visit_type_int32(v, &current_tm.tm_mday, "tm_mday", &err);
+    visit_type_int32(v, &current_tm.tm_hour, "tm_hour", &err);
+    visit_type_int32(v, &current_tm.tm_min, "tm_min", &err);
+    visit_type_int32(v, &current_tm.tm_sec, "tm_sec", &err);
+    visit_end_struct(v, &err);
+
+out:
+    error_propagate(errp, err);
 }
 
 static void rtc_realizefn(DeviceState *dev, Error **errp)
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 51f02eb..65d05c8 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -108,6 +108,7 @@ static void balloon_stats_poll_cb(void *opaque)
 static void balloon_stats_get_all(Object *obj, struct Visitor *v,
                                   void *opaque, const char *name, Error **errp)
 {
+    Error *err = NULL;
     VirtIOBalloon *s = opaque;
     int i;
 
@@ -116,17 +117,29 @@ static void balloon_stats_get_all(Object *obj, struct Visitor *v,
         return;
     }
 
-    visit_start_struct(v, NULL, "guest-stats", name, 0, errp);
-    visit_type_int(v, &s->stats_last_update, "last-update", errp);
+    visit_start_struct(v, NULL, "guest-stats", name, 0, &err);
+    if (err) {
+        goto out;
+    }
+
+    visit_type_int(v, &s->stats_last_update, "last-update", &err);
 
-    visit_start_struct(v, NULL, NULL, "stats", 0, errp);
+    visit_start_struct(v, NULL, NULL, "stats", 0, &err);
+    if (err) {
+        goto out_end;
+    }
+        
     for (i = 0; i < VIRTIO_BALLOON_S_NR; i++) {
         visit_type_int64(v, (int64_t *) &s->stats[i], balloon_stat_names[i],
-                         errp);
+                         &err);
     }
-    visit_end_struct(v, errp);
+    visit_end_struct(v, &err);
+
+out_end:
+    visit_end_struct(v, &err);
 
-    visit_end_struct(v, errp);
+out:
+    error_propagate(errp, err);
 }
 
 static void balloon_stats_get_poll_interval(Object *obj, struct Visitor *v,
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 10/13] hw: Don't call visit_end_struct() after visit_start_struct() fails

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 870 bytes --]

On 05/02/2014 06:44 AM, Markus Armbruster wrote:
> When visit_start_struct() succeeds, visit_end_struct() must not be

s/succeeds/fails/ (this really confused me on my first read, until I saw
the code and the subject line and determined the typo)

> called.  rtc_get_date() and balloon_stats_all() call it anyway.  As
> far as I can tell, they're only used with the string output visitor,
> which doesn't care.  Fix them anyway.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/timer/mc146818rtc.c     | 23 +++++++++++++++--------
>  hw/virtio/virtio-balloon.c | 25 +++++++++++++++++++------
>  2 files changed, 34 insertions(+), 14 deletions(-)

With commit message fixed,

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 10/13] hw: Don't call visit_end_struct() after visit_start_struct() fails

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/02/2014 06:44 AM, Markus Armbruster wrote:
>> When visit_start_struct() succeeds, visit_end_struct() must not be
>
> s/succeeds/fails/ (this really confused me on my first read, until I saw
> the code and the subject line and determined the typo)

D'oh!  Will fix both here an in 10/13/

>> called.  rtc_get_date() and balloon_stats_all() call it anyway.  As
>> far as I can tell, they're only used with the string output visitor,
>> which doesn't care.  Fix them anyway.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/timer/mc146818rtc.c     | 23 +++++++++++++++--------
>>  hw/virtio/virtio-balloon.c | 25 +++++++++++++++++++------
>>  2 files changed, 34 insertions(+), 14 deletions(-)
>
> With commit message fixed,
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

[Qemu-devel] [PATCH 11/13] tests: Don't call visit_end_struct() after visit_start_struct() fails

[Markus Armbruster]

When visit_start_struct() succeeds, visit_end_struct() must not be
called.  Three out of four visit_type_TestStruct() call it anyway.  As
far as I can tell, visit_start_struct() doesn't actually fail there.
Fix them anyway.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-qmp-input-strict.c      | 18 +++++++++++++-----
 tests/test-qmp-output-visitor.c    | 18 +++++++++++++-----
 tests/test-visitor-serialization.c | 18 +++++++++++++-----
 3 files changed, 39 insertions(+), 15 deletions(-)

diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
index 449d285..ec798c2 100644
--- a/tests/test-qmp-input-strict.c
+++ b/tests/test-qmp-input-strict.c
@@ -72,14 +72,22 @@ typedef struct TestStruct
 static void visit_type_TestStruct(Visitor *v, TestStruct **obj,
                                   const char *name, Error **errp)
 {
+    Error *err = NULL;
+
     visit_start_struct(v, (void **)obj, "TestStruct", name, sizeof(TestStruct),
-                       errp);
+                       &err);
+    if (err) {
+        goto out;
+    }
+
+    visit_type_int(v, &(*obj)->integer, "integer", &err);
+    visit_type_bool(v, &(*obj)->boolean, "boolean", &err);
+    visit_type_str(v, &(*obj)->string, "string", &err);
 
-    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, &err);
 
-    visit_end_struct(v, errp);
+out:
+    error_propagate(errp, err);
 }
 
 static void test_validate_struct(TestInputVisitorData *data,
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 2580f3d..dfd597c 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -176,14 +176,22 @@ typedef struct TestStruct
 static void visit_type_TestStruct(Visitor *v, TestStruct **obj,
                                   const char *name, Error **errp)
 {
+    Error *err = NULL;
+
     visit_start_struct(v, (void **)obj, "TestStruct", name, sizeof(TestStruct),
-                       errp);
+                       &err);
+    if (err) {
+        goto out;
+    }
+
+    visit_type_int(v, &(*obj)->integer, "integer", &err);
+    visit_type_bool(v, &(*obj)->boolean, "boolean", &err);
+    visit_type_str(v, &(*obj)->string, "string", &err);
 
-    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, &err);
 
-    visit_end_struct(v, errp);
+out:
+    error_propagate(errp, err);
 }
 
 static void test_visitor_out_struct(TestOutputVisitorData *data,
diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c
index 8166cf1..85170e5 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -195,13 +195,21 @@ typedef struct TestStruct
 static void visit_type_TestStruct(Visitor *v, TestStruct **obj,
                                   const char *name, Error **errp)
 {
-    visit_start_struct(v, (void **)obj, NULL, name, sizeof(TestStruct), errp);
+    Error *err= NULL;
 
-    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_start_struct(v, (void **)obj, NULL, name, sizeof(TestStruct), &err);
+    if (err) {
+        goto out;
+    }
+
+    visit_type_int(v, &(*obj)->integer, "integer", &err);
+    visit_type_bool(v, &(*obj)->boolean, "boolean", &err);
+    visit_type_str(v, &(*obj)->string, "string", &err);
+
+    visit_end_struct(v, &err);
 
-    visit_end_struct(v, errp);
+out:
+    error_propagate(errp, err);
 }
 
 static TestStruct *struct_create(void)
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 11/13] tests: Don't call visit_end_struct() after visit_start_struct() fails

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 825 bytes --]

On 05/02/2014 06:44 AM, Markus Armbruster wrote:
> When visit_start_struct() succeeds, visit_end_struct() must not be

As in 10/13, s/succeeds/fails/

> called.  Three out of four visit_type_TestStruct() call it anyway.  As
> far as I can tell, visit_start_struct() doesn't actually fail there.
> Fix them anyway.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/test-qmp-input-strict.c      | 18 +++++++++++++-----
>  tests/test-qmp-output-visitor.c    | 18 +++++++++++++-----
>  tests/test-visitor-serialization.c | 18 +++++++++++++-----
>  3 files changed, 39 insertions(+), 15 deletions(-)
> 

With commit message fixed,

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Markus Armbruster]

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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1616 bytes --]

On 05/02/2014 06:44 AM, Markus Armbruster wrote:
> We commonly use the error API like this:
> 

> 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.

Nice comparison of the two techniques.

> 
> Standardize on the "check separately" technique for now, because it's
> overwhelmingly prevalent.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

> +++ b/hw/virtio/virtio-balloon.c
> @@ -121,23 +121,27 @@ static void balloon_stats_get_all(Object *obj, struct Visitor *v,

>      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++) {

Oops; logic error makes this loop dead code.

s/err/!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

Unrelated typo fix; could go in separately via -trivial if you were so
inclined, but I don't mind it going in here.

If the logic error is the only fix,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/02/2014 06:44 AM, Markus Armbruster wrote:
>> We commonly use the error API like this:
>> 
>
>> 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.
>
> Nice comparison of the two techniques.
>
>> 
>> Standardize on the "check separately" technique for now, because it's
>> overwhelmingly prevalent.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
>> +++ b/hw/virtio/virtio-balloon.c
>> @@ -121,23 +121,27 @@ static void balloon_stats_get_all(Object *obj, struct Visitor *v,
>
>>      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++) {
>
> Oops; logic error makes this loop dead code.
>
> s/err/!err/

Oww!  Respin coming...  with all your nits fixed (you earned that).

>> @@ -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
>
> Unrelated typo fix; could go in separately via -trivial if you were so
> inclined, but I don't mind it going in here.
>
> If the logic error is the only fix,
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

[Qemu-devel] [PATCH 13/13] error: error_is_set() is finally unused; remove

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/error.h | 6 ------
 util/error.c         | 5 -----
 2 files changed, 11 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 7995801..d712089 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -67,12 +67,6 @@ void error_set_win32(Error **errp, int win32_err, ErrorClass err_class,
  */
 void error_setg_file_open(Error **errp, int os_errno, const char *filename);
 
-/**
- * Returns true if an indirect pointer to an error is pointing to a valid
- * error object.
- */
-bool error_is_set(Error **errp);
-
 /*
  * Get the error class of an error object.
  */
diff --git a/util/error.c b/util/error.c
index 66245cc..2ace0d8 100644
--- a/util/error.c
+++ b/util/error.c
@@ -142,11 +142,6 @@ Error *error_copy(const Error *err)
     return err_new;
 }
 
-bool error_is_set(Error **errp)
-{
-    return (errp && *errp);
-}
-
 ErrorClass error_get_class(const Error *err)
 {
     return err->err_class;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 13/13] error: error_is_set() is finally unused; remove

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 498 bytes --]

On 05/02/2014 06:44 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/qapi/error.h | 6 ------
>  util/error.c         | 5 -----
>  2 files changed, 11 deletions(-)

Of course, depends on several in-flight series.  But assuming that all
works out,

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

and thanks for tackling this.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]