| Message ID | 20200624124301.7112-2-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Avoid abort on QMP attempt to add an object with duplicate id | expand |
On 24/06/20 14:43, Eric Auger wrote: > + op = object_property_try_add(obj, name, type, object_get_child_property, > + NULL, object_finalize_child_property, > + child, errp); > + if (!op) { > + goto out; > + } > op->resolve = object_resolve_child_property; > +out: > object_ref(child); > child->parent = obj; > return op; I think if there's an error you need to return NULL without ref-ing child, shouldn't you? You can then add another test that object_property_add_child succeeds after object_property_try_add_child fails. Paolo
Hi Paolo, On 6/24/20 4:12 PM, Paolo Bonzini wrote: > On 24/06/20 14:43, Eric Auger wrote: >> + op = object_property_try_add(obj, name, type, object_get_child_property, >> + NULL, object_finalize_child_property, >> + child, errp); >> + if (!op) { >> + goto out; >> + } >> op->resolve = object_resolve_child_property; >> +out: >> object_ref(child); >> child->parent = obj; >> return op; > > I think if there's an error you need to return NULL without ref-ing > child, shouldn't you? hum yes you're fully right, the out label is badly placed. > > You can then add another test that object_property_add_child succeeds > after object_property_try_add_child fails. OK Thanks Eric > > Paolo >
Hi Paolo, On 6/24/20 4:17 PM, Auger Eric wrote: > Hi Paolo, > > On 6/24/20 4:12 PM, Paolo Bonzini wrote: >> On 24/06/20 14:43, Eric Auger wrote: >>> + op = object_property_try_add(obj, name, type, object_get_child_property, >>> + NULL, object_finalize_child_property, >>> + child, errp); >>> + if (!op) { >>> + goto out; >>> + } >>> op->resolve = object_resolve_child_property; >>> +out: >>> object_ref(child); >>> child->parent = obj; >>> return op; >> >> I think if there's an error you need to return NULL without ref-ing >> child, shouldn't you? > hum yes you're fully right, the out label is badly placed. Looks the unref is done in user_creatable_add_type() in case of error. Isn't it the corresponding one? Anyway I think it is better to avoid getting the ref here as you suggest (and also free type) and don't unref in user_creatable_add_type. Thanks Eric >> >> You can then add another test that object_property_add_child succeeds >> after object_property_try_add_child fails. > OK > > Thanks > > Eric >> >> Paolo >>
On 24/06/20 18:02, Auger Eric wrote: > Hi Paolo, > > On 6/24/20 4:17 PM, Auger Eric wrote: >> Hi Paolo, >> >> On 6/24/20 4:12 PM, Paolo Bonzini wrote: >>> On 24/06/20 14:43, Eric Auger wrote: >>>> + op = object_property_try_add(obj, name, type, object_get_child_property, >>>> + NULL, object_finalize_child_property, >>>> + child, errp); >>>> + if (!op) { >>>> + goto out; >>>> + } >>>> op->resolve = object_resolve_child_property; >>>> +out: >>>> object_ref(child); >>>> child->parent = obj; >>>> return op; >>> >>> I think if there's an error you need to return NULL without ref-ing >>> child, shouldn't you? >> hum yes you're fully right, the out label is badly placed. > Looks the unref is done in user_creatable_add_type() in case of error. There are two references involved: - a reference returned from object_new. user_creatable_add_type() passes it back to the caller. The object_unref() you found is done before returning NULL, because in that case nothing is being passed to the caller - a reference stored in child->parent. In case of error that reference is dropped with object_property_del before returning NULL. object_property_try_add_child must not store anything in child->parent in case of error, and therefore it need not add that reference either. I hope this is clearer. Thanks, Paolo > Isn't it the corresponding one? Anyway I think it is better to avoid > getting the ref here as you suggest (and also free type) and don't unref > in user_creatable_add_type. > > Thanks > > Eric >>> >>> You can then add another test that object_property_add_child succeeds >>> after object_property_try_add_child fails. >> OK >> >> Thanks >> >> Eric >>> >>> Paolo >>> >
Hi Paolo, On 6/24/20 6:16 PM, Paolo Bonzini wrote: > On 24/06/20 18:02, Auger Eric wrote: >> Hi Paolo, >> >> On 6/24/20 4:17 PM, Auger Eric wrote: >>> Hi Paolo, >>> >>> On 6/24/20 4:12 PM, Paolo Bonzini wrote: >>>> On 24/06/20 14:43, Eric Auger wrote: >>>>> + op = object_property_try_add(obj, name, type, object_get_child_property, >>>>> + NULL, object_finalize_child_property, >>>>> + child, errp); >>>>> + if (!op) { >>>>> + goto out; >>>>> + } >>>>> op->resolve = object_resolve_child_property; >>>>> +out: >>>>> object_ref(child); >>>>> child->parent = obj; >>>>> return op; >>>> >>>> I think if there's an error you need to return NULL without ref-ing >>>> child, shouldn't you? >>> hum yes you're fully right, the out label is badly placed. >> Looks the unref is done in user_creatable_add_type() in case of error. > > There are two references involved: > > - a reference returned from object_new. user_creatable_add_type() > passes it back to the caller. The object_unref() you found is done > before returning NULL, because in that case nothing is being passed to > the caller > > - a reference stored in child->parent. In case of error that reference > is dropped with object_property_del before returning NULL. > object_property_try_add_child must not store anything in child->parent > in case of error, and therefore it need not add that reference either. > > I hope this is clearer. Yes it helps. Thank you for the clarifications. Eric > > Thanks, > > Paolo > >> Isn't it the corresponding one? Anyway I think it is better to avoid >> getting the ref here as you suggest (and also free type) and don't unref >> in user_creatable_add_type. >> >> Thanks >> >> Eric >>>> >>>> You can then add another test that object_property_add_child succeeds >>>> after object_property_try_add_child fails. >>> OK >>> >>> Thanks >>> >>> Eric >>>> >>>> Paolo >>>> >> > >
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 * * 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 +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 * * 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 + */ ObjectProperty *object_property_add_child(Object *obj, const char *name, Object *child); diff --git a/qom/object.c b/qom/object.c index 6ece96bc2b..96e85cdaf7 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,26 @@ 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) { + goto out; + } op->resolve = object_resolve_child_property; +out: 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);
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. 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") --- include/qom/object.h | 24 ++++++++++++++++++++++-- qom/object.c | 22 +++++++++++++++++----- qom/object_interfaces.c | 7 +++++-- 3 files changed, 44 insertions(+), 9 deletions(-)