@@ -27,6 +27,25 @@
* scripts/qapi-visit.py. For the visitor callback contracts, see
* visitor-impl.h. */
+/* All qapi types have a corresponding function with a signature
+ * compatible with this:
+ *
+ * void visit_type_XXX(Visitor *v, void *obj, const char *name, Error **errp);
+ *
+ * where *@obj is itself a pointer or a scalar. (The visit functions for
+ * built-in types are declared here, while the functions for qapi-defined
+ * struct, union, and enum types are generated; see qapi-visit.h). Input
+ * visitors populate *@obj on success, and leave it unchanged on failure.
+ *
+ * Additionally, all qapi structs have a generated function compatible
+ * with this:
+ *
+ * void qapi_free_XXX(void *obj);
+ *
+ * which behaves like free(), even if @obj is NULL or was only partially
+ * allocated before encountering an error.
+ */
+
/* This struct is layout-compatible with all other *List structs
* created by the qapi generator. */
typedef struct GenericList
@@ -46,12 +65,12 @@ typedef struct GenericList
* input visitor, @obj can be NULL to validate that the visit will
* succeed; otherwise, *@obj is assigned with an allocation of @size
* bytes. For other visitors, *@obj is the object to visit. Set *@errp
- * on failure.
- *
- * FIXME: *@obj can be modified even on error; this can lead to
- * memory leaks if clients aren't careful.
+ * on failure. Returns true if *@obj was allocated; if that happens,
+ * and an error occurs any time before the matching visit_end_struct(),
+ * then the caller (usually a visit_type_XXX() function) knows to undo
+ * the allocation before returning control further.
*/
-void visit_start_struct(Visitor *v, void **obj, const char *kind,
+bool visit_start_struct(Visitor *v, void **obj, const char *kind,
const char *name, size_t size, Error **errp);
/**
* Complete the struct started by a previous visit_start_struct().
@@ -60,14 +79,11 @@ void visit_end_struct(Visitor *v, Error **errp);
/**
* Prepare to visit an implicit struct.
- * Similar to visit_start_struct(), except that this will visit a
- * C pointer pointing to @size bytes, and where the QDict fields are
- * part of the parent object.
- *
- * FIXME: *@obj can be modified even on error; this can lead to
- * memory leaks if clients aren't careful.
+ * Similar to visit_start_struct(), including return semantics, except
+ * that this will visit a C pointer pointing to @size bytes, and where
+ * the QDict fields are part of the parent object.
*/
-void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
+bool visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
Error **errp);
/**
* Complete the implicit struct started earlier.
@@ -17,10 +17,12 @@
#include "qapi/visitor.h"
#include "qapi/visitor-impl.h"
-void visit_start_struct(Visitor *v, void **obj, const char *kind,
+bool visit_start_struct(Visitor *v, void **obj, const char *kind,
const char *name, size_t size, Error **errp)
{
+ bool track_allocation = obj && !*obj;
v->start_struct(v, obj, kind, name, size, errp);
+ return track_allocation && *obj;
}
void visit_end_struct(Visitor *v, Error **errp)
@@ -28,12 +30,14 @@ void visit_end_struct(Visitor *v, Error **errp)
v->end_struct(v, errp);
}
-void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
+bool visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
Error **errp)
{
+ bool track_allocation = obj && !*obj;
if (v->start_implicit_struct) {
v->start_implicit_struct(v, obj, size, errp);
}
+ return track_allocation && *obj;
}
void visit_end_implicit_struct(Visitor *v, Error **errp)
@@ -46,14 +46,19 @@ static void visit_type_%(c_type)s_fields(Visitor *m, %(c_type)s **obj, Error **e
static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error **errp)
{
Error *err = NULL;
+ bool allocated;
- visit_start_implicit_struct(m, (void **)obj, sizeof(%(c_type)s), &err);
+ allocated = visit_start_implicit_struct(m, (void **)obj, sizeof(%(c_type)s), &err);
if (!err) {
if (!obj || *obj) {
visit_type_%(c_type)s_fields(m, obj, &err);
}
visit_end_implicit_struct(m, err ? NULL : &err);
}
+ if (allocated && err) {
+ g_free(*obj);
+ *obj = NULL;
+ }
error_propagate(errp, err);
}
''',
@@ -131,23 +136,24 @@ out:
def gen_visit_struct(name, base, members):
ret = gen_visit_struct_fields(name, base, members)
- # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
- # *obj, but then visit_type_FOO_fields() fails, we should clean up *obj
- # rather than leaving it non-NULL. As currently written, the caller must
- # call qapi_free_FOO() to avoid a memory leak of the partial FOO.
ret += mcgen('''
void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error **errp)
{
Error *err = NULL;
+ bool allocated;
- visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
+ allocated = visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
if (!err) {
if (*obj) {
visit_type_%(c_name)s_fields(m, obj, &err);
}
visit_end_struct(m, err ? NULL : &err);
}
+ if (allocated && err) {
+ qapi_free_%(c_name)s(*obj);
+ *obj = NULL;
+ }
error_propagate(errp, err);
}
''',
@@ -203,8 +209,9 @@ def gen_visit_alternate(name, variants):
void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error **errp)
{
Error *err = NULL;
+ bool allocated;
- visit_start_implicit_struct(m, (void**) obj, sizeof(%(c_name)s), &err);
+ allocated = visit_start_implicit_struct(m, (void**) obj, sizeof(%(c_name)s), &err);
if (err) {
goto out;
}
@@ -233,11 +240,15 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
}
out_obj:
visit_end_implicit_struct(m, err ? NULL : &err);
+ if (allocated && err) {
+ qapi_free_%(c_name)s(*obj);
+ *obj = NULL;
+ }
out:
error_propagate(errp, err);
}
''',
- name=name)
+ name=name, c_name=c_name(name))
return ret
@@ -258,8 +269,9 @@ def gen_visit_union(name, base, variants):
void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error **errp)
{
Error *err = NULL;
+ bool allocated;
- visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
+ allocated = visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
if (err) {
goto out;
}
@@ -321,10 +333,15 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
}
out_obj:
visit_end_struct(m, err ? NULL : &err);
+ if (allocated && err) {
+ qapi_free_%(c_name)s(*obj);
+ *obj = NULL;
+ }
out:
error_propagate(errp, err);
}
-''')
+''',
+ c_name=c_name(name))
return ret
@@ -218,15 +218,14 @@ static void test_dealloc_partial(void)
QDECREF(ud2_dict);
}
- /* verify partial success */
- assert(ud2 != NULL);
- assert(ud2->string0 != NULL);
- assert(strcmp(ud2->string0, text) == 0);
- assert(ud2->dict1 == NULL);
-
- /* confirm & release construction error */
- assert(err != NULL);
+ /* verify that visit_type_XXX() cleans up properly on error */
+ assert(err);
error_free(err);
+ assert(!ud2);
+
+ /* Manually create a partial object, leaving ud2->dict1 at NULL */
+ ud2 = g_new0(UserDefTwo, 1);
+ ud2->string0 = g_strdup(text);
/* tear down partial object */
qapi_free_UserDefTwo(ud2);
Returning a partial object on error is an invitation for a careless caller to leak memory. As no one outside the testsuite was actually relying on these semantics, it is cleaner to just document and guarantee that ALL visit_type_XXX() functions do not alter *obj when an error is encountered during an input visitor. Signed-off-by: Eric Blake <eblake@redhat.com> --- include/qapi/visitor.h | 40 ++++++++++++++++++++++++++++------------ qapi/qapi-visit-core.c | 8 ++++++-- scripts/qapi-visit.py | 37 +++++++++++++++++++++++++++---------- tests/test-qmp-commands.c | 15 +++++++-------- 4 files changed, 68 insertions(+), 32 deletions(-)