[PATCH v2 00/15] qapi: Spring cleaning

[PATCH v2 00/15] qapi: Spring cleaning

[Markus Armbruster]

v2:
* PATCH 10: Redone to avoid memory leak [Eric]
* PATCH 14+15: New

Since PATCH 14 depends on "[PATCH 2/3] fuzz: Simplify how we compute
available machines and types":
Based-on: <20200424071142.3525-1-armbru@redhat.com>

Markus Armbruster (15):
  qapi: Belatedly update visitor.h's big comment for QAPI modules
  qapi: Fix the virtual walk example in visitor.h's big comment
  qapi: Fix typo in visit_start_list()'s contract
  qapi: Document @errp usage more thoroughly in visitor.h
  qapi: Polish prose in visitor.h
  qapi: Assert incomplete object occurs only in dealloc visitor
  qapi: Fix Visitor contract for start_alternate()
  qapi: Assert output visitors see only valid enum values
  qapi: Assert non-input visitors see only valid narrow integers
  qapi: Clean up visitor's recovery from input with invalid type
  qapi: Assert non-input visitors see only valid alternate tags
  qapi: Only input visitors can actually fail
  qom: Simplify object_property_get_enum()
  qapi: Disallow qmp_marshal_FOO(NULL, ...)
  qapi: Generate simpler marshalling code when no arguments

 docs/devel/qapi-code-gen.txt        |   4 +-
 include/qapi/visitor-impl.h         |   9 +-
 include/qapi/visitor.h              | 192 ++++++++++++++++------------
 block.c                             |   9 +-
 block/sheepdog.c                    |   9 +-
 blockdev.c                          |  16 +--
 hw/core/machine-hmp-cmds.c          |   2 +-
 monitor/hmp-cmds.c                  |   3 +-
 monitor/qmp.c                       |   5 +-
 qapi/qapi-dealloc-visitor.c         |   7 -
 qapi/qapi-visit-core.c              |  20 +--
 qom/object.c                        |   4 +-
 tests/test-qobject-output-visitor.c |  39 ------
 tests/test-string-output-visitor.c  |  19 ---
 scripts/qapi/commands.py            |  62 ++++-----
 scripts/qapi/visit.py               |   8 ++
 16 files changed, 174 insertions(+), 234 deletions(-)

-- 
2.21.1

[PATCH v2 01/15] qapi: Belatedly update visitor.h's big comment for QAPI modules

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/qapi/visitor.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index c5b23851a1..f8a0fc1ea9 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -58,7 +58,7 @@
  *
  * where T is FOO for scalar types, and FOO * otherwise.  The scalar
  * visitors are declared here; the remaining visitors are generated in
- * qapi-visit.h.
+ * qapi-visit-MODULE.h.
  *
  * The @name parameter of visit_type_FOO() describes the relation
  * between this QAPI value and its parent container.  When visiting
@@ -86,16 +86,16 @@
  * by manual construction.
  *
  * For the QAPI object types (structs, unions, and alternates), there
- * is an additional generated function in qapi-visit.h compatible
- * with:
+ * is an additional generated function in qapi-visit-MODULE.h
+ * compatible with:
  *
  * void visit_type_FOO_members(Visitor *v, FOO *obj, Error **errp);
  *
  * for visiting the members of a type without also allocating the QAPI
  * struct.
  *
- * Additionally, in qapi-types.h, all QAPI pointer types (structs,
- * unions, alternates, and lists) have a generated function compatible
+ * Additionally, QAPI pointer types (structs, unions, alternates, and
+ * lists) have a generated function in qapi-types-MODULE.h compatible
  * with:
  *
  * void qapi_free_FOO(FOO *obj);
-- 
2.21.1

[PATCH v2 02/15] qapi: Fix the virtual walk example in visitor.h's big comment

[Markus Armbruster]

Call visit_check_list().  Missed in commit a4a1c70dc7 "qapi: Make
input visitors detect unvisited list tails".

Drop an irrelevant error_propagate() while there.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/qapi/visitor.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index f8a0fc1ea9..7f63e4c381 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -215,6 +215,9 @@
  *      goto outlist;
  *  }
  * outlist:
