All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v4 0/2] Fixes/tests for hmp_object_del()
@ 2016-12-15 17:24 Michael Roth
  2016-12-15 17:24 ` [Qemu-devel] [PATCH 1/2] tests: check-qom-proplist: add checks for cmdline-created objects Michael Roth
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Michael Roth @ 2016-12-15 17:24 UTC (permalink / raw)
  To: qemu-devel

hmp_object_del() followed by a subsequent hmp_object_add() can trigger a
duplicate ID error if the previous object shared the same ID and was added
via the command-line. Please see patch 2/2 for more details.

This patchset fixes the issue in question and adds some general unit tests
for object created via -object, which we later extend to verify the fix in
question.

Changes since v3:

  - Fixed up comment formating (Markus)
  - Instead of segfaulting, use &error_abort if assumptions about
    'object' property group existence change (Markus)
  - Use g_assert_null in place of g_assert(... == NULL) (Markus)

Changes since v2:

  - Moved the generic unit tests ahead of the fix patch, with a FIXME
    in place of the actual check for the failure addressed in patch
    2/2 (Daniel/Markus)
  - Dropped check for existence of objects' QemuOptsList (Markus)
  - Dropped unintended whitespace removal in PATCH 1/2
  - Slight rewording of commit messages to reflect the changes and fix
    minor grammar errors.

Changes since v1:

  - Moved QemuOpt cleanup out of {qmp,hmp}_object_del() and into common
    user_creatable_del() path (Daniel, David)
  - Added corresponding test case in check-qom-proplist

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 1/2] tests: check-qom-proplist: add checks for cmdline-created objects
  2016-12-15 17:24 [Qemu-devel] [PATCH v4 0/2] Fixes/tests for hmp_object_del() Michael Roth
@ 2016-12-15 17:24 ` Michael Roth
  2017-05-12 10:22   ` Dr. David Alan Gilbert
  2016-12-15 17:24 ` [Qemu-devel] [PATCH 2/2] monitor: fix object_del for command-line-created objects Michael Roth
  2017-01-13 12:51 ` [Qemu-devel] [PATCH v4 0/2] Fixes/tests for hmp_object_del() Markus Armbruster
  2 siblings, 1 reply; 9+ messages in thread
From: Michael Roth @ 2016-12-15 17:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Dr. David Alan Gilbert, Markus Armbruster, Eric Blake,
	Daniel Berrange, qemu-stable

check-qom-proplist originally added tests for verifying that
object-creation helpers object_new_with_{props,propv} behaved in
similar fashion to the "traditional" method involving setting each
individual property separately after object creation rather than
via a single call.

Another similar "helper" for creating Objects exists in the form of
objects specified via -object command-line parameters. By that
rationale, we extend check-qom-proplist to include similar checks
for command-line-created objects by employing the same
qemu_opts_parse()-based parsing the vl.c employs.

This parser has a side-effect of parsing the object's options into
a QemuOpt structure and registering this in the global QemuOptsList
using the Object's ID. This can conflict with future Object instances
that attempt to use the same ID if we don't ensure this is cleaned
up as part of Object finalization, so we include a FIXME stub to test
for this case, which will then be resolved in a subsequent patch.

Suggested-by: Daniel Berrange <berrange@redhat.com>
Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Eric Blake <eblake@redhat.com>
Cc: Daniel Berrange <berrange@redhat.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 tests/check-qom-proplist.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index a16cefc..e3f56ca 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -23,6 +23,9 @@
 #include "qapi/error.h"
 #include "qom/object.h"
 #include "qemu/module.h"
+#include "qemu/option.h"
+#include "qemu/config-file.h"
+#include "qom/object_interfaces.h"
 
 
 #define TYPE_DUMMY "qemu-dummy"
@@ -162,6 +165,10 @@ static const TypeInfo dummy_info = {
     .instance_finalize = dummy_finalize,
     .class_size = sizeof(DummyObjectClass),
     .class_init = dummy_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_USER_CREATABLE },
+        { }
+    }
 };
 
 
@@ -320,6 +327,14 @@ static const TypeInfo dummy_backend_info = {
     .class_size = sizeof(DummyBackendClass),
 };
 
+static QemuOptsList qemu_object_opts = {
+    .name = "object",
+    .implied_opt_name = "qom-type",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
+    .desc = {
+        { }
+    },
+};
 
 
 static void test_dummy_createv(void)
@@ -388,6 +403,45 @@ static void test_dummy_createlist(void)
     object_unparent(OBJECT(dobj));
 }
 
+static void test_dummy_createcmdl(void)
+{
+    QemuOpts *opts;
+    DummyObject *dobj;
+    Error *err = NULL;
+    const char *params = TYPE_DUMMY \
+                         ",id=dev0," \
+                         "bv=yes,sv=Hiss hiss hiss,av=platypus";
+
+    qemu_add_opts(&qemu_object_opts);
+    opts = qemu_opts_parse(&qemu_object_opts, params, true, &err);
+    g_assert(err == NULL);
+    g_assert(opts);
+
+    dobj = DUMMY_OBJECT(user_creatable_add_opts(opts, &err));
+    g_assert(err == NULL);
+    g_assert(dobj);
+    g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
+    g_assert(dobj->bv == true);
+    g_assert(dobj->av == DUMMY_PLATYPUS);
+
+    user_creatable_del("dev0", &err);
+    g_assert(err == NULL);
+    error_free(err);
+
+    /* cmdline-parsing via qemu_opts_parse() results in a QemuOpts entry
+     * corresponding to the Object's ID to be added to the QemuOptsList
+     * for objects. To avoid having this entry conflict with future
+     * Objects using the same ID (which can happen in cases where
+     * qemu_opts_parse() is used to parse the object params, such as
+     * with hmp_object_add() at the time of this comment), we need to
+     * check for this in user_creatable_del() and remove the QemuOpts if
+     * it is present.
+     *
+     * FIXME: add an assert to verify that the QemuOpts is cleaned up
+     * once the corresponding cleanup code is added.
+     */
+}
+
 static void test_dummy_badenum(void)
 {
     Error *err = NULL;
@@ -525,6 +579,7 @@ int main(int argc, char **argv)
 
     g_test_add_func("/qom/proplist/createlist", test_dummy_createlist);
     g_test_add_func("/qom/proplist/createv", test_dummy_createv);
+    g_test_add_func("/qom/proplist/createcmdline", test_dummy_createcmdl);
     g_test_add_func("/qom/proplist/badenum", test_dummy_badenum);
     g_test_add_func("/qom/proplist/getenum", test_dummy_getenum);
     g_test_add_func("/qom/proplist/iterator", test_dummy_iterator);
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 2/2] monitor: fix object_del for command-line-created objects
  2016-12-15 17:24 [Qemu-devel] [PATCH v4 0/2] Fixes/tests for hmp_object_del() Michael Roth
  2016-12-15 17:24 ` [Qemu-devel] [PATCH 1/2] tests: check-qom-proplist: add checks for cmdline-created objects Michael Roth
