From: Auger Eric <eric.auger@redhat.com>
To: Greg Kurz <groug@kaod.org>
Cc: berrange@redhat.com, ehabkost@redhat.com, armbru@redhat.com,
qemu-devel@nongnu.org, pbonzini@redhat.com,
eric.auger.pro@gmail.com
Subject: Re: [PATCH v5 1/3] qom: Introduce object_property_try_add_child()
Date: Wed, 1 Jul 2020 15:23:19 +0200 [thread overview]
Message-ID: <883c3eee-b86e-3788-49e6-f515e82319cb@redhat.com> (raw)
In-Reply-To: <20200630154151.229e60eb@bahia.lan>
Hi Greg,
On 6/30/20 3:41 PM, Greg Kurz wrote:
> On Mon, 29 Jun 2020 21:34:22 +0200
> Eric Auger <eric.auger@redhat.com> wrote:
>
>> 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 achieved from the following call path:
>>
>> qmp_object_add -> user_creatable_add_dict -> user_creatable_add_type ->
>> object_property_add_child -> object_property_add
>>
>> For instance, from the qmp-shell, call twice:
>> object-add qom-type=memory-backend-ram id=mem1 props.size=1073741824
>> and QEMU aborts.
>>
>> 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')"}}
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Fixes: d2623129a7de ("qom: Drop parameter @errp of object_property_add() & friends")
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>
>
> FWIW
>
> Reviewed-by: Greg Kurz <groug@kaod.org>
>
> and
>
> Tested-by: Greg Kurz <groug@kaod.org>
Thanks!
Eric
>
>> ---
>>
>> v3 -> v4:
>> - Took into account Markus' style related comments
>>
>> v2 -> v3:
>> - don't take the object reference on failure in
>> object_property_try_add_child
>> ---
>> include/qom/object.h | 26 ++++++++++++++++++++++++--
>> qom/object.c | 21 ++++++++++++++++-----
>> qom/object_interfaces.c | 7 +++++--
>> 3 files changed, 45 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/qom/object.h b/include/qom/object.h
>> index 94a61ccc3f..1c5cdcd0e3 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,23 @@ 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: pointer to error object
>> *
>> * 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.
>> + */
>> ObjectProperty *object_property_add(Object *obj, const char *name,
>> const char *type,
>> ObjectPropertyAccessor *get,
>> @@ -1495,10 +1508,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: pointer to error object
>> *
>> * 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 +1526,14 @@ 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
>> + */
>> 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);
>
next prev parent reply other threads:[~2020-07-01 13:24 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-29 19:34 [PATCH v5 0/3] Avoid abort on QMP attempt to add an object with duplicate id Eric Auger
2020-06-29 19:34 ` [PATCH v5 1/3] qom: Introduce object_property_try_add_child() Eric Auger
2020-06-30 13:41 ` Greg Kurz
2020-07-01 13:23 ` Auger Eric [this message]
2020-06-29 19:34 ` [PATCH v5 2/3] tests/qmp-cmd-test: Add qmp/object-add-duplicate-id Eric Auger
2020-06-29 19:34 ` [PATCH v5 3/3] tests/qmp-cmd-test: Add qmp/object-add-failure-modes Eric Auger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=883c3eee-b86e-3788-49e6-f515e82319cb@redhat.com \
--to=eric.auger@redhat.com \
--cc=armbru@redhat.com \
--cc=berrange@redhat.com \
--cc=ehabkost@redhat.com \
--cc=eric.auger.pro@gmail.com \
--cc=groug@kaod.org \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).