[PATCH v3 0/2] Avoid abort on QMP attempt to add an object with duplicate id

[PATCH v3 0/2] Avoid abort on QMP attempt to add an object with duplicate id

[Eric Auger]

Attempting to add an object through QMP with an id that is
already used leads to a qemu abort. This is a regression since
d2623129a7de ("qom: Drop parameter @errp of object_property_add()
& friends").

The first patch fixes the issue and the second patch adds a test
to check the error is gracefully returned to the QMP client.

Best Regards

Eric

This series can be found at:
https://github.com/eauger/qemu/tree/qom-graceful-v3

History:
- v2 -> v3:
  - don't take the object reference on failure in
    object_property_try_add_child
  - add g_assert_nonnull(resp) in 2/2 while keeping
    Thomas A-b

- v1 -> v2:
  - use the try terminology.
  - turn object_property_try_add() into a non-static function
  - add the test


Eric Auger (2):
  qom: Introduce object_property_try_add_child()
  tests/qmp-cmd-test: Add qmp/object-add-duplicate-id

 include/qom/object.h       | 24 ++++++++++++++++++++++--
 qom/object.c               | 21 ++++++++++++++++-----
 qom/object_interfaces.c    |  7 +++++--
 tests/qtest/qmp-cmd-test.c | 19 +++++++++++++++++++
 4 files changed, 62 insertions(+), 9 deletions(-)

-- 
2.20.1

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

[Eric Auger]

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

---

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
  *
  * 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..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);
-- 
2.20.1

[PATCH v3 2/2] tests/qmp-cmd-test: Add qmp/object-add-duplicate-id

[Eric Auger]

This new test checks that attempting to create an object
with an existing ID gracefully fails.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Acked-by: Thomas Huth <thuth@redhat.com>
---
 tests/qtest/qmp-cmd-test.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
index 9f5228cd99..ceb3296691 100644
--- a/tests/qtest/qmp-cmd-test.c
+++ b/tests/qtest/qmp-cmd-test.c
@@ -213,6 +213,23 @@ static void test_object_add_without_props(void)
     qtest_quit(qts);
 }
 
+static void test_object_add_with_duplicate_id(void)
+{
+    QTestState *qts;
+    QDict *resp;
+
+    qts = qtest_init(common_args);
+    resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
+                    " {'qom-type': 'memory-backend-ram', 'id': 'ram1', 'props': {'size': 4294967296 } } }");
+    g_assert_nonnull(resp);
+    g_assert(qdict_haskey(resp, "return"));
+    resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
+                    " {'qom-type': 'memory-backend-ram', 'id': 'ram1', 'props': {'size': 4294967296 } } }");
+    g_assert_nonnull(resp);
+    qmp_assert_error_class(resp, "GenericError");
+    qtest_quit(qts);
+}
+
 int main(int argc, char *argv[])
 {
     QmpSchema schema;
@@ -225,6 +242,8 @@ int main(int argc, char *argv[])
 
     qtest_add_func("qmp/object-add-without-props",
                    test_object_add_without_props);
+    qtest_add_func("qmp/object-add-duplicate-id",
+                   test_object_add_with_duplicate_id);
     /* TODO: add coverage of generic object-add failure modes */
 
     ret = g_test_run();
-- 
2.20.1

Re: [PATCH v3 0/2] Avoid abort on QMP attempt to add an object with duplicate id

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200624194809.26600-1-eric.auger@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  TEST    check-qtest-aarch64: tests/qtest/qmp-cmd-test
  TEST    iotest-qcow2: 176
**
ERROR:/tmp/qemu-test/src/tests/qtest/qmp-cmd-test.c:225:test_object_add_with_duplicate_id: assertion failed: (qdict_haskey(resp, "return"))
ERROR - Bail out! ERROR:/tmp/qemu-test/src/tests/qtest/qmp-cmd-test.c:225:test_object_add_with_duplicate_id: assertion failed: (qdict_haskey(resp, "return"))
make: *** [check-qtest-aarch64] Error 1
make: *** Waiting for unfinished jobs....
Could not access KVM kernel module: No such file or directory
qemu-system-x86_64: -accel kvm: failed to initialize kvm: No such file or directory
---
  TEST    check-qtest-x86_64: tests/qtest/qmp-test
  TEST    check-qtest-x86_64: tests/qtest/qmp-cmd-test
**
ERROR:/tmp/qemu-test/src/tests/qtest/qmp-cmd-test.c:225:test_object_add_with_duplicate_id: assertion failed: (qdict_haskey(resp, "return"))
ERROR - Bail out! ERROR:/tmp/qemu-test/src/tests/qtest/qmp-cmd-test.c:225:test_object_add_with_duplicate_id: assertion failed: (qdict_haskey(resp, "return"))
make: *** [check-qtest-x86_64] Error 1
  TEST    iotest-qcow2: 220
  TEST    iotest-qcow2: 226
  TEST    iotest-qcow2: 229
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=60ce26ab60e3443d984623b7775c3cae', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-6xdet1tc/src/docker-src.2020-06-24-16.11.32.12570:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=60ce26ab60e3443d984623b7775c3cae
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-6xdet1tc/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    12m28.815s
user    0m8.490s


The full log is available at
http://patchew.org/logs/20200624194809.26600-1-eric.auger@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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

[Markus Armbruster]

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>

Re: [PATCH v3 2/2] tests/qmp-cmd-test: Add qmp/object-add-duplicate-id

[Paolo Bonzini]

On 24/06/20 21:48, Eric Auger wrote:
> This new test checks that attempting to create an object
> with an existing ID gracefully fails.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Acked-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/qtest/qmp-cmd-test.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
> index 9f5228cd99..ceb3296691 100644
> --- a/tests/qtest/qmp-cmd-test.c
> +++ b/tests/qtest/qmp-cmd-test.c
> @@ -213,6 +213,23 @@ static void test_object_add_without_props(void)
>      qtest_quit(qts);
>  }
>  
> +static void test_object_add_with_duplicate_id(void)
> +{
> +    QTestState *qts;
> +    QDict *resp;
> +
> +    qts = qtest_init(common_args);
> +    resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
> +                    " {'qom-type': 'memory-backend-ram', 'id': 'ram1', 'props': {'size': 4294967296 } } }");
> +    g_assert_nonnull(resp);
> +    g_assert(qdict_haskey(resp, "return"));
> +    resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
> +                    " {'qom-type': 'memory-backend-ram', 'id': 'ram1', 'props': {'size': 4294967296 } } }");
> +    g_assert_nonnull(resp);
> +    qmp_assert_error_class(resp, "GenericError");
> +    qtest_quit(qts);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>      QmpSchema schema;
> @@ -225,6 +242,8 @@ int main(int argc, char *argv[])
>  
>      qtest_add_func("qmp/object-add-without-props",
>                     test_object_add_without_props);
> +    qtest_add_func("qmp/object-add-duplicate-id",
> +                   test_object_add_with_duplicate_id);
>      /* TODO: add coverage of generic object-add failure modes */
>  
>      ret = g_test_run();
> 