@ 2016-12-15 17:24 ` Michael Roth
  2017-01-13 12:51 ` [Qemu-devel] [PATCH v4 0/2] Fixes/tests for hmp_object_del() Markus Armbruster
  2 siblings, 0 replies; 9+ messages in thread
From: Michael Roth @ 2016-12-15 17:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Dr. David Alan Gilbert, Markus Armbruster, Eric Blake,
	Daniel Berrange, qemu-stable

Currently objects specified on the command-line are only partially
cleaned up when 'object_del' is issued in either HMP or QMP: the
object itself is fully finalized, but the QemuOpts are not removed.
This results in the following behavior:

  x86_64-softmmu/qemu-system-x86_64 -monitor stdio \
    -object memory-backend-ram,id=ram1,size=256M

  QEMU 2.7.91 monitor - type 'help' for more information
  (qemu) object_del ram1
  (qemu) object_del ram1
  object 'ram1' not found
  (qemu) object_add memory-backend-ram,id=ram1,size=256M
  Duplicate ID 'ram1' for object
  Try "help object_add" for more information

which can be an issue for use-cases like memory hotplug.

This happens on the HMP side because hmp_object_add() attempts to
create a temporary QemuOpts entry with ID 'ram1', which ends up
conflicting with the command-line-created entry, since it was never
cleaned up during the previous hmp_object_del() call.