+ *  if (!err) {
+ *      visit_check_list(v, &err);
+ *  }
  *  visit_end_list(v, NULL);
  *  if (!err) {
  *      visit_check_struct(v, &err);
@@ -222,7 +225,6 @@
  * outobj:
  *  visit_end_struct(v, NULL);
  * out:
- *  error_propagate(errp, err);
  *  visit_free(v);
  * </example>
  */
-- 
2.21.1

[PATCH v2 03/15] qapi: Fix typo in visit_start_list()'s contract

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/qapi/visitor.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 7f63e4c381..c5d0ce9184 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -345,9 +345,9 @@ void visit_end_struct(Visitor *v, void **obj);
  * input visitors set *@list to NULL.
  *
  * After visit_start_list() succeeds, the caller may visit its members
- * one after the other.  A real visit (where @obj is non-NULL) uses
+ * one after the other.  A real visit (where @list is non-NULL) uses
  * visit_next_list() for traversing the linked list, while a virtual
- * visit (where @obj is NULL) uses other means.  For each list
+ * visit (where @list is NULL) uses other means.  For each list
  * element, call the appropriate visit_type_FOO() with name set to
  * NULL and obj set to the address of the value member of the list
  * element.  Finally, visit_end_list() needs to be called with the
-- 
2.21.1

[PATCH v2 04/15] qapi: Document @errp usage more thoroughly in visitor.h

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/qapi/visitor.h | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index c5d0ce9184..09df7099c6 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -284,9 +284,7 @@ void visit_free(Visitor *v);
  * into *@obj.  @obj may also be NULL for a virtual walk, in which
  * case @size is ignored.
  *
- * @errp obeys typical error usage, and reports failures such as a
- * member @name is not present, or present but not an object.  On
- * error, input visitors set *@obj to NULL.
+ * On failure, set *@obj to NULL and store an error through @errp.
  *
  * After visit_start_struct() succeeds, the caller may visit its
  * members one after the other, passing the member's name and address
@@ -303,8 +301,7 @@ void visit_start_struct(Visitor *v, const char *name, void **obj,
 /*
  * Prepare for completing an object visit.
  *
- * @errp obeys typical error usage, and reports failures such as
- * unparsed keys remaining in the input stream.
+ * On failure, store an error through @errp.
  *
  * Should be called prior to visit_end_struct() if all other
  * intermediate visit steps were successful, to allow the visitor one
@@ -340,9 +337,7 @@ void visit_end_struct(Visitor *v, void **obj);
  * allow @list to be NULL for a virtual walk, in which case @size is
  * ignored.
  *
- * @errp obeys typical error usage, and reports failures such as a
- * member @name is not present, or present but not a list.  On error,
- * input visitors set *@list to NULL.
+ * On failure, set *@list to NULL and store an error through @errp.
  *
  * After visit_start_list() succeeds, the caller may visit its members
  * one after the other.  A real visit (where @list is non-NULL) uses
@@ -376,8 +371,7 @@ GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size);
 /*
  * Prepare for completing a list visit.
  *
- * @errp obeys typical error usage, and reports failures such as
- * unvisited list tail remaining in the input stream.
+ * On failure, store an error through @errp.
  *
  * Should be called prior to visit_end_list() if all other
  * intermediate visit steps were successful, to allow the visitor one
@@ -409,8 +403,10 @@ void visit_end_list(Visitor *v, void **list);
  *
  * @obj must not be NULL. Input and clone visitors use @size to
  * determine how much memory to allocate into *@obj, then determine
- * the qtype of the next thing to be visited, stored in (*@obj)->type.
- * Other visitors will leave @obj unchanged.
+ * the qtype of the next thing to be visited, and store it in
+ * (*@obj)->type.  Other visitors leave @obj unchanged.
+ *
+ * On failure, set *@obj to NULL and store an error through @errp.
  *
  * If successful, this must be paired with visit_end_alternate() with
  * the same @obj to clean up, even if visiting the contents of the
@@ -463,8 +459,9 @@ bool visit_optional(Visitor *v, const char *name, bool *present);
  *
  * Currently, all input visitors parse text input, and all output
  * visitors produce text output.  The mapping between enumeration
- * values and strings is done by the visitor core, using @strings; it
- * should be the ENUM_lookup array from visit-types.h.
+ * values and strings is done by the visitor core, using @lookup.
+ *
+ * On failure, store an error through @errp.
  *
  * May call visit_type_str() under the hood, and the enum visit may
  * fail even if the corresponding string visit succeeded; this implies
@@ -488,6 +485,8 @@ bool visit_is_input(Visitor *v);
  *
  * @obj must be non-NULL.  Input visitors set *@obj to the value;
  * other visitors will leave *@obj unchanged.
+ *
+ * On failure, store an error through @errp.
  */
 void visit_type_int(Visitor *v, const char *name, int64_t *obj, Error **errp);
 
@@ -564,6 +563,8 @@ void visit_type_size(Visitor *v, const char *name, uint64_t *obj,
  *
  * @obj must be non-NULL.  Input visitors set *@obj to the value;
  * other visitors will leave *@obj unchanged.
+ *
+ * On failure, store an error through @errp.
  */
 void visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp);
 
@@ -581,6 +582,8 @@ void visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp);
  * It is safe to cast away const when preparing a (const char *) value
  * into @obj for use by an output visitor.
  *
+ * On failure, set *@obj to NULL and store an error through @errp.
+ *
  * FIXME: Callers that try to output NULL *obj should not be allowed.
  */
 void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp);
@@ -594,6 +597,8 @@ void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp);
  * @obj must be non-NULL.  Input visitors set *@obj to the value;
  * other visitors will leave *@obj unchanged.  Visitors should
  * document if infinity or NaN are not permitted.
+ *
+ * On failure, store an error through @errp.
  */
 void visit_type_number(Visitor *v, const char *name, double *obj,
                        Error **errp);
@@ -608,6 +613,8 @@ void visit_type_number(Visitor *v, const char *name, double *obj,
  * other visitors will leave *@obj unchanged.  *@obj must be non-NULL
  * for output visitors.
  *
+ * On failure, set *@obj to NULL and store an error through @errp.
+ *
  * Note that some kinds of input can't express arbitrary QObject.
  * E.g. the visitor returned by qobject_input_visitor_new_keyval()
  * can't create numbers or booleans, only strings.
@@ -622,6 +629,8 @@ void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp);
  *
  * @obj must be non-NULL.  Input visitors set *@obj to the value;
  * other visitors ignore *@obj.
+ *
+ * On failure, set *@obj to NULL and store an error through @errp.
  */
 void visit_type_null(Visitor *v, const char *name, QNull **obj,
                      Error **errp);
-- 
2.21.1

[PATCH v2 05/15] qapi: Polish prose in visitor.h

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/qapi/visitor.h | 104 +++++++++++++++++++++--------------------
 1 file changed, 54 insertions(+), 50 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 09df7099c6..a425ea514c 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -25,19 +25,21 @@
  * for doing work at each node of a QAPI graph; it can also be used
  * for a virtual walk, where there is no actual QAPI C struct.
  *
- * There are four kinds of visitor classes: input visitors (QObject,
- * string, and QemuOpts) parse an external representation and build
- * the corresponding QAPI graph, output visitors (QObject and string) take
- * a completed QAPI graph and generate an external representation, the
- * dealloc visitor can take a QAPI graph (possibly partially
- * constructed) and recursively free its resources, and the clone
- * visitor performs a deep clone of one QAPI object to another.  While
- * the dealloc and QObject input/output visitors are general, the string,
- * QemuOpts, and clone visitors have some implementation limitations;
- * see the documentation for each visitor for more details on what it
- * supports.  Also, see visitor-impl.h for the callback contracts
- * implemented by each visitor, and docs/devel/qapi-code-gen.txt for more
- * about the QAPI code generator.
+ * There are four kinds of visitors: input visitors (QObject, string,
+ * and QemuOpts) parse an external representation and build the
+ * corresponding QAPI object, output visitors (QObject and string)
+ * take a QAPI object and generate an external representation, the
+ * dealloc visitor takes a QAPI object (possibly partially
+ * constructed) and recursively frees it, and the clone visitor
+ * performs a deep clone of a QAPI object.
+ *
+ * While the dealloc and QObject input/output visitors are general,
+ * the string, QemuOpts, and clone visitors have some implementation
+ * limitations; see the documentation for each visitor for more
+ * details on what it supports.  Also, see visitor-impl.h for the
+ * callback contracts implemented by each visitor, and
+ * docs/devel/qapi-code-gen.txt for more about the QAPI code
+ * generator.
  *
  * All of the visitors are created via:
  *
@@ -45,11 +47,15 @@
  *
  * A visitor should be used for exactly one top-level visit_type_FOO()
  * or virtual walk; if that is successful, the caller can optionally
- * call visit_complete() (for now, useful only for output visits, but
- * safe to call on all visits).  Then, regardless of success or
- * failure, the user should call visit_free() to clean up resources.
- * It is okay to free the visitor without completing the visit, if
- * some other error is detected in the meantime.
+ * call visit_complete() (useful only for output visits, but safe to
+ * call on all visits).  Then, regardless of success or failure, the
+ * user should call visit_free() to clean up resources.  It is okay to
+ * free the visitor without completing the visit, if some other error
+ * is detected in the meantime.
+ *
+ * The clone and dealloc visitor should not be used directly outside
+ * of QAPI code.  Use the qapi_free_FOO() and QAPI_CLONE() instead,
+ * described below.
  *
  * All QAPI types have a corresponding function with a signature
  * roughly compatible with this:
