[Qemu-devel] [PATCH for-2.8 0/2] Fixes/tests for hmp_object_del()

[Qemu-devel] [PATCH for-2.8 0/2] Fixes/tests for hmp_object_del()

[Michael Roth]

I'm tagging this as for-2.8 since there was interest expressed in getting
this fixed with the 2.8 timeframe:

  https://lists.gnu.org/archive/html/qemu-devel/2016-11/msg05488.html
  https://lists.gnu.org/archive/html/qemu-devel/2016-12/msg00607.html

I should note however that the issue noted in PATCH 1/2 is only visible when
using object_del via HMP, and not with QMP/libvirt. If anyone is willing/able
to pull this soon enough to make 2.8 that would be very much appreciated, but
I know this is quite late and I wouldn't consider this a release blocker.

Changes since v2:

  - 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 (Daniel)

[Qemu-devel] [PATCH for-2.8 1/2] monitor: fix object_del for command-line-created objects

[Michael Roth]

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>
---
 qom/object_interfaces.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index ded4d84..23849f9 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)
 {
@@ -197,6 +198,7 @@ void user_creatable_del(const char *id, Error **errp)
 {
     Object *container;
     Object *obj;
+    QemuOptsList *opt_group;
 
     container = object_get_objects_root();
     obj = object_resolve_path_component(container, id);
@@ -209,6 +211,15 @@ 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
+     */
+    opt_group = qemu_find_opts_err("object", NULL);
+    if (opt_group) {
+        qemu_opts_del(qemu_opts_find(opt_group, id));
+    }
+
     object_unparent(obj);
 }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH for-2.8 1/2] monitor: fix object_del for command-line-created objects

[Daniel P. Berrange]

On Tue, Dec 06, 2016 at 02:14:59PM -0600, Michael Roth wrote:
> 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>
> ---
>  qom/object_interfaces.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index ded4d84..23849f9 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)
>  {
> @@ -197,6 +198,7 @@ void user_creatable_del(const char *id, Error **errp)
>  {
>      Object *container;
>      Object *obj;
> +    QemuOptsList *opt_group;
>  
>      container = object_get_objects_root();
>      obj = object_resolve_path_component(container, id);
> @@ -209,6 +211,15 @@ 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
> +     */
> +    opt_group = qemu_find_opts_err("object", NULL);
> +    if (opt_group) {
> +        qemu_opts_del(qemu_opts_find(opt_group, id));
> +    }
> +
>      object_unparent(obj);
>  }

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [PATCH for-2.8 1/2] monitor: fix object_del for command-line-created objects

[Markus Armbruster]

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

> 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>
> ---
>  qom/object_interfaces.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index ded4d84..23849f9 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)
>  {
> @@ -197,6 +198,7 @@ void user_creatable_del(const char *id, Error **errp)
>  {
>      Object *container;
>      Object *obj;
> +    QemuOptsList *opt_group;
>  
>      container = object_get_objects_root();
>      obj = object_resolve_path_component(container, id);
> @@ -209,6 +211,15 @@ 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
> +     */
> +    opt_group = qemu_find_opts_err("object", NULL);
> +    if (opt_group) {

How can opt_group ever be null?

For what it's worth, we assume it can't in hmp_object_add() and main().

> +        qemu_opts_del(qemu_opts_find(opt_group, id));
> +    }
> +
>      object_unparent(obj);
>  }

Re: [Qemu-devel] [PATCH for-2.8 1/2] monitor: fix object_del for command-line-created objects

[Michael Roth]

Quoting Markus Armbruster (2016-12-07 04:36:20)
> Michael Roth <mdroth@linux.vnet.ibm.com> writes:
> 
> > 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>
> > ---
> >  qom/object_interfaces.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> > index ded4d84..23849f9 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)
> >  {
> > @@ -197,6 +198,7 @@ void user_creatable_del(const char *id, Error **errp)
> >  {
> >      Object *container;
> >      Object *obj;
> > +    QemuOptsList *opt_group;
> >  
> >      container = object_get_objects_root();
> >      obj = object_resolve_path_component(container, id);
> > @@ -209,6 +211,15 @@ 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
> > +     */
> > +    opt_group = qemu_find_opts_err("object", NULL);
> > +    if (opt_group) {
> 
> How can opt_group ever be null?
> 
> For what it's worth, we assume it can't in hmp_object_add() and main().

I was trying to avoid as many assumptions as possible since
user_creatable_complete() is kind of reaching out of it's scope here.
If we ever changed the behavior on the parsing side this could result in
a segfault that might slip through if this particular scenario isn't
specifically tested.

However, that's less of a concern now thanks to the unit tests that
Daniel suggested which would catch this breakage. So that kind of handles
my concerns. Will change it for v3.

> 
> > +        qemu_opts_del(qemu_opts_find(opt_group, id));
> > +    }
> > +
> >      object_unparent(obj);
> >  }
> 

[Qemu-devel] [PATCH for-2.8 2/2] tests: check-qom-proplist: add checks for cmdline-created objects

[Michael Roth]

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
in 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 add specific checks for this
scenario in addition to the normal sanity checks.

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>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 tests/check-qom-proplist.c | 54 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index a16cefc..087ce21 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 },
+        { }
+    }
 };
 
 
@@ -291,7 +298,6 @@ static void dummy_backend_init(Object *obj)
 {
 }
 