We address this by adding a check in user_creatable_del(), which
is called by both qmp_object_del() and hmp_object_del() to handle
the actual object cleanup, to determine whether an option group entry
matching the object's ID is present and removing it if it is.

Note that qmp_object_add() never attempts to create a temporary
QemuOpts entry, so it does not encounter the duplicate ID error,
which is why this isn't generally visible in libvirt.

Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Eric Blake <eblake@redhat.com>
Cc: Daniel Berrange <berrange@redhat.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 qom/object_interfaces.c    | 9 +++++++++
 tests/check-qom-proplist.c | 4 ++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index ded4d84..994d0c7 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -5,6 +5,7 @@
 #include "qapi-visit.h"
 #include "qapi/qobject-output-visitor.h"
 #include "qapi/opts-visitor.h"
+#include "qemu/config-file.h"
 
 void user_creatable_complete(Object *obj, Error **errp)
 {
@@ -209,6 +210,14 @@ void user_creatable_del(const char *id, Error **errp)
         error_setg(errp, "object '%s' is in use, can not be deleted", id);
         return;
     }
+
+    /*
+     * if object was defined on the command-line, remove its corresponding
+     * option group entry
+     */
+    qemu_opts_del(qemu_opts_find(qemu_find_opts_err("object", &error_abort),
+                                 id));
+
     object_unparent(obj);
 }
 
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index e3f56ca..ddec574 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -437,9 +437,9 @@ static void test_dummy_createcmdl(void)
      * check for this in user_creatable_del() and remove the QemuOpts if
      * it is present.
      *
-     * FIXME: add an assert to verify that the QemuOpts is cleaned up
-     * once the corresponding cleanup code is added.
+     * The below check ensures this works as expected.
      */
+    g_assert_null(qemu_opts_find(&qemu_object_opts, "dev0"));
 }
 
 static void test_dummy_badenum(void)
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH v4 0/2] Fixes/tests for hmp_object_del()
  2016-12-15 17:24 [Qemu-devel] [PATCH v4 0/2] Fixes/tests for hmp_object_del() Michael Roth
  2016-12-15 17:24 ` [Qemu-devel] [PATCH 1/2] tests: check-qom-proplist: add checks for cmdline-created objects Michael Roth
  2016-12-15 17:24 ` [Qemu-devel] [PATCH 2/2] monitor: fix object_del for command-line-created objects Michael Roth
@ 2017-01-13 12:51 ` Markus Armbruster
  2017-03-06 16:19   ` Michael Roth
  2 siblings, 1 reply; 9+ messages in thread
From: Markus Armbruster @ 2017-01-13 12:51 UTC (permalink / raw)
  To: Michael Roth; +Cc: qemu-devel, Andreas Färber

Andreas, please have a look.  Feel free to ask me to take it through my
tree.

Michael Roth <mdroth@linux.vnet.ibm.com> writes:

> hmp_object_del() followed by a subsequent hmp_object_add() can trigger a
> duplicate ID error if the previous object shared the same ID and was added
> via the command-line. Please see patch 2/2 for more details.
>
> This patchset fixes the issue in question and adds some general unit tests
> for object created via -object, which we later extend to verify the fix in
> question.
>
> Changes since v3:
>
>   - Fixed up comment formating (Markus)
>   - Instead of segfaulting, use &error_abort if assumptions about
>     'object' property group existence change (Markus)
>   - Use g_assert_null in place of g_assert(... == NULL) (Markus)
>
> Changes since v2:
>
>   - Moved the generic unit tests ahead of the fix patch, with a FIXME
>     in place of the actual check for the failure addressed in patch
>     2/2 (Daniel/Markus)
>   - Dropped check for existence of objects' QemuOptsList (Markus)
>   - Dropped unintended whitespace removal in PATCH 1/2
>   - Slight rewording of commit messages to reflect the changes and fix
>     minor grammar errors.
>
> Changes since v1:
>
>   - Moved QemuOpt cleanup out of {qmp,hmp}_object_del() and into common
>     user_creatable_del() path (Daniel, David)
>   - Added corresponding test case in check-qom-proplist

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH v4 0/2] Fixes/tests for hmp_object_del()
  2017-01-13 12:51 ` [Qemu-devel] [PATCH v4 0/2] Fixes/tests for hmp_object_del() Markus Armbruster
@ 2017-03-06 16:19   ` Michael Roth
  0 siblings, 0 replies; 9+ messages in thread
