[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 34/35] qapi: Change visit_type_FOO() to no lo
|
From: |
Marc-André Lureau |
|
Subject: |
Re: [Qemu-devel] [PATCH v8 34/35] qapi: Change visit_type_FOO() to no longer return partial objects |
|
Date: |
Tue, 5 Jan 2016 18:22:35 +0100 |
Hi
On Mon, Dec 21, 2015 at 6:08 PM, Eric Blake <address@hidden> wrote:
> 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 pointer-based visit_type_FOO() functions always
> leave a safe value in *obj during an input visitor (either the new
> object on success, or NULL if an error is encountered).
>
> Since input visitors have blind assignment semantics, we have to
> track the result of whether an assignment is made all the way down
> to each visitor callback implementation, to avoid making decisions
> based on potentially uninitialized storage.
>
> Note that we still leave *obj unchanged after a scalar-based
> visit_type_FOO(); I did not feel like auditing all uses of
> visit_type_Enum() to see if the callers would tolerate a specific
> sentinel value (not to mention having to decide whether it would
> be better to use 0 or ENUM__MAX as that sentinel).
>
> Signed-off-by: Eric Blake <address@hidden>
>
nice cleanup, few issues:
> ---
> v8: rebase to earlier changes
> v7: rebase to earlier changes, enhance commit message, also fix
> visit_type_str() and visit_type_any()
> v6: rebase on top of earlier doc and formatting improvements, mention
> that *obj can be uninitialized on entry to an input visitor, rework
> semantics to keep valgrind happy on uninitialized input, break some
> long lines
> ---
> include/qapi/visitor-impl.h | 6 ++---
> include/qapi/visitor.h | 53
> ++++++++++++++++++++++++++++--------------
> qapi/opts-visitor.c | 11 ++++++---
> qapi/qapi-dealloc-visitor.c | 9 ++++---
> qapi/qapi-visit-core.c | 39 ++++++++++++++++++++++++++-----
> qapi/qmp-input-visitor.c | 18 +++++++++-----
> qapi/qmp-output-visitor.c | 6 +++--
> qapi/string-input-visitor.c | 6 +++--
> qapi/string-output-visitor.c | 3 ++-
> scripts/qapi-visit.py | 40 +++++++++++++++++++++++--------
> tests/test-qmp-commands.c | 13 +++++------
> tests/test-qmp-input-strict.c | 19 +++++++--------
> tests/test-qmp-input-visitor.c | 10 ++------
> 13 files changed, 154 insertions(+), 79 deletions(-)
>
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index 94d65fa..c32e5f5 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -26,7 +26,7 @@ struct Visitor
> {
> /* Must be provided to visit structs (the string visitors do not
> * currently visit structs). */
> - void (*start_struct)(Visitor *v, const char *name, void **obj,
> + bool (*start_struct)(Visitor *v, const char *name, void **obj,
> size_t size, Error **errp);
> /* May be NULL; most useful for input visitors. */
> void (*check_struct)(Visitor *v, Error **errp);
> @@ -34,13 +34,13 @@ struct Visitor
> void (*end_struct)(Visitor *v);
>
> /* May be NULL; most useful for input visitors. */
> - void (*start_implicit_struct)(Visitor *v, void **obj, size_t size,
> + bool (*start_implicit_struct)(Visitor *v, void **obj, size_t size,
> Error **errp);
> /* May be NULL */
> void (*end_implicit_struct)(Visitor *v);
>
> /* Must be set */
> - void (*start_list)(Visitor *v, const char *name, GenericList **list,
> + bool (*start_list)(Visitor *v, const char *name, GenericList **list,
> Error **errp);
> /* Must be set */
> GenericList *(*next_list)(Visitor *v, GenericList *element, Error
> **errp);
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index 4638863..4eae633 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -31,6 +31,27 @@
> * visitor-impl.h.
> */
>
> +/* All qapi types have a corresponding function with a signature
> + * roughly compatible with this:
> + *
> + * void visit_type_FOO(Visitor *v, void *obj, const char *name, Error
> **errp);
> + *
> + * where address@hidden 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, enum, and list types are generated (see qapi-visit.h).
> + * Input visitors may receive an uninitialized address@hidden, and guarantee
> a
> + * safe value is assigned (a new object on success, or NULL on failure).
> + * Output visitors expect address@hidden to be properly initialized on entry.
> + *
> + * Additionally, all qapi structs have a generated function compatible
> + * with this:
> + *
> + * void qapi_free_FOO(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
> @@ -62,11 +83,12 @@ typedef struct GenericList
> * Set address@hidden on failure; for example, if the input stream does not
> * have a member @name or if the member is not an object.
> *
> - * FIXME: For input visitors, address@hidden can be assigned here even if
> later
> - * visits will fail; this can lead to memory leaks if clients aren't
> - * careful.
> + * Returns true if address@hidden was allocated; if that happens, and an
> error
> + * occurs any time before the matching visit_end_struct(), then the
> + * caller (usually a visit_type_FOO() function) knows to undo the
> + * allocation before returning control further.
> */
> -void visit_start_struct(Visitor *v, const char *name, void **obj,
> +bool visit_start_struct(Visitor *v, const char *name, void **obj,
> size_t size, Error **errp);
> /**
> * Prepare for completing a struct visit.
> @@ -85,19 +107,15 @@ void visit_end_struct(Visitor *v);
>
> /**
> * Prepare to visit an implicit struct.
> - * Similar to visit_start_struct(), except that an implicit struct
> - * represents a subset of keys that are present at the same nesting level
> - * of a common object but as a separate qapi C struct, rather than a new
> - * object at a deeper nesting level.
> + * Similar to visit_start_struct(), including return semantics, except
> + * that an implicit struct represents a subset of keys that are present
> + * at the same nesting level of a common object but as a separate qapi
> + * C struct, rather than a new object at a deeper nesting level.
> *
> * @obj must not be NULL, since this function is only called when
> * visiting with qapi structs.
> - *
> - * FIXME: For input visitors, address@hidden can be assigned here even if
> later
> - * visits will fail; this can lead to memory leaks if clients aren't
> - * careful.
> */
> -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 an implicit struct visit started earlier.
> @@ -126,11 +144,12 @@ void visit_end_implicit_struct(Visitor *v);
> * in this case, visit_next_list() will not be needed, but
> * visit_end_list() is still mandatory.
> *
> - * FIXME: For input visitors, address@hidden can be assigned here even if
> - * later visits will fail; this can lead to memory leaks if clients
> - * aren't careful.
> + * Returns true if address@hidden was allocated; if that happens, and an
> error
> + * occurs any time before the matching visit_end_list(), then the
> + * caller (usually a visit_type_FOO() function) knows to undo the
> + * allocation before returning control further.
> */
> -void visit_start_list(Visitor *v, const char *name, GenericList **list,
> +bool visit_start_list(Visitor *v, const char *name, GenericList **list,
> Error **errp);
> /**
> * Iterate over a GenericList during a list visit.
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index c5a7396..38d1e68 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -122,18 +122,20 @@ opts_visitor_insert(GHashTable *unprocessed_opts, const
> QemuOpt *opt)
> }
>
>
> -static void
> +static bool
> opts_start_struct(Visitor *v, const char *name, void **obj,
> size_t size, Error **errp)
> {
> OptsVisitor *ov = to_ov(v);
> const QemuOpt *opt;
> + bool result = false;
>
> if (obj) {
> *obj = g_malloc0(size > 0 ? size : 1);
> + result = true;
> }
> if (ov->depth++ > 0) {
> - return;
> + return result;
> }
>
> ov->unprocessed_opts = g_hash_table_new_full(&g_str_hash, &g_str_equal,
> @@ -152,6 +154,7 @@ opts_start_struct(Visitor *v, const char *name, void
> **obj,
> ov->fake_id_opt->str = g_strdup(ov->opts_root->id);
> opts_visitor_insert(ov->unprocessed_opts, ov->fake_id_opt);
> }
> + return result;
> }
>
>
> @@ -210,7 +213,7 @@ lookup_distinct(const OptsVisitor *ov, const char *name,
> Error **errp)
> }
>
>
> -static void
> +static bool
> opts_start_list(Visitor *v, const char *name, GenericList **list, Error
> **errp)
> {
> OptsVisitor *ov = to_ov(v);
> @@ -223,8 +226,10 @@ opts_start_list(Visitor *v, const char *name,
> GenericList **list, Error **errp)
> if (ov->repeated_opts) {
> ov->list_mode = LM_IN_PROGRESS;
> *list = g_new0(GenericList, 1);
> + return true;
> } else {
> *list = NULL;
> + return false;
> }
> }
>
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index 839049e..0990199 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -53,11 +53,12 @@ static void *qapi_dealloc_pop(QapiDeallocVisitor *qov)
> return value;
> }
>
> -static void qapi_dealloc_start_struct(Visitor *v, const char *name, void
> **obj,
> +static bool qapi_dealloc_start_struct(Visitor *v, const char *name, void
> **obj,
> size_t unused, Error **errp)
> {
> QapiDeallocVisitor *qov = to_qov(v);
> qapi_dealloc_push(qov, obj);
> + return false;
> }
>
> static void qapi_dealloc_end_struct(Visitor *v)
> @@ -69,13 +70,14 @@ static void qapi_dealloc_end_struct(Visitor *v)
> }
> }
>
> -static void qapi_dealloc_start_implicit_struct(Visitor *v,
> +static bool qapi_dealloc_start_implicit_struct(Visitor *v,
> void **obj,
> size_t size,
> Error **errp)
> {
> QapiDeallocVisitor *qov = to_qov(v);
> qapi_dealloc_push(qov, obj);
> + return false;
> }
>
> static void qapi_dealloc_end_implicit_struct(Visitor *v)
> @@ -87,11 +89,12 @@ static void qapi_dealloc_end_implicit_struct(Visitor *v)
> }
> }
>
> -static void qapi_dealloc_start_list(Visitor *v, const char *name,
> +static bool qapi_dealloc_start_list(Visitor *v, const char *name,
> GenericList **list, Error **errp)
> {
> QapiDeallocVisitor *qov = to_qov(v);
> qapi_dealloc_push(qov, NULL);
> + return false;
> }
>
> static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList *list,
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index f391a70..977df85 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -26,14 +26,20 @@ static bool visit_is_output(Visitor *v)
> return v->type_enum == output_type_enum;
> }
>
> -void visit_start_struct(Visitor *v, const char *name, void **obj,
> +bool visit_start_struct(Visitor *v, const char *name, void **obj,
> size_t size, Error **errp)
> {
> + bool result;
> +
> assert(obj ? size : !size);
> if (obj && visit_is_output(v)) {
> assert(*obj);
> }
> - v->start_struct(v, name, obj, size, errp);
> + result = v->start_struct(v, name, obj, size, errp);
> + if (result) {
> + assert(obj && *obj);
> + }
> + return result;
> }
>
> void visit_check_struct(Visitor *v, Error **errp)
> @@ -48,16 +54,22 @@ void visit_end_struct(Visitor *v)
> v->end_struct(v);
> }
>
> -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 result = false;
> +
> assert(obj && size);
> if (visit_is_output(v)) {
> assert(*obj);
> }
> if (v->start_implicit_struct) {
> - v->start_implicit_struct(v, obj, size, errp);
> + result = v->start_implicit_struct(v, obj, size, errp);
> }
> + if (result) {
> + assert(*obj);
> + }
> + return result;
> }
>
> void visit_end_implicit_struct(Visitor *v)
> @@ -67,10 +79,14 @@ void visit_end_implicit_struct(Visitor *v)
> }
> }
>
> -void visit_start_list(Visitor *v, const char *name, GenericList **list,
> +bool visit_start_list(Visitor *v, const char *name, GenericList **list,
> Error **errp)
> {
> - v->start_list(v, name, list, errp);
> + bool result = v->start_list(v, name, list, errp);
> + if (result) {
> + assert(list && *list);
> + }
> + return result;
> }
>
> GenericList *visit_next_list(Visitor *v, GenericList *list, Error **errp)
> @@ -229,6 +245,7 @@ void visit_type_bool(Visitor *v, const char *name, bool
> *obj, Error **errp)
>
> void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp)
> {
> + Error *err = NULL;
> assert(obj);
> /* TODO: Fix callers to not pass NULL when they mean "", so that we
> * can enable:
> @@ -237,6 +254,10 @@ void visit_type_str(Visitor *v, const char *name, char
> **obj, Error **errp)
> }
> */
> v->type_str(v, name, obj, errp);
> + if (!visit_is_output(v) && err) {
> + *obj = NULL;
> + }
This will always evelatute to false, you need to change the line above I suppose
> + error_propagate(errp, err);
> }
>
> void visit_type_number(Visitor *v, const char *name, double *obj,
> @@ -248,11 +269,17 @@ void visit_type_number(Visitor *v, const char *name,
> double *obj,
>
> void visit_type_any(Visitor *v, const char *name, QObject **obj, Error
> **errp)
> {
> + Error *err = NULL;
> +
> assert(obj);
> if (visit_is_output(v)) {
> assert(*obj);
> }
> v->type_any(v, name, obj, errp);
> + if (!visit_is_output(v) && err) {
> + *obj = NULL;
> + }
same here
> + error_propagate(errp, err);
> }
>
> void visit_type_null(Visitor *v, const char *name, Error **errp)
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 550ee93..2427a80 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -130,7 +130,7 @@ static void qmp_input_pop(Visitor *v)
> qiv->nb_stack--;
> }
>
> -static void qmp_input_start_struct(Visitor *v, const char *name, void **obj,
> +static bool qmp_input_start_struct(Visitor *v, const char *name, void **obj,
> size_t size, Error **errp)
> {
> QmpInputVisitor *qiv = to_qiv(v);
> @@ -140,30 +140,34 @@ static void qmp_input_start_struct(Visitor *v, const
> char *name, void **obj,
> if (!qobj || qobject_type(qobj) != QTYPE_QDICT) {
> error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> "QDict");
> - return;
> + return false;
> }
>
> qmp_input_push(qiv, qobj, NULL, &err);
> if (err) {
> error_propagate(errp, err);
> - return;
> + return false;
> }
>
> if (obj) {
> *obj = g_malloc0(size);
> + return true;
> }
> + return false;
> }
>
>
> -static void qmp_input_start_implicit_struct(Visitor *v, void **obj,
> +static bool qmp_input_start_implicit_struct(Visitor *v, void **obj,
> size_t size, Error **errp)
> {
> if (obj) {
> *obj = g_malloc0(size);
> + return true;
> }
> + return false;
> }
>
> -static void qmp_input_start_list(Visitor *v, const char *name,
> +static bool qmp_input_start_list(Visitor *v, const char *name,
> GenericList **list, Error **errp)
> {
> QmpInputVisitor *qiv = to_qiv(v);
> @@ -173,7 +177,7 @@ static void qmp_input_start_list(Visitor *v, const char
> *name,
> if (!qobj || qobject_type(qobj) != QTYPE_QLIST) {
> error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> "list");
> - return;
> + return false;
> }
>
> entry = qlist_first(qobject_to_qlist(qobj));
> @@ -181,10 +185,12 @@ static void qmp_input_start_list(Visitor *v, const char
> *name,
> if (list) {
> if (entry) {
> *list = g_new0(GenericList, 1);
> + return true;
> } else {
> *list = NULL;
> }
> }
> + return false;
> }
>
> static GenericList *qmp_input_next_list(Visitor *v, GenericList *list,
> diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
> index c9615fe..cd45ffb 100644
> --- a/qapi/qmp-output-visitor.c
> +++ b/qapi/qmp-output-visitor.c
> @@ -103,7 +103,7 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov,
> const char *name,
> }
> }
>
> -static void qmp_output_start_struct(Visitor *v, const char *name, void **obj,
> +static bool qmp_output_start_struct(Visitor *v, const char *name, void **obj,
> size_t unused, Error **errp)
> {
> QmpOutputVisitor *qov = to_qov(v);
> @@ -111,6 +111,7 @@ static void qmp_output_start_struct(Visitor *v, const
> char *name, void **obj,
>
> qmp_output_add(qov, name, dict);
> qmp_output_push(qov, dict);
> + return false;
> }
>
> static void qmp_output_end_struct(Visitor *v)
> @@ -119,7 +120,7 @@ static void qmp_output_end_struct(Visitor *v)
> qmp_output_pop(qov);
> }
>
> -static void qmp_output_start_list(Visitor *v, const char *name,
> +static bool qmp_output_start_list(Visitor *v, const char *name,
> GenericList **listp, Error **errp)
> {
> QmpOutputVisitor *qov = to_qov(v);
> @@ -127,6 +128,7 @@ static void qmp_output_start_list(Visitor *v, const char
> *name,
>
> qmp_output_add(qov, name, list);
> qmp_output_push(qov, list);
> + return false;
> }
>
> static GenericList *qmp_output_next_list(Visitor *v, GenericList *list,
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index 582a62a..0e4a07c 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -120,7 +120,7 @@ error:
> siv->ranges = NULL;
> }
>
> -static void
> +static bool
> start_list(Visitor *v, const char *name, GenericList **list, Error **errp)
> {
> StringInputVisitor *siv = to_siv(v);
> @@ -132,7 +132,7 @@ start_list(Visitor *v, const char *name, GenericList
> **list, Error **errp)
> parse_str(siv, &err);
> if (err) {
> error_propagate(errp, err);
> - return;
> + return false;
> }
>
> siv->cur_range = g_list_first(siv->ranges);
> @@ -142,8 +142,10 @@ start_list(Visitor *v, const char *name, GenericList
> **list, Error **errp)
> siv->cur = r->begin;
> }
> *list = g_new0(GenericList, 1);
> + return true;
> } else {
> *list = NULL;
> + return false;
> }
> }
>
> diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> index 7209d80..2666802 100644
> --- a/qapi/string-output-visitor.c
> +++ b/qapi/string-output-visitor.c
> @@ -263,7 +263,7 @@ static void print_type_number(Visitor *v, const char
> *name, double *obj,
> string_output_set(sov, g_strdup_printf("%f", *obj));
> }
>
> -static void
> +static bool
> start_list(Visitor *v, const char *name, GenericList **list, Error **errp)
> {
> StringOutputVisitor *sov = to_sov(v);
> @@ -276,6 +276,7 @@ start_list(Visitor *v, const char *name, GenericList
> **list, Error **errp)
> if (*list && (*list)->next) {
> sov->list_mode = LM_STARTED;
> }
> + return false;
> }
>
> static GenericList *
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 6016734..eebb5f7 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -58,8 +58,10 @@ def gen_visit_implicit_struct(typ):
> static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj,
> Error **errp)
> {
> Error *err = NULL;
> + bool allocated;
>
> - visit_start_implicit_struct(v, (void **)obj, sizeof(%(c_type)s), &err);
> + allocated = visit_start_implicit_struct(v, (void **)obj,
> + sizeof(%(c_type)s), &err);
> if (err) {
> goto out;
> }
> @@ -69,6 +71,10 @@ static void visit_type_implicit_%(c_type)s(Visitor *v,
> %(c_type)s **obj, Error *
> visit_type_%(c_type)s_fields(v, obj, &err);
> out_obj:
> visit_end_implicit_struct(v);
> + if (allocated && err) {
> + g_free(*obj);
> + *obj = NULL;
> + }
> out:
> error_propagate(errp, err);
> }
> @@ -116,18 +122,15 @@ out:
>
>
> def gen_visit_list(name, element_type):
> - # FIXME: if *obj is NULL on entry, and the first visit_next_list()
> - # assigns to *obj, while a later one fails, we should clean up *obj
> - # rather than leaving it non-NULL. As currently written, the caller must
> - # call qapi_free_FOOList() to avoid a memory leak of the partial FOOList.
> return mcgen('''
>
> void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj,
> Error **errp)
> {
> Error *err = NULL;
> %(c_name)s *elt;
> + bool allocated;
>
> - visit_start_list(v, name, (GenericList **)obj, &err);
> + allocated = visit_start_list(v, name, (GenericList **)obj, &err);
> if (err) {
> goto out;
> }
> @@ -144,6 +147,10 @@ void visit_type_%(c_name)s(Visitor *v, const char *name,
> %(c_name)s **obj, Error
> }
> visit_end_list(v);
> out:
> + if (allocated && err) {
> + qapi_free_%(c_name)s(*obj);
> + *obj = NULL;
> + }
> error_propagate(errp, err);
> }
> ''',
> @@ -174,8 +181,10 @@ def gen_visit_alternate(name, variants):
> void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj,
> Error **errp)
> {
> Error *err = NULL;
> + bool allocated;
>
> - visit_start_implicit_struct(v, (void**) obj, sizeof(%(c_name)s), &err);
> + allocated = visit_start_implicit_struct(v, (void **)obj,
> + sizeof(%(c_name)s), &err);
> if (err) {
> goto out;
> }
> @@ -204,11 +213,15 @@ void visit_type_%(c_name)s(Visitor *v, const char
> *name, %(c_name)s **obj, Error
> }
> out_obj:
> visit_end_implicit_struct(v);
> + 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
>
> @@ -236,8 +249,10 @@ def gen_visit_object(name, base, members, variants):
> void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj,
> Error **errp)
> {
> Error *err = NULL;
> + bool allocated;
>
> - visit_start_struct(v, name, (void **)obj, sizeof(%(c_name)s), &err);
> + allocated = visit_start_struct(v, name, (void **)obj, sizeof(%(c_name)s),
> + &err);
> if (err) {
> goto out;
> }
> @@ -301,10 +316,15 @@ void visit_type_%(c_name)s(Visitor *v, const char
> *name, %(c_name)s **obj, Error
> visit_check_struct(v, &err);
> out_obj:
> visit_end_struct(v);
> + if (allocated && err) {
> + qapi_free_%(c_name)s(*obj);
> + *obj = NULL;
> + }
> out:
> error_propagate(errp, err);
> }
> -''')
> +''',
> + c_name=c_name(name))
>
> return ret
>
> diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
> index bc8085d..d3466a4 100644
> --- a/tests/test-qmp-commands.c
> +++ b/tests/test-qmp-commands.c
> @@ -227,14 +227,13 @@ 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 */
> + /* verify that visit_type_XXX() cleans up properly on error */
> error_free_or_abort(&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);
> diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
> index 775ad39..4db35dd 100644
> --- a/tests/test-qmp-input-strict.c
> +++ b/tests/test-qmp-input-strict.c
> @@ -181,10 +181,7 @@ static void
> test_validate_fail_struct(TestInputVisitorData *data,
>
> visit_type_TestStruct(v, NULL, &p, &err);
> error_free_or_abort(&err);
> - if (p) {
> - g_free(p->string);
> - }
> - g_free(p);
> + g_assert(!p);
> }
>
> static void test_validate_fail_struct_nested(TestInputVisitorData *data,
> @@ -198,7 +195,7 @@ static void
> test_validate_fail_struct_nested(TestInputVisitorData *data,
>
> visit_type_UserDefTwo(v, NULL, &udp, &err);
> error_free_or_abort(&err);
> - qapi_free_UserDefTwo(udp);
> + g_assert(!udp);
> }
>
> static void test_validate_fail_list(TestInputVisitorData *data,
> @@ -212,7 +209,7 @@ static void test_validate_fail_list(TestInputVisitorData
> *data,
>
> visit_type_UserDefOneList(v, NULL, &head, &err);
> error_free_or_abort(&err);
> - qapi_free_UserDefOneList(head);
> + g_assert(!head);
> }
>
> static void test_validate_fail_union_native_list(TestInputVisitorData *data,
> @@ -227,7 +224,7 @@ static void
> test_validate_fail_union_native_list(TestInputVisitorData *data,
>
> visit_type_UserDefNativeListUnion(v, NULL, &tmp, &err);
> error_free_or_abort(&err);
> - qapi_free_UserDefNativeListUnion(tmp);
> + g_assert(!tmp);
> }
>
> static void test_validate_fail_union_flat(TestInputVisitorData *data,
> @@ -241,7 +238,7 @@ static void
> test_validate_fail_union_flat(TestInputVisitorData *data,
>
> visit_type_UserDefFlatUnion(v, NULL, &tmp, &err);
> error_free_or_abort(&err);
> - qapi_free_UserDefFlatUnion(tmp);
> + g_assert(!tmp);
> }
>
> static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData
> *data,
> @@ -256,13 +253,13 @@ static void
> test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data,
>
> visit_type_UserDefFlatUnion2(v, NULL, &tmp, &err);
> error_free_or_abort(&err);
> - qapi_free_UserDefFlatUnion2(tmp);
> + g_assert(!tmp);
> }
>
> static void test_validate_fail_alternate(TestInputVisitorData *data,
> const void *unused)
> {
> - UserDefAlternate *tmp = NULL;
> + UserDefAlternate *tmp;
> Visitor *v;
> Error *err = NULL;
>
> @@ -270,7 +267,7 @@ static void
> test_validate_fail_alternate(TestInputVisitorData *data,
>
> visit_type_UserDefAlternate(v, NULL, &tmp, &err);
> error_free_or_abort(&err);
> - qapi_free_UserDefAlternate(tmp);
> + g_assert(!tmp);
> }
>
> static void do_test_validate_qmp_introspect(TestInputVisitorData *data,
> diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
> index f6bd408..7f9191c 100644
> --- a/tests/test-qmp-input-visitor.c
> +++ b/tests/test-qmp-input-visitor.c
> @@ -708,18 +708,12 @@ static void test_visitor_in_errors(TestInputVisitorData
> *data,
>
> visit_type_TestStruct(v, NULL, &p, &err);
> error_free_or_abort(&err);
> - /* FIXME - a failed parse should not leave a partially-allocated p
> - * for us to clean up; this could cause callers to leak memory. */
> - g_assert(p->string == NULL);
> -
> - g_free(p->string);
> - g_free(p);
> + g_assert(!p);
>
> v = visitor_input_test_init(data, "[ '1', '2', false, '3' ]");
> visit_type_strList(v, NULL, &q, &err);
> error_free_or_abort(&err);
> - assert(q);
> - qapi_free_strList(q);
> + assert(!q);
> }
>
> static void test_visitor_in_wrong_type(TestInputVisitorData *data,
> --
> 2.4.3
>
>
--
Marc-André Lureau
- Re: [Qemu-devel] [PATCH v8 34/35] qapi: Change visit_type_FOO() to no longer return partial objects,
Marc-André Lureau <=