@@ -68,22 +74,26 @@
  * alternate, @name should equal the name used for visiting the
  * alternate.
  *
- * The visit_type_FOO() functions expect a non-null @obj argument;
- * they allocate *@obj during input visits, leave it unchanged on
- * output visits, and recursively free any resources during a dealloc
- * visit.  Each function also takes the customary @errp argument (see
+ * The visit_type_FOO() functions take a non-null @obj argument; they
+ * allocate *@obj during input visits, leave it unchanged during
+ * output and clone visits, and free it (recursively) during a dealloc
+ * visit.
+ *
+ * Each function also takes the customary @errp argument (see
  * qapi/error.h for details), for reporting any errors (such as if a
  * member @name is not present, or is present but not the specified
  * type).
  *
  * If an error is detected during visit_type_FOO() with an input
- * visitor, then *@obj will be NULL for pointer types, and left
- * unchanged for scalar types.  Using an output or clone visitor with
- * an incomplete object has undefined behavior (other than a special
- * case for visit_type_str() treating NULL like ""), while the dealloc
- * visitor safely handles incomplete objects.  Since input visitors
- * never produce an incomplete object, such an object is possible only
- * by manual construction.
+ * visitor, then *@obj will be set to NULL for pointer types, and left
+ * unchanged for scalar types.
+ *
+ * Using an output or clone visitor with an incomplete object has
+ * undefined behavior (other than a special case for visit_type_str()
+ * treating NULL like ""), while the dealloc visitor safely handles
+ * incomplete objects.  Since input visitors never produce an
+ * incomplete object, such an object is possible only by manual
+ * construction.
  *
  * For the QAPI object types (structs, unions, and alternates), there
  * is an additional generated function in qapi-visit-MODULE.h
@@ -100,23 +110,20 @@
  *
  * void qapi_free_FOO(FOO *obj);
  *
- * where behaves like free() in that @obj may be NULL.  Such objects
- * may also be used with the following macro, provided alongside the
- * clone visitor:
+ * Does nothing when @obj is NULL.
+ *
+ * Such objects may also be used with macro
  *
  * Type *QAPI_CLONE(Type, src);
  *
- * in order to perform a deep clone of @src.  Because of the generated
- * qapi_free functions and the QAPI_CLONE() macro, the clone and
- * dealloc visitor should not be used directly outside of QAPI code.
+ * in order to perform a deep clone of @src.
  *
- * QAPI types can also inherit from a base class; when this happens, a
- * function is generated for easily going from the derived type to the
- * base type:
+ * For QAPI types can that inherit from a base type, a function is
+ * generated for going from the derived type to the base type:
  *
  * BASE *qapi_CHILD_base(CHILD *obj);
  *
- * For a real QAPI struct, typical input usage involves:
+ * Typical input visitor usage involves:
  *
  * <example>
  *  Foo *f;
@@ -153,7 +160,7 @@
  *  qapi_free_FooList(l);
  * </example>
  *
- * Similarly, typical output usage is:
+ * Typical output visitor usage:
  *
  * <example>
  *  Foo *f = ...obtain populated object...
@@ -172,17 +179,8 @@
  *  visit_free(v);
  * </example>
  *
- * When visiting a real QAPI struct, this file provides several
- * helpers that rely on in-tree information to control the walk:
- * visit_optional() for the 'has_member' field associated with
- * optional 'member' in the C struct; and visit_next_list() for
- * advancing through a FooList linked list.  Similarly, the
- * visit_is_input() helper makes it possible to write code that is
- * visitor-agnostic everywhere except for cleanup.  Only the generated
- * visit_type functions need to use these helpers.
- *
  * It is also possible to use the visitors to do a virtual walk, where
- * no actual QAPI struct is present.  In this situation, decisions
+ * no actual QAPI object is present.  In this situation, decisions
  * about what needs to be walked are made by the calling code, and
  * structured visits are split between pairs of start and end methods
  * (where the end method must be called if the start function
@@ -227,6 +225,12 @@
  * out:
  *  visit_free(v);
  * </example>
+ *
+ * This file provides helpers for use by the generated
+ * visit_type_FOO(): visit_optional() for the 'has_member' field
+ * associated with optional 'member' in the C struct,
+ * visit_next_list() for advancing through a FooList linked list, and
+ * visit_is_input() for cleaning up on failure.
  */
 
 /*** Useful types ***/
-- 
2.21.1

[PATCH v2 06/15] qapi: Assert incomplete object occurs only in dealloc visitor

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/devel/qapi-code-gen.txt | 2 ++
 include/qapi/visitor.h       | 5 +++++
 qapi/qapi-visit-core.c       | 5 +++++
 scripts/qapi/visit.py        | 4 ++++
 4 files changed, 16 insertions(+)

diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index 1967adfa92..c6dd1891c3 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -1446,6 +1446,8 @@ Example:
             goto out;
         }
         if (!*obj) {
+            /* incomplete */
+            assert(visit_is_dealloc(v));
             goto out_obj;
         }
         visit_type_UserDefOne_members(v, *obj, &err);
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index a425ea514c..2d40d2fe0f 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -479,6 +479,11 @@ void visit_type_enum(Visitor *v, const char *name, int *obj,
  */
 bool visit_is_input(Visitor *v);
 
+/*
+ * Check if visitor is a dealloc visitor.
+ */
+bool visit_is_dealloc(Visitor *v);
+
 /*** Visiting built-in types ***/
 
 /*
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 5365561b07..d4aac206cf 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -142,6 +142,11 @@ bool visit_is_input(Visitor *v)
     return v->type == VISITOR_INPUT;
 }
 
+bool visit_is_dealloc(Visitor *v)
+{
+    return v->type == VISITOR_DEALLOC;
+}
+
 void visit_type_int(Visitor *v, const char *name, int64_t *obj, Error **errp)
 {
     assert(obj);
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 23d9194aa4..e3467b770b 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -189,6 +189,8 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
         goto out;
     }
     if (!*obj) {
+        /* incomplete */
+        assert(visit_is_dealloc(v));
         goto out_obj;
     }
     switch ((*obj)->type) {
@@ -260,6 +262,8 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
         goto out;
     }
     if (!*obj) {
+        /* incomplete */
+        assert(visit_is_dealloc(v));
         goto out_obj;
     }
     visit_type_%(c_name)s_members(v, *obj, &err);
-- 
2.21.1

Re: [PATCH v2 06/15] qapi: Assert incomplete object occurs only in dealloc visitor

[Eric Blake]

On 4/24/20 3:43 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Double S-o-b is odd.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 06/15] qapi: Assert incomplete object occurs only in dealloc visitor

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 4/24/20 3:43 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Double S-o-b is odd.

