* [PATCH 00/13] qapi: Spring cleaning
@ 2020-04-23 16:00 Markus Armbruster
2020-04-23 16:00 ` [PATCH 01/13] qapi: Belatedly update visitor.h's big comment for QAPI modules Markus Armbruster
` (12 more replies)
0 siblings, 13 replies; 30+ messages in thread
From: Markus Armbruster @ 2020-04-23 16:00 UTC (permalink / raw)
To: qemu-devel; +Cc: mdroth
Markus Armbruster (13):
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 seeg 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()
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 +-
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/visit.py | 7 +-
13 files changed, 141 insertions(+), 195 deletions(-)
--
2.21.1
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 01/13] qapi: Belatedly update visitor.h's big comment for QAPI modules
2020-04-23 16:00 [PATCH 00/13] qapi: Spring cleaning Markus Armbruster
@ 2020-04-23 16:00 ` Markus Armbruster
2020-04-23 17:33 ` Eric Blake
2020-04-23 16:00 ` [PATCH 02/13] qapi: Fix the virtual walk example in visitor.h's big comment Markus Armbruster
` (11 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Markus Armbruster @ 2020-04-23 16:00 UTC (permalink / raw)
To: qemu-devel; +Cc: mdroth
Signed-off-by: Markus Armbruster <armbru@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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 02/13] qapi: Fix the virtual walk example in visitor.h's big comment
2020-04-23 16:00 [PATCH 00/13] qapi: Spring cleaning Markus Armbruster
2020-04-23 16:00 ` [PATCH 01/13] qapi: Belatedly update visitor.h's big comment for QAPI modules Markus Armbruster
@ 2020-04-23 16:00 ` Markus Armbruster
2020-04-23 17:33 ` Eric Blake
2020-04-23 16:00 ` [PATCH 03/13] qapi: Fix typo in visit_start_list()'s contract Markus Armbruster
` (10 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Markus Armbruster @ 2020-04-23 16:00 UTC (permalink / raw)
To: qemu-devel; +Cc: mdroth
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>
---
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 03/13] qapi: Fix typo in visit_start_list()'s contract
2020-04-23 16:00 [PATCH 00/13] qapi: Spring cleaning Markus Armbruster
2020-04-23 16:00 ` [PATCH 01/13] qapi: Belatedly update visitor.h's big comment for QAPI modules Markus Armbruster
2020-04-23 16:00 ` [PATCH 02/13] qapi: Fix the virtual walk example in visitor.h's big comment Markus Armbruster
@ 2020-04-23 16:00 ` Markus Armbruster
2020-04-23 17:34 ` Eric Blake
2020-04-23 16:00 ` [PATCH 04/13] qapi: Document @errp usage more thoroughly in visitor.h Markus Armbruster
` (9 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Markus Armbruster @ 2020-04-23 16:00 UTC (permalink / raw)
To: qemu-devel; +Cc: mdroth
Signed-off-by: Markus Armbruster <armbru@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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 04/13] qapi: Document @errp usage more thoroughly in visitor.h
2020-04-23 16:00 [PATCH 00/13] qapi: Spring cleaning Markus Armbruster
` (2 preceding siblings ...)
2020-04-23 16:00 ` [PATCH 03/13] qapi: Fix typo in visit_start_list()'s contract Markus Armbruster
@ 2020-04-23 16:00 ` Markus Armbruster
2020-04-23 17:35 ` Eric Blake
2020-04-23 16:00 ` [PATCH 05/13] qapi: Polish prose " Markus Armbruster
` (8 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Markus Armbruster @ 2020-04-23 16:00 UTC (permalink / raw)
To: qemu-devel; +Cc: mdroth
Signed-off-by: Markus Armbruster <armbru@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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 05/13] qapi: Polish prose in visitor.h
2020-04-23 16:00 [PATCH 00/13] qapi: Spring cleaning Markus Armbruster
` (3 preceding siblings ...)
2020-04-23 16:00 ` [PATCH 04/13] qapi: Document @errp usage more thoroughly in visitor.h Markus Armbruster
@ 2020-04-23 16:00 ` Markus Armbruster
2020-04-23 17:51 ` Eric Blake
2020-04-23 16:00 ` [PATCH 06/13] qapi: Assert incomplete object occurs only in dealloc visitor Markus Armbruster
` (7 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Markus Armbruster @ 2020-04-23 16:00 UTC (permalink / raw)
To: qemu-devel; +Cc: mdroth
Signed-off-by: Markus Armbruster <armbru@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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 06/13] qapi: Assert incomplete object occurs only in dealloc visitor
2020-04-23 16:00 [PATCH 00/13] qapi: Spring cleaning Markus Armbruster
` (4 preceding siblings ...)
2020-04-23 16:00 ` [PATCH 05/13] qapi: Polish prose " Markus Armbruster
@ 2020-04-23 16:00 ` Markus Armbruster
2020-04-23 17:52 ` Eric Blake
2020-04-23 16:00 ` [PATCH 07/13] qapi: Fix Visitor contract for start_alternate() Markus Armbruster
` (6 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Markus Armbruster @ 2020-04-23 16:00 UTC (permalink / raw)
To: qemu-devel; +Cc: mdroth
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
include/qapi/visitor.h | 5 +++++
qapi/qapi-visit-core.c | 5 +++++
scripts/qapi/visit.py | 4 ++++
3 files changed, 14 insertions(+)
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 07/13] qapi: Fix Visitor contract for start_alternate()
2020-04-23 16:00 [PATCH 00/13] qapi: Spring cleaning Markus Armbruster
` (5 preceding siblings ...)
2020-04-23 16:00 ` [PATCH 06/13] qapi: Assert incomplete object occurs only in dealloc visitor Markus Armbruster
@ 2020-04-23 16:00 ` Markus Armbruster
2020-04-23 17:53 ` Eric Blake
2020-04-23 16:00 ` [PATCH 08/13] qapi: Assert output visitors see only valid enum values Markus Armbruster
` (5 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Markus Armbruster @ 2020-04-23 16:00 UTC (permalink / raw)
To: qemu-devel; +Cc: mdroth
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>
---
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 08/13] qapi: Assert output visitors see only valid enum values
2020-04-23 16:00 [PATCH 00/13] qapi: Spring cleaning Markus Armbruster
` (6 preceding siblings ...)
2020-04-23 16:00 ` [PATCH 07/13] qapi: Fix Visitor contract for start_alternate() Markus Armbruster
@ 2020-04-23 16:00 ` Markus Armbruster
2020-04-23 17:56 ` Eric Blake
2020-04-23 16:00 ` [PATCH 09/13] qapi: Assert non-input visitors see only valid narrow integers Markus Armbruster
` (4 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Markus Armbruster @ 2020-04-23 16:00 UTC (permalink / raw)
To: qemu-devel; +Cc: mdroth
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>
---
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 09/13] qapi: Assert non-input visitors see only valid narrow integers
2020-04-23 16:00 [PATCH 00/13] qapi: Spring cleaning Markus Armbruster
` (7 preceding siblings ...)
2020-04-23 16:00 ` [PATCH 08/13] qapi: Assert output visitors see only valid enum values Markus Armbruster
@ 2020-04-23 16:00 ` Markus Armbruster
2020-04-23 17:57 ` Eric Blake
2020-04-23 16:00 ` [PATCH 10/13] qapi: Clean up visitor's recovery from input with invalid type Markus Armbruster
` (3 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Markus Armbruster @ 2020-04-23 16:00 UTC (permalink / raw)
To: qemu-devel; +Cc: mdroth
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>
---
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 10/13] qapi: Clean up visitor's recovery from input with invalid type
2020-04-23 16:00 [PATCH 00/13] qapi: Spring cleaning Markus Armbruster
` (8 preceding siblings ...)
2020-04-23 16:00 ` [PATCH 09/13] qapi: Assert non-input visitors see only valid narrow integers Markus Armbruster
@ 2020-04-23 16:00 ` Markus Armbruster
2020-04-23 18:06 ` Eric Blake
2020-04-23 16:00 ` [PATCH 11/13] qapi: Assert non-input visitors see only valid alternate tags Markus Armbruster
` (2 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Markus Armbruster @ 2020-04-23 16:00 UTC (permalink / raw)
To: qemu-devel; +Cc: mdroth
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.
This is a roundabout way to g_free() the alternate.
Simplify: replace the qapi_free_FOO() by g_free().
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 e3467b770b..3b28ba93f3 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -238,7 +238,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
out_obj:
visit_end_alternate(v, (void **)obj);
if (err && visit_is_input(v)) {
- qapi_free_%(c_name)s(*obj);
+ g_free(*obj);
*obj = NULL;
}
out:
--
2.21.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 11/13] qapi: Assert non-input visitors see only valid alternate tags
2020-04-23 16:00 [PATCH 00/13] qapi: Spring cleaning Markus Armbruster
` (9 preceding siblings ...)
2020-04-23 16:00 ` [PATCH 10/13] qapi: Clean up visitor's recovery from input with invalid type Markus Armbruster
@ 2020-04-23 16:00 ` Markus Armbruster
2020-04-23 18:09 ` Eric Blake
2020-04-23 16:00 ` [PATCH 12/13] qapi: Only input visitors can actually fail Markus Armbruster
2020-04-23 16:00 ` [PATCH 13/13] qom: Simplify object_property_get_enum() Markus Armbruster
12 siblings, 1 reply; 30+ messages in thread
From: Markus Armbruster @ 2020-04-23 16:00 UTC (permalink / raw)
To: qemu-devel; +Cc: mdroth
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>
---
scripts/qapi/visit.py | 1 +
1 file changed, 1 insertion(+)
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 3b28ba93f3..99b73eb7c1 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");
}
--
2.21.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 12/13] qapi: Only input visitors can actually fail
2020-04-23 16:00 [PATCH 00/13] qapi: Spring cleaning Markus Armbruster
` (10 preceding siblings ...)
2020-04-23 16:00 ` [PATCH 11/13] qapi: Assert non-input visitors see only valid alternate tags Markus Armbruster
@ 2020-04-23 16:00 ` Markus Armbruster
2020-04-23 18:31 ` Eric Blake
2020-04-23 16:00 ` [PATCH 13/13] qom: Simplify object_property_get_enum() Markus Armbruster
12 siblings, 1 reply; 30+ messages in thread
From: Markus Armbruster @ 2020-04-23 16:00 UTC (permalink / raw)
To: qemu-devel; +Cc: mdroth
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>
---
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 13/13] qom: Simplify object_property_get_enum()
2020-04-23 16:00 [PATCH 00/13] qapi: Spring cleaning Markus Armbruster
` (11 preceding siblings ...)
2020-04-23 16:00 ` [PATCH 12/13] qapi: Only input visitors can actually fail Markus Armbruster
@ 2020-04-23 16:00 ` Markus Armbruster
2020-04-23 18:40 ` Eric Blake
12 siblings, 1 reply; 30+ messages in thread
From: Markus Armbruster @ 2020-04-23 16:00 UTC (permalink / raw)
To: qemu-devel; +Cc: mdroth
Signed-off-by: Markus Armbruster <armbru@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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 01/13] qapi: Belatedly update visitor.h's big comment for QAPI modules
2020-04-23 16:00 ` [PATCH 01/13] qapi: Belatedly update visitor.h's big comment for QAPI modules Markus Armbruster
@ 2020-04-23 17:33 ` Eric Blake
0 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2020-04-23 17:33 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: mdroth
On 4/23/20 11:00 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> include/qapi/visitor.h | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 02/13] qapi: Fix the virtual walk example in visitor.h's big comment
2020-04-23 16:00 ` [PATCH 02/13] qapi: Fix the virtual walk example in visitor.h's big comment Markus Armbruster
@ 2020-04-23 17:33 ` Eric Blake
0 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2020-04-23 17:33 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: mdroth
On 4/23/20 11:00 AM, Markus Armbruster wrote:
> Call visit_check_list(). Missed in commit a4a1c70dc7 "qapi: Make
> input visitors detect unvisited list tails".
>
> Drop an irrelevant error_propagate() while there.
Aha - you found this because of your error cleanup work ;)
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> include/qapi/visitor.h | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 03/13] qapi: Fix typo in visit_start_list()'s contract
2020-04-23 16:00 ` [PATCH 03/13] qapi: Fix typo in visit_start_list()'s contract Markus Armbruster
@ 2020-04-23 17:34 ` Eric Blake
0 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2020-04-23 17:34 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: mdroth
On 4/23/20 11:00 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> include/qapi/visitor.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Too much copy-and-paste in the original ;)
Reviewed-by: Eric Blake <eblake@redhat.com>
>
> 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
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 04/13] qapi: Document @errp usage more thoroughly in visitor.h
2020-04-23 16:00 ` [PATCH 04/13] qapi: Document @errp usage more thoroughly in visitor.h Markus Armbruster
@ 2020-04-23 17:35 ` Eric Blake
0 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2020-04-23 17:35 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: mdroth
On 4/23/20 11:00 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> include/qapi/visitor.h | 37 +++++++++++++++++++++++--------------
> 1 file changed, 23 insertions(+), 14 deletions(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 05/13] qapi: Polish prose in visitor.h
2020-04-23 16:00 ` [PATCH 05/13] qapi: Polish prose " Markus Armbruster
@ 2020-04-23 17:51 ` Eric Blake
0 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2020-04-23 17:51 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: mdroth
On 4/23/20 11:00 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> include/qapi/visitor.h | 104 +++++++++++++++++++++--------------------
> 1 file changed, 54 insertions(+), 50 deletions(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 06/13] qapi: Assert incomplete object occurs only in dealloc visitor
2020-04-23 16:00 ` [PATCH 06/13] qapi: Assert incomplete object occurs only in dealloc visitor Markus Armbruster
@ 2020-04-23 17:52 ` Eric Blake
0 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2020-04-23 17:52 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: mdroth
On 4/23/20 11:00 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> include/qapi/visitor.h | 5 +++++
> qapi/qapi-visit-core.c | 5 +++++
> scripts/qapi/visit.py | 4 ++++
> 3 files changed, 14 insertions(+)
>
Nice enforcement of the contract.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 07/13] qapi: Fix Visitor contract for start_alternate()
2020-04-23 16:00 ` [PATCH 07/13] qapi: Fix Visitor contract for start_alternate() Markus Armbruster
@ 2020-04-23 17:53 ` Eric Blake
0 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2020-04-23 17:53 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: mdroth
On 4/23/20 11:00 AM, Markus Armbruster wrote:
> 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>
> ---
> include/qapi/visitor-impl.h | 5 ++---
> qapi/qapi-dealloc-visitor.c | 7 -------
> 2 files changed, 2 insertions(+), 10 deletions(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 08/13] qapi: Assert output visitors see only valid enum values
2020-04-23 16:00 ` [PATCH 08/13] qapi: Assert output visitors see only valid enum values Markus Armbruster
@ 2020-04-23 17:56 ` Eric Blake
2020-04-24 7:31 ` Markus Armbruster
0 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2020-04-23 17:56 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: mdroth
On 4/23/20 11:00 AM, Markus Armbruster wrote:
> 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>
> ---
> 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(-)
Nice cleanup.
The commit message implies adding an assertion; but in reality, the
change is deleting dead code (because we already have the assertion in
place). Maybe it's worth updating the subject?
Reviewed-by: Eric Blake <eblake@redhat.com>
> - /*
> - * 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;
> - }
The comment being deleted is what points out the assertion that already
exists.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 09/13] qapi: Assert non-input visitors see only valid narrow integers
2020-04-23 16:00 ` [PATCH 09/13] qapi: Assert non-input visitors see only valid narrow integers Markus Armbruster
@ 2020-04-23 17:57 ` Eric Blake
0 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2020-04-23 17:57 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: mdroth
On 4/23/20 11:00 AM, Markus Armbruster wrote:
> 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>
> ---
> qapi/qapi-visit-core.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 10/13] qapi: Clean up visitor's recovery from input with invalid type
2020-04-23 16:00 ` [PATCH 10/13] qapi: Clean up visitor's recovery from input with invalid type Markus Armbruster
@ 2020-04-23 18:06 ` Eric Blake
2020-04-23 18:18 ` Eric Blake
0 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2020-04-23 18:06 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: mdroth
On 4/23/20 11:00 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.
> This is a roundabout way to g_free() the alternate.
>
> Simplify: replace the qapi_free_FOO() by g_free().
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> scripts/qapi/visit.py | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Required looking at what gets generated into qapi_free_FOO() as well as
when visit_start_alternate() can fail, but makes sense.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 11/13] qapi: Assert non-input visitors see only valid alternate tags
2020-04-23 16:00 ` [PATCH 11/13] qapi: Assert non-input visitors see only valid alternate tags Markus Armbruster
@ 2020-04-23 18:09 ` Eric Blake
0 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2020-04-23 18:09 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: mdroth
On 4/23/20 11:00 AM, Markus Armbruster wrote:
> 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>
> ---
> scripts/qapi/visit.py | 1 +
> 1 file changed, 1 insertion(+)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 10/13] qapi: Clean up visitor's recovery from input with invalid type
2020-04-23 18:06 ` Eric Blake
@ 2020-04-23 18:18 ` Eric Blake
2020-04-24 7:44 ` Markus Armbruster
0 siblings, 1 reply; 30+ messages in thread
From: Eric Blake @ 2020-04-23 18:18 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: mdroth
On 4/23/20 1:06 PM, Eric Blake wrote:
> On 4/23/20 11:00 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.
>> This is a roundabout way to g_free() the alternate.
>>
>> Simplify: replace the qapi_free_FOO() by g_free().
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> scripts/qapi/visit.py | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>
> Required looking at what gets generated into qapi_free_FOO() as well as
> when visit_start_alternate() can fail, but makes sense.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
Actually, I'm having second thoughts. As an example, look at the generated:
> void visit_type_BlockDirtyBitmapMergeSource(Visitor *v, const char *name, BlockDirtyBitmapMergeSource **obj, Error **errp)
> {
> Error *err = NULL;
>
> visit_start_alternate(v, name, (GenericAlternate **)obj, sizeof(**obj),
> &err);
> if (err) {
> goto out;
> }
> if (!*obj) {
> goto out_obj;
[1]
> }
> switch ((*obj)->type) {
> case QTYPE_QSTRING:
> visit_type_str(v, name, &(*obj)->u.local, &err);
[2]
> break;
> case QTYPE_QDICT:
> visit_start_struct(v, name, NULL, 0, &err);
> if (err) {
> break;
[3]
> }
> visit_type_BlockDirtyBitmap_members(v, &(*obj)->u.external, &err);
> if (!err) {
> visit_check_struct(v, &err);
[4]
> }
> visit_end_struct(v, NULL);
> break;
> case QTYPE_NONE:
> abort();
> default:
> error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> "BlockDirtyBitmapMergeSource");
[5]
> }
> out_obj:
> visit_end_alternate(v, (void **)obj);
> if (err && visit_is_input(v)) {
> qapi_free_BlockDirtyBitmapMergeSource(*obj);
If we got here, we must have failed at any of the points mentioned above.
If [1], visit_start_alternate() failed, but *obj is NULL and both
qapi_free_FOO(NULL) and g_free(NULL) are safe.
If [2], visit_type_str() failed, so *obj is allocated but the embedded
string (here, u.local) was left NULL. qapi_free_FOO() then does nothing
further than g_free(obj).
If [3], visit_start_struct() failed, the embedded dict (here,
u.external) was left NULL. qapi_free_FOO() then does nothing further
than g_free(obj).
If [5], we have the wrong ->type. As pointed out by this commit,
qapi_free_FOO() does nothing further than g_free(obj).
But what happens in [4]? Here, the embedded dict was allocated, but we
then failed while parsing its members. That leaves us in a
partially-allocated state, and g_free(NULL) does NOT recursively visit
that partial allocation. I think this patch is prone to a memory leak
unless you _also_ patch things to free any dict branch on failure
(perhaps during the QTYPE_QDICT case label, rather than here at the end).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 12/13] qapi: Only input visitors can actually fail
2020-04-23 16:00 ` [PATCH 12/13] qapi: Only input visitors can actually fail Markus Armbruster
@ 2020-04-23 18:31 ` Eric Blake
0 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2020-04-23 18:31 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: mdroth
On 4/23/20 11:00 AM, Markus Armbruster wrote:
> 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>
> ---
I guess the only other failures an output visitor might encounter are
out-of-memory, or invalid contents (such as a NaN in a 'number', or
invalid encoding in a string...), and aborting on those errors at the
point they happen rather than trying to return errp makes sense.
It makes for a nice conceptual result: input parsing fails on unexpected
input, but output visitors should never be used on invalid data.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 13/13] qom: Simplify object_property_get_enum()
2020-04-23 16:00 ` [PATCH 13/13] qom: Simplify object_property_get_enum() Markus Armbruster
@ 2020-04-23 18:40 ` Eric Blake
0 siblings, 0 replies; 30+ messages in thread
From: Eric Blake @ 2020-04-23 18:40 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel; +Cc: mdroth
On 4/23/20 11:00 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> qom/object.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 08/13] qapi: Assert output visitors see only valid enum values
2020-04-23 17:56 ` Eric Blake
@ 2020-04-24 7:31 ` Markus Armbruster
0 siblings, 0 replies; 30+ messages in thread
From: Markus Armbruster @ 2020-04-24 7:31 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, mdroth
Eric Blake <eblake@redhat.com> writes:
> On 4/23/20 11:00 AM, Markus Armbruster wrote:
>> 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>
>> ---
>> 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(-)
>
> Nice cleanup.
>
> The commit message implies adding an assertion; but in reality, the
> change is deleting dead code (because we already have the assertion in
> place). Maybe it's worth updating the subject?
I tried to say it in the body: "qapi_enum_lookup()'s assertion still
guards." I could replace that by "This unmasks qapi_enum_lookup()'s
assertion." Okay? Better ideas?
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>> - /*
>> - * 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;
>> - }
>
> The comment being deleted is what points out the assertion that
> already exists.
Thanks!
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 10/13] qapi: Clean up visitor's recovery from input with invalid type
2020-04-23 18:18 ` Eric Blake
@ 2020-04-24 7:44 ` Markus Armbruster
0 siblings, 0 replies; 30+ messages in thread
From: Markus Armbruster @ 2020-04-24 7:44 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, mdroth
Eric Blake <eblake@redhat.com> writes:
> On 4/23/20 1:06 PM, Eric Blake wrote:
>> On 4/23/20 11:00 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.
>>> This is a roundabout way to g_free() the alternate.
>>>
>>> Simplify: replace the qapi_free_FOO() by g_free().
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>> scripts/qapi/visit.py | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>
>> Required looking at what gets generated into qapi_free_FOO() as well
>> as when visit_start_alternate() can fail, but makes sense.
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> Actually, I'm having second thoughts. As an example, look at the generated:
>
>> void visit_type_BlockDirtyBitmapMergeSource(Visitor *v, const char *name, BlockDirtyBitmapMergeSource **obj, Error **errp)
>> {
>> Error *err = NULL;
>>
>> visit_start_alternate(v, name, (GenericAlternate **)obj, sizeof(**obj),
>> &err);
>> if (err) {
>> goto out;
>> }
>> if (!*obj) {
>> goto out_obj;
> [1]
>> }
>> switch ((*obj)->type) {
>> case QTYPE_QSTRING:
>> visit_type_str(v, name, &(*obj)->u.local, &err);
> [2]
>> break;
>> case QTYPE_QDICT:
>> visit_start_struct(v, name, NULL, 0, &err);
>> if (err) {
>> break;
> [3]
>> }
>> visit_type_BlockDirtyBitmap_members(v, &(*obj)->u.external, &err);
>> if (!err) {
>> visit_check_struct(v, &err);
> [4]
>> }
>> visit_end_struct(v, NULL);
>> break;
>> case QTYPE_NONE:
>> abort();
>> default:
>> error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>> "BlockDirtyBitmapMergeSource");
> [5]
>> }
>> out_obj:
>> visit_end_alternate(v, (void **)obj);
>> if (err && visit_is_input(v)) {
>> qapi_free_BlockDirtyBitmapMergeSource(*obj);
>
> If we got here, we must have failed at any of the points mentioned above.
>
> If [1], visit_start_alternate() failed, but *obj is NULL and both
> qapi_free_FOO(NULL) and g_free(NULL) are safe.
>
> If [2], visit_type_str() failed, so *obj is allocated but the embedded
> string (here, u.local) was left NULL. qapi_free_FOO() then does
> nothing further than g_free(obj).
>
> If [3], visit_start_struct() failed, the embedded dict (here,
> u.external) was left NULL. qapi_free_FOO() then does nothing further
> than g_free(obj).
>
> If [5], we have the wrong ->type. As pointed out by this commit,
> qapi_free_FOO() does nothing further than g_free(obj).
>
> But what happens in [4]? Here, the embedded dict was allocated, but
> we then failed while parsing its members. That leaves us in a
> partially-allocated state, and g_free(NULL) does NOT recursively visit
> that partial allocation. I think this patch is prone to a memory leak
> unless you _also_ patch things to free any dict branch on failure
> (perhaps during the QTYPE_QDICT case label, rather than here at the
> end).
You're right.
Let's change cleanup only for the default case, like this:
default:
error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"BlockDirtyBitmapMergeSource");
+ g_free(*obj);
+ *obj = NULL;
}
out_obj:
visit_end_alternate(v, (void **)obj);
if (err && visit_is_input(v)) {
qapi_free_BlockDirtyBitmapMergeSource(*obj);
*obj = NULL;
}
out:
error_propagate(errp, err);
}
Thanks!
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2020-04-24 7:45 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-23 16:00 [PATCH 00/13] qapi: Spring cleaning Markus Armbruster
2020-04-23 16:00 ` [PATCH 01/13] qapi: Belatedly update visitor.h's big comment for QAPI modules Markus Armbruster
2020-04-23 17:33 ` Eric Blake
2020-04-23 16:00 ` [PATCH 02/13] qapi: Fix the virtual walk example in visitor.h's big comment Markus Armbruster
2020-04-23 17:33 ` Eric Blake
2020-04-23 16:00 ` [PATCH 03/13] qapi: Fix typo in visit_start_list()'s contract Markus Armbruster
2020-04-23 17:34 ` Eric Blake
2020-04-23 16:00 ` [PATCH 04/13] qapi: Document @errp usage more thoroughly in visitor.h Markus Armbruster
2020-04-23 17:35 ` Eric Blake
2020-04-23 16:00 ` [PATCH 05/13] qapi: Polish prose " Markus Armbruster
2020-04-23 17:51 ` Eric Blake
2020-04-23 16:00 ` [PATCH 06/13] qapi: Assert incomplete object occurs only in dealloc visitor Markus Armbruster
2020-04-23 17:52 ` Eric Blake
2020-04-23 16:00 ` [PATCH 07/13] qapi: Fix Visitor contract for start_alternate() Markus Armbruster
2020-04-23 17:53 ` Eric Blake
2020-04-23 16:00 ` [PATCH 08/13] qapi: Assert output visitors see only valid enum values Markus Armbruster
2020-04-23 17:56 ` Eric Blake
2020-04-24 7:31 ` Markus Armbruster
2020-04-23 16:00 ` [PATCH 09/13] qapi: Assert non-input visitors see only valid narrow integers Markus Armbruster
2020-04-23 17:57 ` Eric Blake
2020-04-23 16:00 ` [PATCH 10/13] qapi: Clean up visitor's recovery from input with invalid type Markus Armbruster
2020-04-23 18:06 ` Eric Blake
2020-04-23 18:18 ` Eric Blake
2020-04-24 7:44 ` Markus Armbruster
2020-04-23 16:00 ` [PATCH 11/13] qapi: Assert non-input visitors see only valid alternate tags Markus Armbruster
2020-04-23 18:09 ` Eric Blake
2020-04-23 16:00 ` [PATCH 12/13] qapi: Only input visitors can actually fail Markus Armbruster
2020-04-23 18:31 ` Eric Blake
2020-04-23 16:00 ` [PATCH 13/13] qom: Simplify object_property_get_enum() Markus Armbruster
2020-04-23 18:40 ` Eric Blake
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.