-
 static const TypeInfo dummy_dev_info = {
     .name          = TYPE_DUMMY_DEV,
     .parent        = TYPE_OBJECT,
@@ -320,6 +326,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 +402,43 @@ 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 check
+     * for this in user_creatable_del() and remove the QemuOpts if it
+     * is present. The below check ensures this works as expected.
+     */
+    g_assert(qemu_opts_find(&qemu_object_opts, "dev0") == NULL);
+}
+
 static void test_dummy_badenum(void)
 {
     Error *err = NULL;
@@ -525,6 +576,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

Re: [Qemu-devel] [PATCH for-2.8 2/2] tests: check-qom-proplist: add checks for cmdline-created objects

[Daniel P. Berrange]

On Tue, Dec 06, 2016 at 02:15:00PM -0600, Michael Roth 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
> in 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 add specific checks for this
> scenario in addition to the normal sanity checks.
> 
> 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>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  tests/check-qom-proplist.c | 54 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
> index a16cefc..087ce21 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 },
> +        { }
> +    }
>  };
>  
>  
> @@ -291,7 +298,6 @@ static void dummy_backend_init(Object *obj)
>  {
>  }
>  
> -
>  static const TypeInfo dummy_dev_info = {
>      .name          = TYPE_DUMMY_DEV,
>      .parent        = TYPE_OBJECT,
> @@ -320,6 +326,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 +402,43 @@ 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 check
> +     * for this in user_creatable_del() and remove the QemuOpts if it
> +     * is present. The below check ensures this works as expected.
> +     */
> +    g_assert(qemu_opts_find(&qemu_object_opts, "dev0") == NULL);
> +}
> +
>  static void test_dummy_badenum(void)
>  {
>      Error *err = NULL;
> @@ -525,6 +576,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);

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>


except that this should be squashed into the previous commit - tests should
always be in the same commit as the new code that they're verifying.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [PATCH for-2.8 2/2] tests: check-qom-proplist: add checks for cmdline-created objects

[Markus Armbruster]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Tue, Dec 06, 2016 at 02:15:00PM -0600, Michael Roth 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
>> in 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 add specific checks for this
>> scenario in addition to the normal sanity checks.
>> 
>> 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>
>> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>> ---
>>  tests/check-qom-proplist.c | 54 +++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 53 insertions(+), 1 deletion(-)
>> 
>> diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
>> index a16cefc..087ce21 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 },
>> +        { }
>> +    }
>>  };
>>  
>>  
>> @@ -291,7 +298,6 @@ static void dummy_backend_init(Object *obj)
>>  {
>>  }
>>  
>> -
>>  static const TypeInfo dummy_dev_info = {
>>      .name          = TYPE_DUMMY_DEV,
>>      .parent        = TYPE_OBJECT,
>> @@ -320,6 +326,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 +402,43 @@ 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 check
>> +     * for this in user_creatable_del() and remove the QemuOpts if it
>> +     * is present. The below check ensures this works as expected.
>> +     */
>> +    g_assert(qemu_opts_find(&qemu_object_opts, "dev0") == NULL);
>> +}
>> +
>>  static void test_dummy_badenum(void)
>>  {
>>      Error *err = NULL;
>> @@ -525,6 +576,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);
>
> Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
>
>
> except that this should be squashed into the previous commit - tests should
> always be in the same commit as the new code that they're verifying.

Not sure how particular we are about that.

When neither test nor fix is trivial, I personally like to commit the
test first, defanged so it doesn't blow up make check, with a suitable
FIXME comment.  The fix then resolves the FIXME.

Re: [Qemu-devel] [PATCH for-2.8 2/2] tests: check-qom-proplist: add checks for cmdline-created objects

[Michael Roth]

Quoting Markus Armbruster (2016-12-07 05:10:47)
> "Daniel P. Berrange" <berrange@redhat.com> writes:
> 
> > On Tue, Dec 06, 2016 at 02:15:00PM -0600, Michael Roth 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
> >> in 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 add specific checks for this
> >> scenario in addition to the normal sanity checks.
> >> 
> >> 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>
> >> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> >> ---
> >>  tests/check-qom-proplist.c | 54 +++++++++++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 53 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
> >> index a16cefc..087ce21 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 },
> >> +        { }
> >> +    }
> >>  };
> >>  
> >>  
> >> @@ -291,7 +298,6 @@ static void dummy_backend_init(Object *obj)
> >>  {
> >>  }
> >>  
> >> -
> >>  static const TypeInfo dummy_dev_info = {
> >>      .name          = TYPE_DUMMY_DEV,
> >>      .parent        = TYPE_OBJECT,
> >> @@ -320,6 +326,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 +402,43 @@ 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 check
> >> +     * for this in user_creatable_del() and remove the QemuOpts if it
> >> +     * is present. The below check ensures this works as expected.
> >> +     */
> >> +    g_assert(qemu_opts_find(&qemu_object_opts, "dev0") == NULL);
> >> +}
> >> +
> >>  static void test_dummy_badenum(void)
> >>  {
> >>      Error *err = NULL;
> >> @@ -525,6 +576,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);
> >
> > Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
> >
> >
> > except that this should be squashed into the previous commit - tests should
> > always be in the same commit as the new code that they're verifying.
> 
> Not sure how particular we are about that.
> 
> When neither test nor fix is trivial, I personally like to commit the
> test first, defanged so it doesn't blow up make check, with a suitable
> FIXME comment.  The fix then resolves the FIXME.

I think this approach also makes sense here due to the fact that the
bulk of the checks added are pertaining to general sanity checks.
There's really only one assert here that checks the fix in question.
I'll switch it around and replace that check with a FIXME, then squash
the check into the fix patch.

>