Accident, will tidy up.  Thanks!

[PATCH v2 07/15] qapi: Fix Visitor contract for start_alternate()

[Markus Armbruster]

The contract demands v->start_alternate() for input and dealloc
visitors, but visit_start_alternate() actually requires it for input
and clone visitors.  Fix the contract, and delete superfluous
qapi_dealloc_start_alternate().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/qapi/visitor-impl.h | 5 ++---
 qapi/qapi-dealloc-visitor.c | 7 -------
 2 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 8ccb3b6c20..252206dc0d 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -67,13 +67,12 @@ struct Visitor
     /* Must be set */
     void (*end_list)(Visitor *v, void **list);
 
-    /* Must be set by input and dealloc visitors to visit alternates;
-     * optional for output visitors. */
+    /* Must be set by input and clone visitors to visit alternates */
     void (*start_alternate)(Visitor *v, const char *name,
                             GenericAlternate **obj, size_t size,
                             Error **errp);
 
-    /* Optional, needed for dealloc visitor */
+    /* Optional */
     void (*end_alternate)(Visitor *v, void **obj);
 
     /* Must be set */
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index d192724b13..2239fc6417 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -34,12 +34,6 @@ static void qapi_dealloc_end_struct(Visitor *v, void **obj)
     }
 }
 
-static void qapi_dealloc_start_alternate(Visitor *v, const char *name,
-                                         GenericAlternate **obj, size_t size,
-                                         Error **errp)
-{
-}
-
 static void qapi_dealloc_end_alternate(Visitor *v, void **obj)
 {
     if (obj) {
@@ -123,7 +117,6 @@ Visitor *qapi_dealloc_visitor_new(void)
     v->visitor.type = VISITOR_DEALLOC;
     v->visitor.start_struct = qapi_dealloc_start_struct;
     v->visitor.end_struct = qapi_dealloc_end_struct;
-    v->visitor.start_alternate = qapi_dealloc_start_alternate;
     v->visitor.end_alternate = qapi_dealloc_end_alternate;
     v->visitor.start_list = qapi_dealloc_start_list;
     v->visitor.next_list = qapi_dealloc_next_list;
-- 
2.21.1

[PATCH v2 08/15] qapi: Assert output visitors see only valid enum values

[Markus Armbruster]

output_type_enum() fails when *obj is not a valid value of the enum
type.  Should not happen.  Drop the check, along with its unit tests.
qapi_enum_lookup()'s assertion still guards.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qapi/qapi-visit-core.c              |  9 -------
 tests/test-qobject-output-visitor.c | 39 -----------------------------
 tests/test-string-output-visitor.c  | 19 --------------
 3 files changed, 67 deletions(-)

diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index d4aac206cf..80ca83bcb9 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -341,15 +341,6 @@ static void output_type_enum(Visitor *v, const char *name, int *obj,
     int value = *obj;
     char *enum_str;
 
-    /*
-     * TODO why is this an error, not an assertion?  If assertion:
-     * delete, and rely on qapi_enum_lookup()
-     */
-    if (value < 0 || value >= lookup->size) {
-        error_setg(errp, QERR_INVALID_PARAMETER, name ? name : "null");
-        return;
-    }
-
     enum_str = (char *)qapi_enum_lookup(lookup, value);
     visit_type_str(v, name, &enum_str, errp);
 }
diff --git a/tests/test-qobject-output-visitor.c b/tests/test-qobject-output-visitor.c
index d7761ebf84..1c856d9bd2 100644
--- a/tests/test-qobject-output-visitor.c
+++ b/tests/test-qobject-output-visitor.c
@@ -141,21 +141,6 @@ static void test_visitor_out_enum(TestOutputVisitorData *data,
     }
 }
 
-static void test_visitor_out_enum_errors(TestOutputVisitorData *data,
-                                         const void *unused)
-{
-    EnumOne i, bad_values[] = { ENUM_ONE__MAX, -1 };
-
-    for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
-        Error *err = NULL;
-
-        visit_type_EnumOne(data->ov, "unused", &bad_values[i], &err);
-        error_free_or_abort(&err);
-        visitor_reset(data);
-    }
-}
-
-
 static void test_visitor_out_struct(TestOutputVisitorData *data,
                                     const void *unused)
 {
@@ -234,26 +219,6 @@ static void test_visitor_out_struct_nested(TestOutputVisitorData *data,
     qapi_free_UserDefTwo(ud2);
 }
 
-static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
-                                           const void *unused)
-{
-    EnumOne bad_values[] = { ENUM_ONE__MAX, -1 };
-    UserDefOne u = {0};
-    UserDefOne *pu = &u;
-    int i;
-
-    for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
-        Error *err = NULL;
-
-        u.has_enum1 = true;
-        u.enum1 = bad_values[i];
-        visit_type_UserDefOne(data->ov, "unused", &pu, &err);
-        error_free_or_abort(&err);
-        visitor_reset(data);
-    }
-}
-
-
 static void test_visitor_out_list(TestOutputVisitorData *data,
                                   const void *unused)
 {
@@ -821,14 +786,10 @@ int main(int argc, char **argv)
                             &out_visitor_data, test_visitor_out_no_string);
     output_visitor_test_add("/visitor/output/enum",
                             &out_visitor_data, test_visitor_out_enum);
-    output_visitor_test_add("/visitor/output/enum-errors",
-                            &out_visitor_data, test_visitor_out_enum_errors);
     output_visitor_test_add("/visitor/output/struct",
                             &out_visitor_data, test_visitor_out_struct);
     output_visitor_test_add("/visitor/output/struct-nested",
                             &out_visitor_data, test_visitor_out_struct_nested);
-    output_visitor_test_add("/visitor/output/struct-errors",
-                            &out_visitor_data, test_visitor_out_struct_errors);
     output_visitor_test_add("/visitor/output/list",
                             &out_visitor_data, test_visitor_out_list);
     output_visitor_test_add("/visitor/output/any",
diff --git a/tests/test-string-output-visitor.c b/tests/test-string-output-visitor.c
index 1be1540767..3bd732222c 100644
--- a/tests/test-string-output-visitor.c
+++ b/tests/test-string-output-visitor.c
@@ -203,19 +203,6 @@ static void test_visitor_out_enum(TestOutputVisitorData *data,
     }
 }
 
-static void test_visitor_out_enum_errors(TestOutputVisitorData *data,
-                                         const void *unused)
-{
-    EnumOne i, bad_values[] = { ENUM_ONE__MAX, -1 };
-
-    for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
-        Error *err = NULL;
-
-        visit_type_EnumOne(data->ov, "unused", &bad_values[i], &err);
-        error_free_or_abort(&err);
-    }
-}
-
 static void
 output_visitor_test_add(const char *testpath,
                         TestOutputVisitorData *data,
@@ -260,12 +247,6 @@ int main(int argc, char **argv)
                             &out_visitor_data, test_visitor_out_enum, false);
     output_visitor_test_add("/string-visitor/output/enum-human",
                             &out_visitor_data, test_visitor_out_enum, true);