From: Michael Roth @ 2017-03-06 16:19 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: qemu-devel, Andreas Färber

Quoting Markus Armbruster (2017-01-13 06:51:57)
> Andreas, please have a look.  Feel free to ask me to take it through my
> tree.

Ping for 2.9. (sorry for not noticing this sooner)

> 
> Michael Roth <mdroth@linux.vnet.ibm.com> writes:
> 
> > hmp_object_del() followed by a subsequent hmp_object_add() can trigger a
> > duplicate ID error if the previous object shared the same ID and was added
> > via the command-line. Please see patch 2/2 for more details.
> >
> > This patchset fixes the issue in question and adds some general unit tests
> > for object created via -object, which we later extend to verify the fix in
> > question.
> >
> > Changes since v3:
> >
> >   - Fixed up comment formating (Markus)
> >   - Instead of segfaulting, use &error_abort if assumptions about
> >     'object' property group existence change (Markus)
> >   - Use g_assert_null in place of g_assert(... == NULL) (Markus)
> >
> > Changes since v2:
> >
> >   - Moved the generic unit tests ahead of the fix patch, with a FIXME
> >     in place of the actual check for the failure addressed in patch
> >     2/2 (Daniel/Markus)
> >   - Dropped check for existence of objects' QemuOptsList (Markus)
> >   - Dropped unintended whitespace removal in PATCH 1/2
> >   - Slight rewording of commit messages to reflect the changes and fix
> >     minor grammar errors.
> >
> > Changes since v1:
> >
> >   - Moved QemuOpt cleanup out of {qmp,hmp}_object_del() and into common
> >     user_creatable_del() path (Daniel, David)
> >   - Added corresponding test case in check-qom-proplist
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] tests: check-qom-proplist: add checks for cmdline-created objects
  2016-12-15 17:24 ` [Qemu-devel] [PATCH 1/2] tests: check-qom-proplist: add checks for cmdline-created objects Michael Roth
@ 2017-05-12 10:22   ` Dr. David Alan Gilbert
  2017-05-15  7:13     ` Markus Armbruster
  0 siblings, 1 reply; 9+ messages in thread
From: Dr. David Alan Gilbert @ 2017-05-12 10:22 UTC (permalink / raw)
  To: Michael Roth
  Cc: qemu-devel, Markus Armbruster, Eric Blake, Daniel Berrange, qemu-stable

I notice this pair of patches doesn't seem to have gone anywhere.
WHile it's labelled as a monitor fix, it's all QOM stuff, so I don't
think it should be going via me.

Dave

* Michael Roth (mdroth@linux.vnet.ibm.com) wrote:
> check-qom-proplist originally added tests for verifying that
> object-creation helpers object_new_with_{props,propv} behaved in
> similar fashion to the "traditional" method involving setting each
> individual property separately after object creation rather than
> via a single call.
> 
> Another similar "helper" for creating Objects exists in the form of
> objects specified via -object command-line parameters. By that
> rationale, we extend check-qom-proplist to include similar checks
> for command-line-created objects by employing the same
> qemu_opts_parse()-based parsing the vl.c employs.
> 
> This parser has a side-effect of parsing the object's options into
> a QemuOpt structure and registering this in the global QemuOptsList
> using the Object's ID. This can conflict with future Object instances
> that attempt to use the same ID if we don't ensure this is cleaned
> up as part of Object finalization, so we include a FIXME stub to test
> for this case, which will then be resolved in a subsequent patch.
> 
> Suggested-by: Daniel Berrange <berrange@redhat.com>
> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: Daniel Berrange <berrange@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/check-qom-proplist.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
> index a16cefc..e3f56ca 100644
> --- a/tests/check-qom-proplist.c
> +++ b/tests/check-qom-proplist.c
> @@ -23,6 +23,9 @@
>  #include "qapi/error.h"
>  #include "qom/object.h"
>  #include "qemu/module.h"
> +#include "qemu/option.h"
> +#include "qemu/config-file.h"
> +#include "qom/object_interfaces.h"
>  
>  
>  #define TYPE_DUMMY "qemu-dummy"
> @@ -162,6 +165,10 @@ static const TypeInfo dummy_info = {
>      .instance_finalize = dummy_finalize,
>      .class_size = sizeof(DummyObjectClass),
>      .class_init = dummy_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_USER_CREATABLE },
> +        { }
> +    }
>  };
>  
>  
> @@ -320,6 +327,14 @@ static const TypeInfo dummy_backend_info = {
>      .class_size = sizeof(DummyBackendClass),
>  };
>  
> +static QemuOptsList qemu_object_opts = {
> +    .name = "object",
> +    .implied_opt_name = "qom-type",
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
> +    .desc = {
> +        { }
> +    },
> +};
>  
>  
>  static void test_dummy_createv(void)
> @@ -388,6 +403,45 @@ static void test_dummy_createlist(void)
>      object_unparent(OBJECT(dobj));
>  }
>  
> +static void test_dummy_createcmdl(void)
> +{
> +    QemuOpts *opts;
> +    DummyObject *dobj;
> +    Error *err = NULL;
> +    const char *params = TYPE_DUMMY \
> +                         ",id=dev0," \
> +                         "bv=yes,sv=Hiss hiss hiss,av=platypus";
> +
> +    qemu_add_opts(&qemu_object_opts);
> +    opts = qemu_opts_parse(&qemu_object_opts, params, true, &err);
> +    g_assert(err == NULL);
> +    g_assert(opts);
> +
> +    dobj = DUMMY_OBJECT(user_creatable_add_opts(opts, &err));
> +    g_assert(err == NULL);
> +    g_assert(dobj);
> +    g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
> +    g_assert(dobj->bv == true);
> +    g_assert(dobj->av == DUMMY_PLATYPUS);
> +
> +    user_creatable_del("dev0", &err);
> +    g_assert(err == NULL);
> +    error_free(err);
> +
> +    /* cmdline-parsing via qemu_opts_parse() results in a QemuOpts entry
> +     * corresponding to the Object's ID to be added to the QemuOptsList
> +     * for objects. To avoid having this entry conflict with future
> +     * Objects using the same ID (which can happen in cases where
> +     * qemu_opts_parse() is used to parse the object params, such as
> +     * with hmp_object_add() at the time of this comment), we need to
> +     * check for this in user_creatable_del() and remove the QemuOpts if
> +     * it is present.
> +     *
> +     * FIXME: add an assert to verify that the QemuOpts is cleaned up
> +     * once the corresponding cleanup code is added.
> +     */
> +}
> +
>  static void test_dummy_badenum(void)
>  {
>      Error *err = NULL;
> @@ -525,6 +579,7 @@ int main(int argc, char **argv)
>  
>      g_test_add_func("/qom/proplist/createlist", test_dummy_createlist);
>      g_test_add_func("/qom/proplist/createv", test_dummy_createv);
> +    g_test_add_func("/qom/proplist/createcmdline", test_dummy_createcmdl);
>      g_test_add_func("/qom/proplist/badenum", test_dummy_badenum);
>      g_test_add_func("/qom/proplist/getenum", test_dummy_getenum);
>      g_test_add_func("/qom/proplist/iterator", test_dummy_iterator);
> -- 
> 1.9.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] tests: check-qom-proplist: add checks for cmdline-created objects
  2017-05-12 10:22   ` Dr. David Alan Gilbert
