QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Greg Kurz <groug@kaod.org>
To: Eric Auger <eric.auger@redhat.com>
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: Tue, 30 Jun 2020 15:41:51 +0200
Message-ID: <20200630154151.229e60eb@bahia.lan> (raw)
In-Reply-To: <20200629193424.30280-2-eric.auger@redhat.com>

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>

> ---
> 
> 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);



  reply index

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 [this message]
2020-07-01 13:23     ` Auger Eric
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=20200630154151.229e60eb@bahia.lan \
    --to=groug@kaod.org \
    --cc=armbru@redhat.com \
    --cc=berrange@redhat.com \
    --cc=ehabkost@redhat.com \
    --cc=eric.auger.pro@gmail.com \
    --cc=eric.auger@redhat.com \
    --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

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git