-    output_visitor_test_add("/string-visitor/output/enum-errors",
-                            &out_visitor_data, test_visitor_out_enum_errors,
-                            false);
-    output_visitor_test_add("/string-visitor/output/enum-errors-human",
-                            &out_visitor_data, test_visitor_out_enum_errors,
-                            true);
     output_visitor_test_add("/string-visitor/output/intList",
                             &out_visitor_data, test_visitor_out_intList, false);
     output_visitor_test_add("/string-visitor/output/intList-human",
-- 
2.21.1

[PATCH v2 09/15] qapi: Assert non-input visitors see only valid narrow integers

[Markus Armbruster]

visit_type_intN() and visit_type_uintN() fail when the value is out of
bounds.

This is appropriate with an input visitor: the value comes from input,
and input may be bad.

It should never happen with the other visitors: the value comes from
the caller, and callers must keep it within bounds.  Assert that.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qapi/qapi-visit-core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 80ca83bcb9..74aa9c04bd 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -160,10 +160,13 @@ static void visit_type_uintN(Visitor *v, uint64_t *obj, const char *name,
     Error *err = NULL;
     uint64_t value = *obj;
 
+    assert(v->type == VISITOR_INPUT || value <= max);
+
     v->type_uint64(v, name, &value, &err);
     if (err) {
         error_propagate(errp, err);
     } else if (value > max) {
+        assert(v->type == VISITOR_INPUT);
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    name ? name : "null", type);
     } else {
@@ -219,10 +222,13 @@ static void visit_type_intN(Visitor *v, int64_t *obj, const char *name,
     Error *err = NULL;
     int64_t value = *obj;
 
+    assert(v->type == VISITOR_INPUT || (value >= min && value <= max));
+
     v->type_int64(v, name, &value, &err);
     if (err) {
         error_propagate(errp, err);
     } else if (value < min || value > max) {
+        assert(v->type == VISITOR_INPUT);
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    name ? name : "null", type);
     } else {
-- 
2.21.1

[PATCH v2 10/15] qapi: Clean up visitor's recovery from input with invalid type

[Markus Armbruster]

An alternate type's visit_type_FOO() fails when it runs into an
invalid ->type.  If it's an input visit, we then need to free the the
object we got from visit_start_alternate().  We do that with
qapi_free_FOO(), which uses the dealloc visitor.

Trouble is that object is in a bad state: its ->type is invalid.  So
the dealloc visitor will run into the same error again, and the error
recovery skips deallocating the alternate's (invalid) alternative.
Works, because qapi_free_FOO() ignores the error.

Avoid it instead: free the messed up object with by g_free().

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

diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index e3467b770b..678109dfb5 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -234,6 +234,9 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
     default:
         error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "%(name)s");
+        /* Avoid passing invalid *obj to qapi_free_%(c_name)s() */
+        g_free(*obj);
+        *obj = NULL;
     }
 out_obj:
     visit_end_alternate(v, (void **)obj);
-- 
2.21.1

Re: [PATCH v2 10/15] qapi: Clean up visitor's recovery from input with invalid type

[Eric Blake]

On 4/24/20 3:43 AM, Markus Armbruster wrote:
> An alternate type's visit_type_FOO() fails when it runs into an
> invalid ->type.  If it's an input visit, we then need to free the the
> object we got from visit_start_alternate().  We do that with
> qapi_free_FOO(), which uses the dealloc visitor.
> 
> Trouble is that object is in a bad state: its ->type is invalid.  So
> the dealloc visitor will run into the same error again, and the error
> recovery skips deallocating the alternate's (invalid) alternative.
> Works, because qapi_free_FOO() ignores the error.
> 
> Avoid it instead: free the messed up object with by g_free().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   scripts/qapi/visit.py | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> index e3467b770b..678109dfb5 100644
> --- a/scripts/qapi/visit.py
> +++ b/scripts/qapi/visit.py
> @@ -234,6 +234,9 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
>       default:
>           error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                      "%(name)s");
> +        /* Avoid passing invalid *obj to qapi_free_%(c_name)s() */
> +        g_free(*obj);
> +        *obj = NULL;

Yep, fixes the problem I spotted in v1, and much easier to review with 
the reduced scope of change.

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH v2 11/15] qapi: Assert non-input visitors see only valid alternate tags

[Markus Armbruster]

An alternate type's visit_type_FOO() fails when it runs into an
invalid ->type.

This is appropriate with an input visitor: visit_start_alternate()
sets ->type according to the input, and bad input can lead to bad
->type.

It should never happen with an output, clone or dealloc visitor: if it
did, the alternate being output, cloned or deallocated would be messed
up beyond repair.  Assert that.

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

diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 678109dfb5..d5d7a1031f 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -232,6 +232,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
     case QTYPE_NONE:
         abort();
     default:
+        assert(visit_is_input(v));
         error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "%(name)s");
         /* Avoid passing invalid *obj to qapi_free_%(c_name)s() */
-- 
2.21.1

[PATCH v2 12/15] qapi: Only input visitors can actually fail

[Markus Armbruster]

The previous few commits have made this more obvious, and removed the
one exception.  Time to clarify the documentation, and drop dead error
checking.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/qapi/visitor-impl.h |  4 ++++
 include/qapi/visitor.h      | 40 ++++++++++++++++++++++---------------
 block.c                     |  9 +--------
 block/sheepdog.c            |  9 +--------
 blockdev.c                  | 16 ++-------------
 hw/core/machine-hmp-cmds.c  |  2 +-
 monitor/hmp-cmds.c          |  3 ++-
 7 files changed, 35 insertions(+), 48 deletions(-)

diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 252206dc0d..98dc533d39 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -43,6 +43,10 @@ typedef enum VisitorType {
 
 struct Visitor
 {
+    /*
+     * Only input visitors may fail!
+     */
+
     /* Must be set to visit structs */
     void (*start_struct)(Visitor *v, const char *name, void **obj,
                          size_t size, Error **errp);
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 2d40d2fe0f..5573906966 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -82,7 +82,7 @@
  * Each function also takes the customary @errp argument (see
  * qapi/error.h for details), for reporting any errors (such as if a
  * member @name is not present, or is present but not the specified
- * type).
+ * type).  Only input visitors can fail.
  *
  * If an error is detected during visit_type_FOO() with an input
  * visitor, then *@obj will be set to NULL for pointer types, and left
@@ -164,19 +164,14 @@
  *
  * <example>
  *  Foo *f = ...obtain populated object...
- *  Error *err = NULL;
  *  Visitor *v;
  *  Type *result;
  *
  *  v = FOO_visitor_new(..., &result);
- *  visit_type_Foo(v, NULL, &f, &err);
- *  if (err) {
- *      ...handle error...
- *  } else {
- *      visit_complete(v, &result);
- *      ...use result...
- *  }
+ *  visit_type_Foo(v, NULL, &f, &error_abort);
+ *  visit_complete(v, &result);
  *  visit_free(v);
+ *  ...use result...
  * </example>
  *
  * It is also possible to use the visitors to do a virtual walk, where
@@ -289,6 +284,7 @@ void visit_free(Visitor *v);
  * case @size is ignored.
  *
  * On failure, set *@obj to NULL and store an error through @errp.
+ * Can happen only when @v is an input visitor.
  *
  * After visit_start_struct() succeeds, the caller may visit its
  * members one after the other, passing the member's name and address
@@ -305,7 +301,8 @@ void visit_start_struct(Visitor *v, const char *name, void **obj,
 /*
  * Prepare for completing an object visit.
  *
- * On failure, store an error through @errp.
+ * On failure, store an error through @errp.  Can happen only when @v
+ * is an input visitor.
  *
  * Should be called prior to visit_end_struct() if all other
  * intermediate visit steps were successful, to allow the visitor one
@@ -342,6 +339,7 @@ void visit_end_struct(Visitor *v, void **obj);
  * ignored.
  *
  * On failure, set *@list to NULL and store an error through @errp.
+ * Can happen only when @v is an input visitor.
  *
  * After visit_start_list() succeeds, the caller may visit its members
  * one after the other.  A real visit (where @list is non-NULL) uses
@@ -375,7 +373,8 @@ GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size);
 /*
  * Prepare for completing a list visit.
  *
- * On failure, store an error through @errp.
+ * On failure, store an error through @errp.  Can happen only when @v
+ * is an input visitor.
  *
  * Should be called prior to visit_end_list() if all other
  * intermediate visit steps were successful, to allow the visitor one
@@ -411,6 +410,7 @@ void visit_end_list(Visitor *v, void **list);
  * (*@obj)->type.  Other visitors leave @obj unchanged.
  *
  * On failure, set *@obj to NULL and store an error through @errp.
+ * Can happen only when @v is an input visitor.
  *
  * If successful, this must be paired with visit_end_alternate() with
  * the same @obj to clean up, even if visiting the contents of the
@@ -465,11 +465,13 @@ bool visit_optional(Visitor *v, const char *name, bool *present);
  * visitors produce text output.  The mapping between enumeration
  * values and strings is done by the visitor core, using @lookup.
  *
- * On failure, store an error through @errp.
+ * On failure, store an error through @errp.  Can happen only when @v
+ * is an input visitor.
  *
  * May call visit_type_str() under the hood, and the enum visit may
  * fail even if the corresponding string visit succeeded; this implies
- * that visit_type_str() must have no unwelcome side effects.
+ * that an input visitor's visit_type_str() must have no unwelcome
+ * side effects.
  */
 void visit_type_enum(Visitor *v, const char *name, int *obj,
                      const QEnumLookup *lookup, Error **errp);
@@ -495,7 +497,8 @@ bool visit_is_dealloc(Visitor *v);
  * @obj must be non-NULL.  Input visitors set *@obj to the value;
  * other visitors will leave *@obj unchanged.
  *
- * On failure, store an error through @errp.
+ * On failure, store an error through @errp.  Can happen only when @v
+ * is an input visitor.
  */
 void visit_type_int(Visitor *v, const char *name, int64_t *obj, Error **errp);
 
@@ -573,7 +576,8 @@ void visit_type_size(Visitor *v, const char *name, uint64_t *obj,
  * @obj must be non-NULL.  Input visitors set *@obj to the value;
  * other visitors will leave *@obj unchanged.
  *
- * On failure, store an error through @errp.
+ * On failure, store an error through @errp.  Can happen only when @v
+ * is an input visitor.
  */
 void visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp);
 
@@ -592,6 +596,7 @@ void visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp);
  * into @obj for use by an output visitor.
  *
  * On failure, set *@obj to NULL and store an error through @errp.
+ * Can happen only when @v is an input visitor.
  *
  * FIXME: Callers that try to output NULL *obj should not be allowed.
  */
@@ -607,7 +612,8 @@ void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp);
  * other visitors will leave *@obj unchanged.  Visitors should
  * document if infinity or NaN are not permitted.
  *
- * On failure, store an error through @errp.
+ * On failure, store an error through @errp.  Can happen only when @v
+ * is an input visitor.
  */
 void visit_type_number(Visitor *v, const char *name, double *obj,
                        Error **errp);
@@ -623,6 +629,7 @@ void visit_type_number(Visitor *v, const char *name, double *obj,
  * for output visitors.
  *
  * On failure, set *@obj to NULL and store an error through @errp.
+ * Can happen only when @v is an input visitor.
  *
  * Note that some kinds of input can't express arbitrary QObject.
  * E.g. the visitor returned by qobject_input_visitor_new_keyval()
@@ -640,6 +647,7 @@ void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp);
  * other visitors ignore *@obj.
  *
  * On failure, set *@obj to NULL and store an error through @errp.
+ * Can happen only when @v is an input visitor.
  */
 void visit_type_null(Visitor *v, const char *name, QNull **obj,
                      Error **errp);
diff --git a/block.c b/block.c
index 2e3905c99e..c11385ae05 100644
--- a/block.c
+++ b/block.c
@@ -2982,7 +2982,6 @@ BdrvChild *bdrv_open_child(const char *filename,
 BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp)
 {
     BlockDriverState *bs = NULL;
-    Error *local_err = NULL;
     QObject *obj = NULL;
     QDict *qdict = NULL;
     const char *reference = NULL;
@@ -2995,11 +2994,7 @@ BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp)
         assert(ref->type == QTYPE_QDICT);
 
         v = qobject_output_visitor_new(&obj);
-        visit_type_BlockdevOptions(v, NULL, &options, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            goto fail;
-        }
+        visit_type_BlockdevOptions(v, NULL, &options, &error_abort);
         visit_complete(v, &obj);
 
         qdict = qobject_to(QDict, obj);
@@ -3017,8 +3012,6 @@ BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp)
 
     bs = bdrv_open_inherit(NULL, reference, qdict, 0, NULL, NULL, errp);
     obj = NULL;