@ 2017-05-15  7:13     ` Markus Armbruster
  2017-05-31 17:05       ` Markus Armbruster
  0 siblings, 1 reply; 9+ messages in thread
From: Markus Armbruster @ 2017-05-15  7:13 UTC (permalink / raw)
  To: Dr. David Alan Gilbert
  Cc: Michael Roth, qemu-stable, qemu-devel, Andreas Färber

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> I notice this pair of patches doesn't seem to have gone anywhere.
> WHile it's labelled as a monitor fix, it's all QOM stuff, so I don't
> think it should be going via me.

It's somewhere between QOM and QemuOpts.  Andreas, please have a look.
Feel free to ask me to take the patch through my tree.

> Dave
>
> * Michael Roth (mdroth@linux.vnet.ibm.com) wrote:
>> check-qom-proplist originally added tests for verifying that
>> object-creation helpers object_new_with_{props,propv} behaved in
>> similar fashion to the "traditional" method involving setting each
>> individual property separately after object creation rather than
>> via a single call.
>> 
>> Another similar "helper" for creating Objects exists in the form of
>> objects specified via -object command-line parameters. By that
>> rationale, we extend check-qom-proplist to include similar checks
>> for command-line-created objects by employing the same
>> qemu_opts_parse()-based parsing the vl.c employs.
>> 
>> This parser has a side-effect of parsing the object's options into
>> a QemuOpt structure and registering this in the global QemuOptsList
>> using the Object's ID. This can conflict with future Object instances
>> that attempt to use the same ID if we don't ensure this is cleaned
>> up as part of Object finalization, so we include a FIXME stub to test
>> for this case, which will then be resolved in a subsequent patch.
>> 
>> Suggested-by: Daniel Berrange <berrange@redhat.com>
>> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Cc: Eric Blake <eblake@redhat.com>
>> Cc: Daniel Berrange <berrange@redhat.com>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/check-qom-proplist.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 55 insertions(+)
>> 
>> diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
>> index a16cefc..e3f56ca 100644
>> --- a/tests/check-qom-proplist.c
>> +++ b/tests/check-qom-proplist.c
>> @@ -23,6 +23,9 @@
>>  #include "qapi/error.h"
>>  #include "qom/object.h"
>>  #include "qemu/module.h"
>> +#include "qemu/option.h"
>> +#include "qemu/config-file.h"
>> +#include "qom/object_interfaces.h"
>>  
>>  
>>  #define TYPE_DUMMY "qemu-dummy"
>> @@ -162,6 +165,10 @@ static const TypeInfo dummy_info = {
>>      .instance_finalize = dummy_finalize,
>>      .class_size = sizeof(DummyObjectClass),
>>      .class_init = dummy_class_init,
>> +    .interfaces = (InterfaceInfo[]) {
>> +        { TYPE_USER_CREATABLE },
>> +        { }
>> +    }
>>  };
>>  
>>  
>> @@ -320,6 +327,14 @@ static const TypeInfo dummy_backend_info = {
>>      .class_size = sizeof(DummyBackendClass),
>>  };
>>  
>> +static QemuOptsList qemu_object_opts = {
>> +    .name = "object",
>> +    .implied_opt_name = "qom-type",
>> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
>> +    .desc = {
>> +        { }
>> +    },
>> +};
>>  
>>  
>>  static void test_dummy_createv(void)
>> @@ -388,6 +403,45 @@ static void test_dummy_createlist(void)
>>      object_unparent(OBJECT(dobj));
>>  }
>>  
>> +static void test_dummy_createcmdl(void)
>> +{
>> +    QemuOpts *opts;
>> +    DummyObject *dobj;
>> +    Error *err = NULL;
>> +    const char *params = TYPE_DUMMY \
>> +                         ",id=dev0," \
>> +                         "bv=yes,sv=Hiss hiss hiss,av=platypus";
>> +
>> +    qemu_add_opts(&qemu_object_opts);
>> +    opts = qemu_opts_parse(&qemu_object_opts, params, true, &err);
>> +    g_assert(err == NULL);
>> +    g_assert(opts);
>> +
>> +    dobj = DUMMY_OBJECT(user_creatable_add_opts(opts, &err));
>> +    g_assert(err == NULL);
>> +    g_assert(dobj);
>> +    g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
>> +    g_assert(dobj->bv == true);
>> +    g_assert(dobj->av == DUMMY_PLATYPUS);
>> +
>> +    user_creatable_del("dev0", &err);
>> +    g_assert(err == NULL);
>> +    error_free(err);
>> +
>> +    /* cmdline-parsing via qemu_opts_parse() results in a QemuOpts entry
>> +     * corresponding to the Object's ID to be added to the QemuOptsList
>> +     * for objects. To avoid having this entry conflict with future
>> +     * Objects using the same ID (which can happen in cases where
>> +     * qemu_opts_parse() is used to parse the object params, such as
>> +     * with hmp_object_add() at the time of this comment), we need to
>> +     * check for this in user_creatable_del() and remove the QemuOpts if
>> +     * it is present.
>> +     *
>> +     * FIXME: add an assert to verify that the QemuOpts is cleaned up
>> +     * once the corresponding cleanup code is added.
>> +     */
>> +}
>> +
>>  static void test_dummy_badenum(void)
>>  {
>>      Error *err = NULL;
>> @@ -525,6 +579,7 @@ int main(int argc, char **argv)
>>  
>>      g_test_add_func("/qom/proplist/createlist", test_dummy_createlist);
>>      g_test_add_func("/qom/proplist/createv", test_dummy_createv);
>> +    g_test_add_func("/qom/proplist/createcmdline", test_dummy_createcmdl);
>>      g_test_add_func("/qom/proplist/badenum", test_dummy_badenum);
>>      g_test_add_func("/qom/proplist/getenum", test_dummy_getenum);
>>      g_test_add_func("/qom/proplist/iterator", test_dummy_iterator);
>> -- 
>> 1.9.1
>> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] tests: check-qom-proplist: add checks for cmdline-created objects
  2017-05-15  7:13     ` Markus Armbruster
@ 2017-05-31 17:05       ` Markus Armbruster
  2017-06-03 23:16         ` Michael Roth
  0 siblings, 1 reply; 9+ messages in thread
From: Markus Armbruster @ 2017-05-31 17:05 UTC (permalink / raw)
  To: Dr. David Alan Gilbert
  Cc: Michael Roth, qemu-stable, qemu-devel, Andreas Färber

Markus Armbruster <armbru@redhat.com> writes:

> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>
>> I notice this pair of patches doesn't seem to have gone anywhere.
>> WHile it's labelled as a monitor fix, it's all QOM stuff, so I don't
>> think it should be going via me.
>
> It's somewhere between QOM and QemuOpts.  Andreas, please have a look.
> Feel free to ask me to take the patch through my tree.

I considered applying the series to qapi-next, and realized I'm not sure
what the latest version is.  I could peruse the archives, but that's
work.  Mike, would you mind resending your latest version?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] tests: check-qom-proplist: add checks for cmdline-created objects
  2017-05-31 17:05       ` Markus Armbruster
