[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 1/2] qom: Introduce object_property_try_add_child()
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v3 1/2] qom: Introduce object_property_try_add_child() |
Date: |
Thu, 25 Jun 2020 11:24:17 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
Eric Auger <eric.auger@redhat.com> writes:
> object_property_add() does not allow object_property_try_add()
> to gracefully fail as &error_abort is passed as an error handle.
>
> However such failure can easily be triggered from the QMP shell when,
> for instance, one attempts to create an object with an id that already
> exists. This is achived from the following call path:
>
> user_creatable_add_type -> object_property_add_child ->
> object_property_add
>
> For instance, call twice:
> object-add qom-type=memory-backend-ram id=mem1 props.size=1073741824
> and QEMU aborts.
qmp_object_add -> user_creatable_add_dict -> user_creatable_add_type ->
...
> This behavior is undesired as a user/management application mistake
> in reusing a property ID shouldn't result in loss of the VM and live
> data within.
>
> This patch introduces a new function, object_property_try_add_child()
> which takes an error handle and turn object_property_try_add() into
> a non-static one.
>
> Now the call path becomes:
>
> user_creatable_add_type -> object_property_try_add_child ->
> object_property_try_add
>
> and the error is returned gracefully to the QMP client.
>
> (QEMU) object-add qom-type=memory-backend-ram id=mem2 props.size=4294967296
> {"return": {}}
> (QEMU) object-add qom-type=memory-backend-ram id=mem2 props.size=4294967296
> {"error": {"class": "GenericError", "desc": "attempt to add duplicate property
> 'mem2' to object (type 'container')"}}
What's this? qmp-shell?
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Fixes: d2623129a7de ("qom: Drop parameter @errp of object_property_add() &
> friends")
>
> ---
>
> v2 -> v3:
> - don't take the object reference on failure in
> object_property_try_add_child
> ---
> include/qom/object.h | 24 ++++++++++++++++++++++--
> qom/object.c | 21 ++++++++++++++++-----
> qom/object_interfaces.c | 7 +++++--
> 3 files changed, 43 insertions(+), 9 deletions(-)
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 94a61ccc3f..91cf058d86 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1039,7 +1039,7 @@ Object *object_ref(Object *obj);
> void object_unref(Object *obj);
>
> /**
> - * object_property_add:
> + * object_property_try_add:
> * @obj: the object to add a property to
> * @name: the name of the property. This can contain any character except
> for
> * a forward slash. In general, you should use hyphens '-' instead of
> @@ -1056,10 +1056,22 @@ void object_unref(Object *obj);
> * meant to allow a property to free its opaque upon object
> * destruction. This may be NULL.
> * @opaque: an opaque pointer to pass to the callbacks for the property
> + * @errp: error handle
We have several descriptions of @errp parameters in this file already,
and you're inventing a new one :)
Suggest to pick one of the existing ones instead:
* @errp: a pointer to an Error that is filled if getting/setting fails.
* @errp: If an error occurs, a pointer to an area to store the error
* @errp: pointer to error object
* @errp: returns an error if this function fails
> *
> * Returns: The #ObjectProperty; this can be used to set the @resolve
> * callback for child and link properties.
> */
> +ObjectProperty *object_property_try_add(Object *obj, const char *name,
> + const char *type,
> + ObjectPropertyAccessor *get,
> + ObjectPropertyAccessor *set,
> + ObjectPropertyRelease *release,
> + void *opaque, Error **errp);
> +
> +/**
> + * object_property_add: same as object_property_try_add with
> + * errp hardcoded to &error_abort
> + */
Style:
/**
* object_property_add:
* Same as object_property_try_add() with @errp hardcoded to
* &error_abort.
*/
The line break after ':' matches the rest of the file (personally, I
think the whole line is a complete waste then, but let's go with the
flow). The @ in @errp and the () in object_property_try_add() help
tools with highlighting and linking. Sentences start with a capital
letter, and end with punctuation.
> ObjectProperty *object_property_add(Object *obj, const char *name,
> const char *type,
> ObjectPropertyAccessor *get,
> @@ -1495,10 +1507,11 @@ Object *object_resolve_path_type(const char *path,
> const char *typename,
> Object *object_resolve_path_component(Object *parent, const char *part);
>
> /**
> - * object_property_add_child:
> + * object_property_try_add_child:
> * @obj: the object to add a property to
> * @name: the name of the property
> * @child: the child object
> + * @errp: error handle
Likewise.
> *
> * Child properties form the composition tree. All objects need to be a
> child
> * of another object. Objects can only be a child of one object.
> @@ -1512,6 +1525,13 @@ Object *object_resolve_path_component(Object *parent,
> const char *part);
> *
> * Returns: The newly added property on success, or %NULL on failure.
> */
> +ObjectProperty *object_property_try_add_child(Object *obj, const char *name,
> + Object *child, Error **errp);
> +
> +/**
> + * object_property_add_child: same as object_property_try_add_child with
> + * errp hardcoded to &error_abort
> + */
Likewise.
>
> ObjectProperty *object_property_add_child(Object *obj, const char *name,
> Object *child);
>
> diff --git a/qom/object.c b/qom/object.c
> index 6ece96bc2b..dc10bb1889 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1132,7 +1132,7 @@ void object_unref(Object *obj)
> }
> }
>
> -static ObjectProperty *
> +ObjectProperty *
> object_property_try_add(Object *obj, const char *name, const char *type,
> ObjectPropertyAccessor *get,
> ObjectPropertyAccessor *set,
> @@ -1651,8 +1651,8 @@ static void object_finalize_child_property(Object *obj,
> const char *name,
> }
>
> ObjectProperty *
> -object_property_add_child(Object *obj, const char *name,
> - Object *child)
> +object_property_try_add_child(Object *obj, const char *name,
> + Object *child, Error **errp)
> {
> g_autofree char *type = NULL;
> ObjectProperty *op;
> @@ -1661,14 +1661,25 @@ object_property_add_child(Object *obj, const char
> *name,
>
> type = g_strdup_printf("child<%s>", object_get_typename(child));
>
> - op = object_property_add(obj, name, type, object_get_child_property,
> NULL,
> - object_finalize_child_property, child);
> + op = object_property_try_add(obj, name, type, object_get_child_property,
> + NULL, object_finalize_child_property,
> + child, errp);
> + if (!op) {
> + return NULL;
> + }
> op->resolve = object_resolve_child_property;
> object_ref(child);
> child->parent = obj;
> return op;
> }
>
> +ObjectProperty *
> +object_property_add_child(Object *obj, const char *name,
> + Object *child)
> +{
> + return object_property_try_add_child(obj, name, child, &error_abort);
> +}
> +
> void object_property_allow_set_link(const Object *obj, const char *name,
> Object *val, Error **errp)
> {
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index 7e26f86fa6..1e05e41d2f 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -82,8 +82,11 @@ Object *user_creatable_add_type(const char *type, const
> char *id,
> }
>
> if (id != NULL) {
> - object_property_add_child(object_get_objects_root(),
> - id, obj);
> + object_property_try_add_child(object_get_objects_root(),
> + id, obj, &local_err);
> + if (local_err) {
> + goto out;
> + }
> }
>
> user_creatable_complete(USER_CREATABLE(obj), &local_err);
Preferably with the comments touched up:
Reviewed-by: Markus Armbruster <armbru@redhat.com>