It would be nice also to add a test for adding the same object twice,
first failing and then succeeding (the difference between v2 and v3).
However it can be added as a separate patch.

Paolo

Re: [PATCH v3 2/2] tests/qmp-cmd-test: Add qmp/object-add-duplicate-id

[Markus Armbruster]

Eric Auger <eric.auger@redhat.com> writes:

> This new test checks that attempting to create an object
> with an existing ID gracefully fails.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Acked-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/qtest/qmp-cmd-test.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
> index 9f5228cd99..ceb3296691 100644
> --- a/tests/qtest/qmp-cmd-test.c
> +++ b/tests/qtest/qmp-cmd-test.c
> @@ -213,6 +213,23 @@ static void test_object_add_without_props(void)
>      qtest_quit(qts);
>  }
>  
> +static void test_object_add_with_duplicate_id(void)
> +{
> +    QTestState *qts;
> +    QDict *resp;
> +
> +    qts = qtest_init(common_args);
> +    resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
> +                    " {'qom-type': 'memory-backend-ram', 'id': 'ram1', 'props': {'size': 4294967296 } } }");
> +    g_assert_nonnull(resp);
> +    g_assert(qdict_haskey(resp, "return"));
> +    resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
> +                    " {'qom-type': 'memory-backend-ram', 'id': 'ram1', 'props': {'size': 4294967296 } } }");
> +    g_assert_nonnull(resp);
> +    qmp_assert_error_class(resp, "GenericError");
> +    qtest_quit(qts);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>      QmpSchema schema;
> @@ -225,6 +242,8 @@ int main(int argc, char *argv[])
>  
>      qtest_add_func("qmp/object-add-without-props",
>                     test_object_add_without_props);
> +    qtest_add_func("qmp/object-add-duplicate-id",
> +                   test_object_add_with_duplicate_id);
>      /* TODO: add coverage of generic object-add failure modes */
>  
>      ret = g_test_run();

One test case per error gets tedious real quick.  I'd add a test case
for generic failures (resolving the TODO), test duplicate ID there, and
add a TODO for the remaining generic failures.

No reason to withhold my
Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Auger Eric]

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
>