-
-fail:
     qobject_unref(obj);
     visit_free(v);
     return bs;
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 59f7ebb171..5f3aead038 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1854,19 +1854,12 @@ static int sd_create_prealloc(BlockdevOptionsSheepdog *location, int64_t size,
     Visitor *v;
     QObject *obj = NULL;
     QDict *qdict;
-    Error *local_err = NULL;
     int ret;
 
     v = qobject_output_visitor_new(&obj);
-    visit_type_BlockdevOptionsSheepdog(v, NULL, &location, &local_err);
+    visit_type_BlockdevOptionsSheepdog(v, NULL, &location, &error_abort);
     visit_free(v);
 
-    if (local_err) {
-        error_propagate(errp, local_err);
-        qobject_unref(obj);
-        return -EINVAL;
-    }
-
     qdict = qobject_to(QDict, obj);
     qdict_flatten(qdict);
 
diff --git a/blockdev.c b/blockdev.c
index 5faddaa705..9da960b1e7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3725,14 +3725,8 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
     QObject *obj;
     Visitor *v = qobject_output_visitor_new(&obj);
     QDict *qdict;
-    Error *local_err = NULL;
-
-    visit_type_BlockdevOptions(v, NULL, &options, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        goto fail;
-    }
 
+    visit_type_BlockdevOptions(v, NULL, &options, &error_abort);
     visit_complete(v, &obj);
     qdict = qobject_to(QDict, obj);
 
@@ -3760,7 +3754,6 @@ void qmp_x_blockdev_reopen(BlockdevOptions *options, Error **errp)
     AioContext *ctx;
     QObject *obj;
     Visitor *v = qobject_output_visitor_new(&obj);
-    Error *local_err = NULL;
     BlockReopenQueue *queue;
     QDict *qdict;
 
@@ -3777,12 +3770,7 @@ void qmp_x_blockdev_reopen(BlockdevOptions *options, Error **errp)
     }
 
     /* Put all options in a QDict and flatten it */
-    visit_type_BlockdevOptions(v, NULL, &options, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        goto fail;
-    }
-
+    visit_type_BlockdevOptions(v, NULL, &options, &error_abort);
     visit_complete(v, &obj);
     qdict = qobject_to(QDict, obj);
 
