Re: [PATCH v3 1/2] qom: Introduce object_property_try_add_child()

[Auger Eric <eric.auger@redhat.com>]

Hi Markus

On 6/25/20 11:24 AM, Markus Armbruster wrote:
> 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 ->
> ...
OK
> 
>> 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?
yes qmp-shell, I will add this info
> 
>>
>> 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
OK
>     * @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.
OK
> 
>>  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.
ok
> 
>>   *
>>   * 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.
ok
> 
>>
>>  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>
Thanks!

Eric
>