@ 2017-06-03 23:16         ` Michael Roth
  0 siblings, 0 replies; 9+ messages in thread
From: Michael Roth @ 2017-06-03 23:16 UTC (permalink / raw)
  To: Dr. David Alan Gilbert, Markus Armbruster
  Cc: qemu-devel, Andreas Färber, qemu-stable

Quoting Markus Armbruster (2017-05-31 12:05:16)
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> >
> >> I notice this pair of patches doesn't seem to have gone anywhere.
> >> WHile it's labelled as a monitor fix, it's all QOM stuff, so I don't
> >> think it should be going via me.
> >
> > It's somewhere between QOM and QemuOpts.  Andreas, please have a look.
> > Feel free to ask me to take the patch through my tree.
> 
> I considered applying the series to qapi-next, and realized I'm not sure
> what the latest version is.  I could peruse the archives, but that's
> work.  Mike, would you mind resending your latest version?

This version (v4) is the latest, but I've rebased/re-tested on master
and re-submitted those as v5. Thanks!

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2017-06-03 23:17 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-15 17:24 [Qemu-devel] [PATCH v4 0/2] Fixes/tests for hmp_object_del() Michael Roth
2016-12-15 17:24 ` [Qemu-devel] [PATCH 1/2] tests: check-qom-proplist: add checks for cmdline-created objects Michael Roth
2017-05-12 10:22   ` Dr. David Alan Gilbert
2017-05-15  7:13     ` Markus Armbruster
2017-05-31 17:05       ` Markus Armbruster
2017-06-03 23:16         ` Michael Roth
2016-12-15 17:24 ` [Qemu-devel] [PATCH 2/2] monitor: fix object_del for command-line-created objects Michael Roth
2017-01-13 12:51 ` [Qemu-devel] [PATCH v4 0/2] Fixes/tests for hmp_object_del() Markus Armbruster
2017-03-06 16:19   ` Michael Roth

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.