diff --git a/hw/core/machine-hmp-cmds.c b/hw/core/machine-hmp-cmds.c
index b76f7223af..39999c47c5 100644
--- a/hw/core/machine-hmp-cmds.c
+++ b/hw/core/machine-hmp-cmds.c
@@ -113,7 +113,7 @@ void hmp_info_memdev(Monitor *mon, const QDict *qdict)
 
     while (m) {
         v = string_output_visitor_new(false, &str);
-        visit_type_uint16List(v, NULL, &m->value->host_nodes, NULL);
+        visit_type_uint16List(v, NULL, &m->value->host_nodes, &error_abort);
         monitor_printf(mon, "memory backend: %s\n", m->value->id);
         monitor_printf(mon, "  size:  %" PRId64 "\n", m->value->size);
         monitor_printf(mon, "  merge: %s\n",
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 9b94e67879..7f6e982dc8 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -334,7 +334,8 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
         Visitor *v;
         char *str;
         v = string_output_visitor_new(false, &str);
-        visit_type_uint32List(v, NULL, &info->postcopy_vcpu_blocktime, NULL);
+        visit_type_uint32List(v, NULL, &info->postcopy_vcpu_blocktime,
+                              &error_abort);
         visit_complete(v, &str);
         monitor_printf(mon, "postcopy vcpu blocktime: %s\n", str);
         g_free(str);
-- 
2.21.1

[PATCH v2 13/15] qom: Simplify object_property_get_enum()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qom/object.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 1812f79224..be700e831f 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1550,11 +1550,9 @@ int object_property_get_enum(Object *obj, const char *name,
     }
     visit_complete(v, &str);
     visit_free(v);
-    v = string_input_visitor_new(str);
-    visit_type_enum(v, name, &ret, enumprop->lookup, errp);
 
+    ret = qapi_enum_parse(enumprop->lookup, str, -1, errp);
     g_free(str);
-    visit_free(v);
 
     return ret;
 }
-- 
2.21.1

[PATCH v2 14/15] qapi: Disallow qmp_marshal_FOO(NULL, ...)

[Markus Armbruster]

For QMP commands without arguments, gen_marshal() laboriously
generates a qmp_marshal_FOO() that copes with null @args.  Turns
there's just one caller that passes null instead of an empty QDict.
Adjust that caller, and simplify gen_marshal().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/devel/qapi-code-gen.txt |  2 +-
 monitor/qmp.c                |  5 ++++-
 scripts/qapi/commands.py     | 26 ++------------------------
 3 files changed, 7 insertions(+), 26 deletions(-)

diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index c6dd1891c3..a7794ef658 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -1579,8 +1579,8 @@ Example:
     void qmp_marshal_my_command(QDict *args, QObject **ret, Error **errp)
     {
         Error *err = NULL;
-        UserDefOne *retval;
         Visitor *v;
+        UserDefOne *retval;
         q_obj_my_command_arg arg = {0};
 
         v = qobject_input_visitor_new(QOBJECT(args));
diff --git a/monitor/qmp.c b/monitor/qmp.c
index f89e7daf27..d433ceae5b 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -322,9 +322,12 @@ static QDict *qmp_greeting(MonitorQMP *mon)
 {
     QList *cap_list = qlist_new();
     QObject *ver = NULL;
+    QDict *args;
     QMPCapability cap;
 
-    qmp_marshal_query_version(NULL, &ver, NULL);
+    args = qdict_new();
+    qmp_marshal_query_version(args, &ver, NULL);
+    qobject_unref(args);
 
     for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++) {
         if (mon->capab_offered[cap]) {
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index bc30876c88..f545903567 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -104,6 +104,7 @@ def gen_marshal(name, arg_type, boxed, ret_type):
 %(proto)s
 {
     Error *err = NULL;
+    Visitor *v;
 ''',
                 proto=build_marshal_proto(name))
 
@@ -117,21 +118,14 @@ def gen_marshal(name, arg_type, boxed, ret_type):
         visit_members = ('visit_type_%s_members(v, &arg, &err);'
                          % arg_type.c_name())
         ret += mcgen('''
-    Visitor *v;
     %(c_name)s arg = {0};
-
 ''',
                      c_name=arg_type.c_name())
     else:
         visit_members = ''
-        ret += mcgen('''
-    Visitor *v = NULL;
-
-    if (args) {
-''')
-        push_indent()
 
     ret += mcgen('''
+
     v = qobject_input_visitor_new(QOBJECT(args));
     visit_start_struct(v, NULL, NULL, 0, &err);
     if (err) {
@@ -148,12 +142,6 @@ def gen_marshal(name, arg_type, boxed, ret_type):
 ''',
                  visit_members=visit_members)
 
-    if not have_args:
-        pop_indent()
-        ret += mcgen('''
-    }
-''')
-
     ret += gen_call(name, arg_type, boxed, ret_type)
 
     ret += mcgen('''
@@ -168,10 +156,6 @@ out:
                          % arg_type.c_name())
     else:
         visit_members = ''
-        ret += mcgen('''
-    if (args) {
-''')
-        push_indent()
 
     ret += mcgen('''
     v = qapi_dealloc_visitor_new();
@@ -182,12 +166,6 @@ out:
 ''',
                  visit_members=visit_members)
 
-    if not have_args:
-        pop_indent()
-        ret += mcgen('''
-    }
-''')
-
     ret += mcgen('''
 }
 ''')
-- 
2.21.1

Re: [PATCH v2 14/15] qapi: Disallow qmp_marshal_FOO(NULL, ...)

[Eric Blake]

On 4/24/20 3:43 AM, Markus Armbruster wrote:
> For QMP commands without arguments, gen_marshal() laboriously
> generates a qmp_marshal_FOO() that copes with null @args.  Turns
> there's just one caller that passes null instead of an empty QDict.
> Adjust that caller, and simplify gen_marshal().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   docs/devel/qapi-code-gen.txt |  2 +-
>   monitor/qmp.c                |  5 ++++-
>   scripts/qapi/commands.py     | 26 ++------------------------
>   3 files changed, 7 insertions(+), 26 deletions(-)
> 

Fun diffstat.

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH v2 15/15] qapi: Generate simpler marshalling code when no arguments

[Markus Armbruster]

When command FOO has no arguments, its generated qmp_marshal_FOO() is
a bit confusing.  Make it simpler:

     visit_start_struct(v, NULL, NULL, 0, &err);
     if (err) {
         goto out;
     }
-
-    if (!err) {
-        visit_check_struct(v, &err);
-    }
+    visit_check_struct(v, &err);
     visit_end_struct(v, NULL);
     if (err) {
         goto out;
     }

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/commands.py | 40 ++++++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index f545903567..6809b0fb6e 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -115,14 +115,10 @@ def gen_marshal(name, arg_type, boxed, ret_type):
                      c_type=ret_type.c_type())
 
     if have_args:
-        visit_members = ('visit_type_%s_members(v, &arg, &err);'
-                         % arg_type.c_name())
         ret += mcgen('''
     %(c_name)s arg = {0};
 ''',
                      c_name=arg_type.c_name())
-    else:
-        visit_members = ''
 
     ret += mcgen('''
 
@@ -131,16 +127,27 @@ def gen_marshal(name, arg_type, boxed, ret_type):
     if (err) {
         goto out;
     }
-    %(visit_members)s
+''')
+
+    if have_args:
+        ret += mcgen('''
+    visit_type_%(c_arg_type)s_members(v, &arg, &err);
     if (!err) {
         visit_check_struct(v, &err);
     }
+''',
+                     c_arg_type=arg_type.c_name())
+    else:
+        ret += mcgen('''
+    visit_check_struct(v, &err);
+''')
+
+    ret += mcgen('''
     visit_end_struct(v, NULL);
     if (err) {
         goto out;
     }
-''',
-                 visit_members=visit_members)
+''')
 
     ret += gen_call(name, arg_type, boxed, ret_type)
 
@@ -151,20 +158,21 @@ out:
     visit_free(v);
 ''')
 
-    if have_args:
-        visit_members = ('visit_type_%s_members(v, &arg, NULL);'
-                         % arg_type.c_name())
-    else:
-        visit_members = ''
-
     ret += mcgen('''
     v = qapi_dealloc_visitor_new();
     visit_start_struct(v, NULL, NULL, 0, NULL);
-    %(visit_members)s
+''')
+
+    if have_args:
+        ret += mcgen('''
+    visit_type_%(c_arg_type)s_members(v, &arg, NULL);
+''',
+                     c_arg_type=arg_type.c_name())
+
+    ret += mcgen('''
     visit_end_struct(v, NULL);
     visit_free(v);
-''',
-                 visit_members=visit_members)
+''')
 
     ret += mcgen('''
 }
-- 
2.21.1

Re: [PATCH v2 15/15] qapi: Generate simpler marshalling code when no arguments

[Eric Blake]

On 4/24/20 3:43 AM, Markus Armbruster wrote:
> When command FOO has no arguments, its generated qmp_marshal_FOO() is
> a bit confusing.  Make it simpler:
> 
>       visit_start_struct(v, NULL, NULL, 0, &err);
>       if (err) {
>           goto out;
>       }
> -
> -    if (!err) {
> -        visit_check_struct(v, &err);
> -    }
> +    visit_check_struct(v, &err);
>       visit_end_struct(v, NULL);
>       if (err) {
>           goto out;
>       }
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   scripts/qapi/commands.py | 40 ++++++++++++++++++++++++----------------
>   1 file changed, 24 insertions(+), 16 deletions(-)
> 

A bit more complex in the generator, but the generated code is indeed 
better.

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 00/15] qapi: Spring cleaning

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200424084338.26803-1-armbru@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

-...........................
+........................E..
+======================================================================
+ERROR: test_stream_pause (__main__.TestSingleDrive)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "030", line 93, in test_stream_pause
---
 Ran 27 tests
 
-OK
+FAILED (errors=1)
Could not access KVM kernel module: No such file or directory
qemu-system-x86_64: -accel kvm: failed to initialize kvm: No such file or directory
qemu-system-x86_64: falling back to tcg
---
Not run: 259
Failures: 030
Failed 1 of 117 iotests
make: *** [check-tests/check-block.sh] Error 1
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
    sys.exit(main())
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=a65cca4cfb0f47d6afb0146008cfda36', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-eyoly1_s/src/docker-src.2020-04-24-05.52.13.3890:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=a65cca4cfb0f47d6afb0146008cfda36
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-eyoly1_s/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    14m41.321s
user    0m9.235s


The full log is available at
http://patchew.org/logs/20200424084338.26803-1-armbru@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v2 00/15] qapi: Spring cleaning

[Eric Blake]

On 4/24/20 5:06 AM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200424084338.26803-1-armbru@redhat.com/
> 
> 
> 
> Hi,
> 
> This series failed the docker-quick@centos7 build test. Please find the testing commands and
> their output below. If you have Docker installed, you can probably reproduce it
> locally.
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> make docker-image-centos7 V=1 NETWORK=1
> time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
> === TEST SCRIPT END ===
> 
> -...........................
> +........................E..
> +======================================================================
> +ERROR: test_stream_pause (__main__.TestSingleDrive)
> +----------------------------------------------------------------------
> +Traceback (most recent call last):
> +  File "030", line 93, in test_stream_pause

Not sure how this would be related to your series, or if it is a 
spurious flakiness in the test that just chose to hit now.  iotest 30 is 
passing fine for me even with your series applied.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 00/15] qapi: Spring cleaning

[Markus Armbruster]

Queued.