[Qemu-devel] [PATCH v3 0/7] qom: misc fixes & enhancements to support TLS work

[Qemu-devel] [PATCH v3 0/7] qom: misc fixes & enhancements to support TLS work

[Daniel P. Berrange]

This series contains the 7 generic QOM API fixes and enhancements
that I previously posted as part of the large series refactoring
and extending the TLS support in QEMU:

  https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02038.html

I'm sending it separately, since the patches are reasonably well
self-contained and thus hopefully suitable for quicker review and
merge.

Changed in v3:

 - Fix test suite additions for change in object_new_propv API

Changed in v2:

 - Pass "Object * parent" instead of "char *path" paremeter
 - Rely on stable reference from parent to keep new object alive
 - Use object_unparent() where appropriate

Daniel P. Berrange (7):
  qom: fix typename of 'policy' enum property in hostmem obj
  qom: document user creatable object types in help text
  qom: create objects in two phases
  qom: add object_new_propv / object_new_proplist constructors
  qom: make enum string tables const-correct
  qom: add a object_property_add_enum helper method
  qom: don't pass string table to object_get_enum method

 backends/hostmem.c          |  22 ++--
 include/hw/qdev-core.h      |   2 +-
 include/qapi/util.h         |   2 +-
 include/qapi/visitor-impl.h |   6 +-
 include/qapi/visitor.h      |   2 +-
 include/qom/object.h        |  90 ++++++++++++-
 numa.c                      |   2 +-
 qapi/qapi-dealloc-visitor.c |   3 +-
 qapi/qapi-util.c            |   2 +-
 qapi/qapi-visit-core.c      |   6 +-
 qemu-options.hx             |  70 +++++++---
 qom/object.c                | 144 +++++++++++++++++++-
 scripts/qapi-types.py       |   4 +-
 tests/.gitignore            |   1 +
 tests/Makefile              |   5 +-
 tests/check-qom-proplist.c  | 310 ++++++++++++++++++++++++++++++++++++++++++++
 vl.c                        |  40 +++++-
 17 files changed, 661 insertions(+), 50 deletions(-)
 create mode 100644 tests/check-qom-proplist.c

-- 
2.1.0

[Qemu-devel] [PATCH v3 1/7] qom: fix typename of 'policy' enum property in hostmem obj

[Daniel P. Berrange]

The 'policy' property was being registered with a typename of
'str', but it is in fact an enum of the 'HostMemPolicy' type.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 backends/hostmem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/backends/hostmem.c b/backends/hostmem.c
index b7b6cf8..f6db33c 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -252,7 +252,7 @@ static void host_memory_backend_init(Object *obj)
     object_property_add(obj, "host-nodes", "int",
                         host_memory_backend_get_host_nodes,
                         host_memory_backend_set_host_nodes, NULL, NULL, NULL);
-    object_property_add(obj, "policy", "str",
+    object_property_add(obj, "policy", "HostMemPolicy",
                         host_memory_backend_get_policy,
                         host_memory_backend_set_policy, NULL, NULL, NULL);
 }
-- 
2.1.0

Re: [Qemu-devel] [PATCH v3 1/7] qom: fix typename of 'policy' enum property in hostmem obj

[Andreas Färber]

Am 01.05.2015 um 12:30 schrieb Daniel P. Berrange:
> The 'policy' property was being registered with a typename of
> 'str', but it is in fact an enum of the 'HostMemPolicy' type.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  backends/hostmem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index b7b6cf8..f6db33c 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -252,7 +252,7 @@ static void host_memory_backend_init(Object *obj)
>      object_property_add(obj, "host-nodes", "int",
>                          host_memory_backend_get_host_nodes,
>                          host_memory_backend_set_host_nodes, NULL, NULL, NULL);
> -    object_property_add(obj, "policy", "str",
> +    object_property_add(obj, "policy", "HostMemPolicy",
>                          host_memory_backend_get_policy,
>                          host_memory_backend_set_policy, NULL, NULL, NULL);
>  }

This patch is mislabeled IMO and none of my business, but cleanup is
appreciated,

Acked-by: Andreas Färber <afaerber@suse.de>

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

[Qemu-devel] [PATCH v3 2/7] qom: document user creatable object types in help text

[Daniel P. Berrange]

The QEMU help for -object is essentially useless, just giving users
the generic syntax. Move it down into its own section and introduce
a nested table where each user creatable object can be documented.
The existing memory-backend-file, rng-random and rng-egd object
types are documented.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qemu-options.hx | 70 ++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 54 insertions(+), 16 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 319d971..5ef0ae4 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3421,22 +3421,6 @@ DEF("no-kvm-irqchip", 0, QEMU_OPTION_no_kvm_irqchip, "", QEMU_ARCH_I386)
 HXCOMM Deprecated (ignored)
 DEF("tdf", 0, QEMU_OPTION_tdf,"", QEMU_ARCH_ALL)
 
-DEF("object", HAS_ARG, QEMU_OPTION_object,
-    "-object TYPENAME[,PROP1=VALUE1,...]\n"
-    "                create an new object of type TYPENAME setting properties\n"
-    "                in the order they are specified.  Note that the 'id'\n"
-    "                property must be set.  These objects are placed in the\n"
-    "                '/objects' path.\n",
-    QEMU_ARCH_ALL)
-STEXI
-@item -object @var{typename}[,@var{prop1}=@var{value1},...]
-@findex -object
-Create an new object of type @var{typename} setting properties
-in the order they are specified.  Note that the 'id'
-property must be set.  These objects are placed in the
-'/objects' path.
-ETEXI
-
 DEF("msg", HAS_ARG, QEMU_OPTION_msg,
     "-msg timestamp[=on|off]\n"
     "                change the format of messages\n"
@@ -3462,6 +3446,60 @@ Dump json-encoded vmstate information for current machine type to file
 in @var{file}
 ETEXI
 
+DEFHEADING(Generic object creation)
+
+DEF("object", HAS_ARG, QEMU_OPTION_object,
+    "-object TYPENAME[,PROP1=VALUE1,...]\n"
+    "                create an new object of type TYPENAME setting properties\n"
+    "                in the order they are specified.  Note that the 'id'\n"
+    "                property must be set.  These objects are placed in the\n"
+    "                '/objects' path.\n",
+    QEMU_ARCH_ALL)
+STEXI
+@item -object @var{typename}[,@var{prop1}=@var{value1},...]
+@findex -object
+Create an new object of type @var{typename} setting properties
+in the order they are specified.  Note that the 'id'
+property must be set.  These objects are placed in the
+'/objects' path.
+
+@table @option
+
+@item -object memory-backend-file,id=@var{id},size=@var{size},mem-path=@var{dir},share=@var{on|off}
+
+Creates a memory file backend object, which can be used to back
+the guest RAM with huge pages. The @option{id} parameter is a
+unique ID that will be used to reference this memory region
+when configuring the @option{-numa} argument. The @option{size}
+option provides the size of the memory region, and accepts
+common suffixes, eg @option{500M}. The @option{mem-path} provides
+the path to either a shared memory or huge page filesystem mount.
+The @option{share} boolean option determines whether the memory
+region is marked as private to QEMU, or shared. The latter allows
+a co-operating external process to access the QEMU memory region.
+
+@item -object rng-random,id=@var{id},filename=@var{/dev/random}
+
+Creates a random number generator backend which obtains entropy from
+a device on the host. The @option{id} parameter is a unique ID that
+will be used to reference this entropy backend from the @option{virtio-rng}
+device. The @option{filename} parameter specifies which file to obtain
+entropy from and if omitted defaults to @option{/dev/random}.
+
+@item -object rng-egd,id=@var{id},chardev=@var{chardevid}
+
+Creates a random number generator backend which obtains entropy from
+an external daemon running on the host. The @option{id} parameter is
+a unique ID that will be used to reference this entropy backend from
+the @option{virtio-rng} device. The @option{chardev} parameter is
+the unique ID of a character device backend that provides the connection
+to the RNG daemon.
+
+@end table
+
+ETEXI
+
+
 HXCOMM This is the last statement. Insert new options before this line!
 STEXI
 @end table
-- 
2.1.0

Re: [Qemu-devel] [PATCH v3 2/7] qom: document user creatable object types in help text

[Andreas Färber]

Am 01.05.2015 um 12:30 schrieb Daniel P. Berrange:
> The QEMU help for -object is essentially useless, just giving users
> the generic syntax. Move it down into its own section and introduce
> a nested table where each user creatable object can be documented.
> The existing memory-backend-file, rng-random and rng-egd object
> types are documented.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  qemu-options.hx | 70 ++++++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 54 insertions(+), 16 deletions(-)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 319d971..5ef0ae4 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3421,22 +3421,6 @@ DEF("no-kvm-irqchip", 0, QEMU_OPTION_no_kvm_irqchip, "", QEMU_ARCH_I386)
>  HXCOMM Deprecated (ignored)
>  DEF("tdf", 0, QEMU_OPTION_tdf,"", QEMU_ARCH_ALL)
>  
> -DEF("object", HAS_ARG, QEMU_OPTION_object,
> -    "-object TYPENAME[,PROP1=VALUE1,...]\n"
> -    "                create an new object of type TYPENAME setting properties\n"
> -    "                in the order they are specified.  Note that the 'id'\n"
> -    "                property must be set.  These objects are placed in the\n"
> -    "                '/objects' path.\n",
> -    QEMU_ARCH_ALL)
> -STEXI
> -@item -object @var{typename}[,@var{prop1}=@var{value1},...]
> -@findex -object
> -Create an new object of type @var{typename} setting properties
> -in the order they are specified.  Note that the 'id'
> -property must be set.  These objects are placed in the
> -'/objects' path.
> -ETEXI
> -
>  DEF("msg", HAS_ARG, QEMU_OPTION_msg,
>      "-msg timestamp[=on|off]\n"
>      "                change the format of messages\n"
> @@ -3462,6 +3446,60 @@ Dump json-encoded vmstate information for current machine type to file
>  in @var{file}
>  ETEXI
>  
> +DEFHEADING(Generic object creation)
> +
> +DEF("object", HAS_ARG, QEMU_OPTION_object,
> +    "-object TYPENAME[,PROP1=VALUE1,...]\n"
> +    "                create an new object of type TYPENAME setting properties\n"

I see you're only moving this, but should we fix the typo while doing so
or as follow-up?

Apart from that,

Reviewed-by: Andreas Färber <afaerber@suse.de>

but again not really "qom:" but rather Paolo's -object domain.

Regards,
Andreas

> +    "                in the order they are specified.  Note that the 'id'\n"
> +    "                property must be set.  These objects are placed in the\n"
> +    "                '/objects' path.\n",
> +    QEMU_ARCH_ALL)
> +STEXI
> +@item -object @var{typename}[,@var{prop1}=@var{value1},...]
> +@findex -object
> +Create an new object of type @var{typename} setting properties
> +in the order they are specified.  Note that the 'id'
> +property must be set.  These objects are placed in the
> +'/objects' path.
> +
> +@table @option
> +
> +@item -object memory-backend-file,id=@var{id},size=@var{size},mem-path=@var{dir},share=@var{on|off}
> +
> +Creates a memory file backend object, which can be used to back
> +the guest RAM with huge pages. The @option{id} parameter is a
> +unique ID that will be used to reference this memory region
> +when configuring the @option{-numa} argument. The @option{size}
> +option provides the size of the memory region, and accepts
> +common suffixes, eg @option{500M}. The @option{mem-path} provides
> +the path to either a shared memory or huge page filesystem mount.
> +The @option{share} boolean option determines whether the memory
> +region is marked as private to QEMU, or shared. The latter allows
> +a co-operating external process to access the QEMU memory region.
> +
> +@item -object rng-random,id=@var{id},filename=@var{/dev/random}
> +
> +Creates a random number generator backend which obtains entropy from
> +a device on the host. The @option{id} parameter is a unique ID that
> +will be used to reference this entropy backend from the @option{virtio-rng}
> +device. The @option{filename} parameter specifies which file to obtain
> +entropy from and if omitted defaults to @option{/dev/random}.
> +
> +@item -object rng-egd,id=@var{id},chardev=@var{chardevid}
> +
> +Creates a random number generator backend which obtains entropy from
> +an external daemon running on the host. The @option{id} parameter is
> +a unique ID that will be used to reference this entropy backend from
> +the @option{virtio-rng} device. The @option{chardev} parameter is
> +the unique ID of a character device backend that provides the connection
> +to the RNG daemon.
> +
> +@end table
> +
> +ETEXI
> +
> +
>  HXCOMM This is the last statement. Insert new options before this line!
>  STEXI
>  @end table
> 


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

[Qemu-devel] [PATCH v3 3/7] qom: create objects in two phases

[Daniel P. Berrange]

Some types of object must be created before chardevs, other types of
object must be created after chardevs. As such there is no option but
to create objects in two phases.

This takes the decision to create as many object types as possible
in the first phase, and only delay those which have a dependency on
the chardevs. Hopefully the set which need delaying will remain
small.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 vl.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index 74c2681..ee2f70a 100644
--- a/vl.c
+++ b/vl.c
@@ -2591,6 +2591,33 @@ static int machine_set_property(const char *name, const char *value,
     return 0;
 }
 
+
+/**
+ * Initial object creation happens before all other
+ * QEMU data types are created. The majority of objects
+ * can be created at this point. The rng-egd object
+ * cannot be created here, as it depends on the chardev
+ * already existing.
+ */
+static bool object_create_initial(const char *type)
+{
+    if (g_str_equal(type, "rng-egd")) {
+        return false;
+    }
+    return true;
+}
+
+
+/**
+ * The remainder of object creation happens after the
+ * creation of chardev, fsdev and device data types.
+ */
+static bool object_create_delayed(const char *type)
+{
+    return !object_create_initial(type);
+}
+
+
 static int object_create(QemuOpts *opts, void *opaque)
 {
     Error *err = NULL;
@@ -2599,6 +2626,7 @@ static int object_create(QemuOpts *opts, void *opaque)
     void *dummy = NULL;
     OptsVisitor *ov;
     QDict *pdict;
+    bool (*type_predicate)(const char *) = opaque;
 
     ov = opts_visitor_new(opts);
     pdict = qemu_opts_to_qdict(opts, NULL);
@@ -2613,6 +2641,9 @@ static int object_create(QemuOpts *opts, void *opaque)
     if (err) {
         goto out;
     }
+    if (!type_predicate(type)) {
+        goto out;
+    }
 
     qdict_del(pdict, "id");
     visit_type_str(opts_get_visitor(ov), &id, "id", &err);
@@ -4008,6 +4039,12 @@ int main(int argc, char **argv, char **envp)
 
     socket_init();
 
+    if (qemu_opts_foreach(qemu_find_opts("object"),
+                          object_create,
+                          object_create_initial, 0) != 0) {
+        exit(1);
+    }
+
     if (qemu_opts_foreach(qemu_find_opts("chardev"), chardev_init_func, NULL, 1) != 0)
         exit(1);
 #ifdef CONFIG_VIRTFS
@@ -4027,7 +4064,8 @@ int main(int argc, char **argv, char **envp)
     }
 
     if (qemu_opts_foreach(qemu_find_opts("object"),
-                          object_create, NULL, 0) != 0) {
+                          object_create,
+                          object_create_delayed, 0) != 0) {
         exit(1);
     }
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH v3 3/7] qom: create objects in two phases

[Andreas Färber]

Hi,

Can we *please* find a better subject for this? To me, creating QOM
objects in two phases is about instance_init vs. realize, and thus I was
pretty upset that Paolo dared to apply this without asking me first.

Am 01.05.2015 um 12:30 schrieb Daniel P. Berrange:
> Some types of object must be created before chardevs, other types of
> object must be created after chardevs. As such there is no option but
> to create objects in two phases.
> 
> This takes the decision to create as many object types as possible
> in the first phase, and only delay those which have a dependency on
> the chardevs. Hopefully the set which need delaying will remain
> small.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  vl.c | 40 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/vl.c b/vl.c
> index 74c2681..ee2f70a 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2591,6 +2591,33 @@ static int machine_set_property(const char *name, const char *value,
>      return 0;
>  }
>  
> +
> +/**

Accidental documentation comment?

> + * Initial object creation happens before all other
> + * QEMU data types are created. The majority of objects
> + * can be created at this point. The rng-egd object
> + * cannot be created here, as it depends on the chardev
> + * already existing.
> + */
> +static bool object_create_initial(const char *type)
> +{
> +    if (g_str_equal(type, "rng-egd")) {
> +        return false;
> +    }
> +    return true;
> +}
> +
> +
> +/**

Ditto?

> + * The remainder of object creation happens after the
> + * creation of chardev, fsdev and device data types.
> + */
> +static bool object_create_delayed(const char *type)
> +{
> +    return !object_create_initial(type);
> +}
> +
> +
>  static int object_create(QemuOpts *opts, void *opaque)
>  {
>      Error *err = NULL;
> @@ -2599,6 +2626,7 @@ static int object_create(QemuOpts *opts, void *opaque)
>      void *dummy = NULL;
>      OptsVisitor *ov;
>      QDict *pdict;
> +    bool (*type_predicate)(const char *) = opaque;
>  
>      ov = opts_visitor_new(opts);
>      pdict = qemu_opts_to_qdict(opts, NULL);
> @@ -2613,6 +2641,9 @@ static int object_create(QemuOpts *opts, void *opaque)
>      if (err) {
>          goto out;
>      }
> +    if (!type_predicate(type)) {
> +        goto out;
> +    }
>  
>      qdict_del(pdict, "id");
>      visit_type_str(opts_get_visitor(ov), &id, "id", &err);
> @@ -4008,6 +4039,12 @@ int main(int argc, char **argv, char **envp)
>  
>      socket_init();
>  
> +    if (qemu_opts_foreach(qemu_find_opts("object"),
> +                          object_create,
> +                          object_create_initial, 0) != 0) {
> +        exit(1);
> +    }
> +
>      if (qemu_opts_foreach(qemu_find_opts("chardev"), chardev_init_func, NULL, 1) != 0)
>          exit(1);
>  #ifdef CONFIG_VIRTFS
> @@ -4027,7 +4064,8 @@ int main(int argc, char **argv, char **envp)
>      }
>  
>      if (qemu_opts_foreach(qemu_find_opts("object"),
> -                          object_create, NULL, 0) != 0) {
> +                          object_create,
> +                          object_create_delayed, 0) != 0) {
>          exit(1);
>      }
>  

Otherwise looks okay and there's a pattern emerging of "not qom:".

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v3 3/7] qom: create objects in two phases

[Paolo Bonzini]

On 08/05/2015 16:37, Andreas Färber wrote:
> Hi,
> 
> Can we *please* find a better subject for this? To me, creating QOM
> objects in two phases is about instance_init vs. realize, and thus I was
> pretty upset that Paolo dared to apply this without asking me first.

Oops, sorry.  I very much understand where you came from, now.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH v3 3/7] qom: create objects in two phases

[Daniel P. Berrange]

On Fri, May 08, 2015 at 04:40:51PM +0200, Paolo Bonzini wrote:
> 
> 
> On 08/05/2015 16:37, Andreas Färber wrote:
> > Hi,
> > 
> > Can we *please* find a better subject for this? To me, creating QOM
> > objects in two phases is about instance_init vs. realize, and thus I was
> > pretty upset that Paolo dared to apply this without asking me first.
> 
> Oops, sorry.  I very much understand where you came from, now.

Ok, I'll change this to say something like

  "create most objects before creating chardev backends"

to better describe what its trying to achieve.

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

[Qemu-devel] [PATCH v3 4/7] qom: add object_new_propv / object_new_proplist constructors

[Daniel P. Berrange]

It is reasonably common to want to create an object, set a
number of properties, register it in the hierarchy and then
mark it as complete (if a user creatable type). This requires
quite a lot of error prone, verbose, boilerplate code to achieve.

The object_new_propv / object_new_proplist constructors will
simplify this task by performing all required steps in one go,
accepting the property names/values as variadic args.

Usage would be:

   Error *err = NULL;
   Object *obj;
   obj = object_new_propv(TYPE_MEMORY_BACKEND_FILE,
                          "/objects",
                          "hostmem0",
                          &err,
                          "share", "yes",
                          "mem-path", "/dev/shm/somefile",
                          "prealloc", "yes",
                          "size", "1048576",
                          NULL);

Note all property values are passed in string form and will
be parsed into their required data types.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 include/qom/object.h       |  67 ++++++++++++++++
 qom/object.c               |  66 ++++++++++++++++
 tests/.gitignore           |   1 +
 tests/Makefile             |   5 +-
 tests/check-qom-proplist.c | 190 +++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 328 insertions(+), 1 deletion(-)
 create mode 100644 tests/check-qom-proplist.c

diff --git a/include/qom/object.h b/include/qom/object.h
index d2d7748..15ac314 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -607,6 +607,73 @@ Object *object_new(const char *typename);
 Object *object_new_with_type(Type type);
 
 /**
+ * object_new_propv:
+ * @typename:  The name of the type of the object to instantiate.
+ * @parent: the parent object
+ * @id: The unique ID of the object
+ * @errp: pointer to error object
+ * @...: list of property names and values
+ *
+ * This function with initialize a new object using heap allocated memory.
+ * The returned object has a reference count of 1, and will be freed when
+ * the last reference is dropped.
+ *
+ * The @id parameter will be used when registering the object as a
+ * child of @parent in the objects hierarchy.
+ *
+ * The variadic parameters are a list of pairs of (propname, propvalue)
+ * strings. The propname of NULL indicates the end of the property
+ * list. If the object implements the user creatable interface, the
+ * object will be marked complete once all the properties have been
+ * processed.
+ *
+ *   Error *err = NULL;
+ *   Object *obj;
+ *
+ *   obj = object_new_propv(TYPE_MEMORY_BACKEND_FILE,
+ *                          container_get(object_get_root(), "/objects")
+ *                          "hostmem0",
+ *                          &err,
+ *                          "share", "yes",
+ *                          "mem-path", "/dev/shm/somefile",
+ *                          "prealloc", "yes",
+ *                          "size", "1048576",
+ *                          NULL);
+ *
+ *   if (!obj) {
+ *     g_printerr("Cannot create memory backend: %s\n",
+ *                error_get_pretty(err));
+ *   }
+ *
+ * The returned object will have one stable reference maintained
+ * for as long as it is present in the object hierarchy.
+ *
+ * Returns: The newly allocated, instantiated & initialized object.
+ */
+Object *object_new_propv(const char *typename,
+                         Object *parent,
+                         const char *id,
+                         Error **errp,
+                         ...)
+    __attribute__((sentinel));
+
+/**
+ * object_new_proplist:
+ * @typename:  The name of the type of the object to instantiate.
+ * @parent: the parent object
+ * @id: The unique ID of the object
+ * @errp: pointer to error object
+ * @vargs: list of property names and values
+ *
+ * See object_new_propv for documentation.
+ */
+Object *object_new_proplist(const char *typename,
+                            Object *parent,
+                            const char *id,
+                            Error **errp,
+                            va_list vargs);
+
+/**
  * object_initialize_with_type:
  * @data: A pointer to the memory to be used for the object.
  * @size: The maximum size available at @data for the object.
diff --git a/qom/object.c b/qom/object.c
index b8dff43..2115542 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -11,6 +11,7 @@
  */
 
 #include "qom/object.h"
+#include "qom/object_interfaces.h"
 #include "qemu-common.h"
 #include "qapi/visitor.h"
 #include "qapi-visit.h"
@@ -439,6 +440,71 @@ Object *object_new(const char *typename)
     return object_new_with_type(ti);
 }
 
+Object *object_new_propv(const char *typename,
+                         Object *parent,
+                         const char *id,
+                         Error **errp,
+                         ...)
+{
+    va_list vargs;
+    Object *obj;
+
+    va_start(vargs, errp);
+    obj = object_new_proplist(typename, parent, id, errp, vargs);
+    va_end(vargs);
+
+    return obj;
+}
+
+Object *object_new_proplist(const char *typename,
+                            Object *parent,
+                            const char *id,
+                            Error **errp,
+                            va_list vargs)
+{
+    Object *obj;
+    const char *propname;
+
+    obj = object_new(typename);
+
+    if (object_class_is_abstract(object_get_class(obj))) {
+        error_setg(errp, "object type '%s' is abstract", typename);
+        goto error;
+    }
+
+    propname = va_arg(vargs, char *);
+    while (propname != NULL) {
+        const char *value = va_arg(vargs, char *);
+
+        g_assert(value != NULL);
+        object_property_parse(obj, value, propname, errp);
+        if (*errp) {
+            goto error;
+        }
+        propname = va_arg(vargs, char *);
+    }
+
+    object_property_add_child(parent, id, obj, errp);
+    if (*errp) {
+        goto error;
+    }
+
+    if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
+        user_creatable_complete(obj, errp);
+        if (*errp) {
+            object_unparent(obj);
+            goto error;
+        }
+    }
+
+    object_unref(OBJECT(obj));
+    return obj;
+
+ error:
+    object_unref(obj);
+    return NULL;
+}
+
 Object *object_dynamic_cast(Object *obj, const char *typename)
 {
     if (obj && object_class_dynamic_cast(object_get_class(obj), typename)) {
diff --git a/tests/.gitignore b/tests/.gitignore
index 0dcb618..dc813c2 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -5,6 +5,7 @@ check-qjson
 check-qlist
 check-qstring
 check-qom-interface
+check-qom-proplist
 rcutorture
 test-aio
 test-bitops
diff --git a/tests/Makefile b/tests/Makefile
index 309e869..e0a831c 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -68,6 +68,8 @@ check-unit-y += tests/test-bitops$(EXESUF)
 check-unit-$(CONFIG_HAS_GLIB_SUBPROCESS_TESTS) += tests/test-qdev-global-props$(EXESUF)
 check-unit-y += tests/check-qom-interface$(EXESUF)
 gcov-files-check-qom-interface-y = qom/object.c
+check-unit-y += tests/check-qom-proplist$(EXESUF)
+gcov-files-check-qom-proplist-y = qom/object.c
 check-unit-y += tests/test-qemu-opts$(EXESUF)
 gcov-files-test-qemu-opts-y = qom/test-qemu-opts.c
 check-unit-y += tests/test-write-threshold$(EXESUF)
@@ -240,7 +242,7 @@ test-qapi-obj-y = tests/test-qapi-visit.o tests/test-qapi-types.o \
 
 $(test-obj-y): QEMU_INCLUDES += -Itests
 QEMU_CFLAGS += -I$(SRC_PATH)/tests
-qom-core-obj = qom/object.o qom/qom-qobject.o qom/container.o
+qom-core-obj = qom/object.o qom/qom-qobject.o qom/container.o qom/object_interfaces.o
 
 tests/check-qint$(EXESUF): tests/check-qint.o libqemuutil.a
 tests/check-qstring$(EXESUF): tests/check-qstring.o libqemuutil.a
@@ -249,6 +251,7 @@ tests/check-qlist$(EXESUF): tests/check-qlist.o libqemuutil.a
 tests/check-qfloat$(EXESUF): tests/check-qfloat.o libqemuutil.a
 tests/check-qjson$(EXESUF): tests/check-qjson.o libqemuutil.a libqemustub.a
 tests/check-qom-interface$(EXESUF): tests/check-qom-interface.o $(qom-core-obj) libqemuutil.a libqemustub.a
+tests/check-qom-proplist$(EXESUF): tests/check-qom-proplist.o $(qom-core-obj) libqemuutil.a libqemustub.a
 tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(block-obj-y) libqemuutil.a libqemustub.a
 tests/test-aio$(EXESUF): tests/test-aio.o $(block-obj-y) libqemuutil.a libqemustub.a
 tests/test-rfifolock$(EXESUF): tests/test-rfifolock.o libqemuutil.a libqemustub.a
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
new file mode 100644
index 0000000..9f16cdb
--- /dev/null
+++ b/tests/check-qom-proplist.c
@@ -0,0 +1,190 @@
+/*
+ * Copyright (C) 2015 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * <http://www.gnu.org/licenses/>.
+ *
+ * Author: Daniel P. Berrange <berrange@redhat.com>
+ */
+
+#include <glib.h>
+
+#include "qom/object.h"
+#include "qemu/module.h"
+
+
+#define TYPE_DUMMY "qemu:dummy"
+
+typedef struct DummyObject DummyObject;
+typedef struct DummyObjectClass DummyObjectClass;
+
+#define DUMMY_OBJECT(obj)                               \
+    OBJECT_CHECK(DummyObject, (obj), TYPE_DUMMY)
+
+struct DummyObject {
+    Object parent;
+
+    bool bv;
+    char *sv;
+};
+
+struct DummyObjectClass {
+    ObjectClass parent;
+};
+
+
+static void dummy_set_bv(Object *obj,
+                         bool value,
+                         Error **errp)
+{
+    DummyObject *dobj = DUMMY_OBJECT(obj);
+
+    dobj->bv = value;
+}
+
+static bool dummy_get_bv(Object *obj,
+                         Error **errp)
+{
+    DummyObject *dobj = DUMMY_OBJECT(obj);
+
+    return dobj->bv;
+}
+
+
+static void dummy_set_sv(Object *obj,
+                         const char *value,
+                         Error **errp)
+{
+    DummyObject *dobj = DUMMY_OBJECT(obj);
+
+    g_free(dobj->sv);
+    dobj->sv = g_strdup(value);
+}
+
+static char *dummy_get_sv(Object *obj,
+                          Error **errp)
+{
+    DummyObject *dobj = DUMMY_OBJECT(obj);
+
+    return g_strdup(dobj->sv);
+}
+
+
+static void dummy_init(Object *obj)
+{
+    object_property_add_bool(obj, "bv",
+                             dummy_get_bv,
+                             dummy_set_bv,
+                             NULL);
+    object_property_add_str(obj, "sv",
+                            dummy_get_sv,
+                            dummy_set_sv,
+                            NULL);
+}
+
+static void dummy_finalize(Object *obj)
+{
+    DummyObject *dobj = DUMMY_OBJECT(obj);
+
+    g_free(dobj->sv);
+}
+
+
+static const TypeInfo dummy_info = {
+    .name          = TYPE_DUMMY,
+    .parent        = TYPE_OBJECT,
+    .instance_size = sizeof(DummyObject),
+    .instance_init = dummy_init,
+    .instance_finalize = dummy_finalize,
+    .class_size = sizeof(DummyObjectClass),
+};
+
+static void test_dummy_createv(void)
+{
+    Error *err = NULL;
+    Object *parent = container_get(object_get_root(),
+                                   "/objects");
+    DummyObject *dobj = DUMMY_OBJECT(
+        object_new_propv(TYPE_DUMMY,
+                         parent,
+                         "dummy0",
+                         &err,
+                         "bv", "yes",
+                         "sv", "Hiss hiss hiss",
+                         NULL));
+
+    g_assert(dobj != NULL);
+    g_assert(err == NULL);
+    g_assert(g_str_equal(dobj->sv, "Hiss hiss hiss"));
+    g_assert(dobj->bv == true);
+
+    g_assert(object_resolve_path_component(parent, "dummy0")
+             == OBJECT(dobj));
+
+    object_unparent(OBJECT(dobj));
+}
+
+
+static Object *new_helper(Error **errp,
+                          Object *parent,
+                          ...)
+{
+    va_list vargs;
+    Object *obj;
+
+    va_start(vargs, parent);
+    obj = object_new_proplist(TYPE_DUMMY,
+                              parent,
+                              "dummy0",
+                              errp,
+                              vargs);
+    va_end(vargs);
+    return obj;
+}
+
+static void test_dummy_createlist(void)
+{
+    Error *err = NULL;
+    Object *parent = container_get(object_get_root(),
+                                   "/objects");
+    DummyObject *dobj = DUMMY_OBJECT(
+        new_helper(&err,
+                   parent,
+                   "bv", "yes",
+                   "sv", "Hiss hiss hiss",
+                   NULL));
+
+    g_assert(dobj != NULL);
+    g_assert(err == NULL);
+    g_assert(g_str_equal(dobj->sv, "Hiss hiss hiss"));
+    g_assert(dobj->bv == true);
+
+    g_assert(object_resolve_path_component(parent, "dummy0")
+             == OBJECT(dobj));
+
+    object_unparent(OBJECT(dobj));
+}
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+
+    module_call_init(MODULE_INIT_QOM);
+    type_register_static(&dummy_info);
+
+    g_test_add_func("/qom/proplist/createlist", test_dummy_createlist);
+    g_test_add_func("/qom/proplist/createv", test_dummy_createv);
+
+    return g_test_run();
+}
-- 
2.1.0

Re: [Qemu-devel] [PATCH v3 4/7] qom: add object_new_propv / object_new_proplist constructors

[Andreas Färber]

Hi Daniel/Paolo,

Am 01.05.2015 um 12:30 schrieb Daniel P. Berrange:
> It is reasonably common to want to create an object, set a
> number of properties, register it in the hierarchy and then
> mark it as complete (if a user creatable type). This requires
> quite a lot of error prone, verbose, boilerplate code to achieve.
> 
> The object_new_propv / object_new_proplist constructors will
> simplify this task by performing all required steps in one go,
> accepting the property names/values as variadic args.

With this I disagree. I can see the virtue of adding properties in one
go via some handy varargs function. But,

1) The function does something different from what its name implies to
me. It does not create a prop or proplist - instead of adding them it
sets existing ones. Suggest object_new_with_props()?

2) You seem to mix up *v and non-v functions. v is with va_list usually,
compare tests/libqtest.h.

3) Object construction is a tricky thing to get right. Anthony chose to
be stricter than C++ and not let object_new() fail, one of the reasons
we have the distinct realize step. Can we keep the two separate? qdev
with all its convenience helpers didn't mix those either.
I.e., use object_new() without Error** followed by object_set_props() or
anything with Error**. That tells you if there's an Error* you need to
unref the object. Otherwise it's in an unknown state.

4) What's the use case for this? I'm concerned about encouraging people
to hardcode properties like this, when doing it in C can let the
compiler detect any mismatches.

> 
> Usage would be:
> 
>    Error *err = NULL;
>    Object *obj;
>    obj = object_new_propv(TYPE_MEMORY_BACKEND_FILE,
>                           "/objects",

This is not an Object*. ;) I like it better as it's implemented below,
but cf. above for mixing this Error**-ing operation with object_new().

>                           "hostmem0",
>                           &err,
>                           "share", "yes",
>                           "mem-path", "/dev/shm/somefile",
>                           "prealloc", "yes",
>                           "size", "1048576",
>                           NULL);
> 
> Note all property values are passed in string form and will
> be parsed into their required data types.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  include/qom/object.h       |  67 ++++++++++++++++
>  qom/object.c               |  66 ++++++++++++++++
>  tests/.gitignore           |   1 +
>  tests/Makefile             |   5 +-
>  tests/check-qom-proplist.c | 190 +++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 328 insertions(+), 1 deletion(-)
>  create mode 100644 tests/check-qom-proplist.c
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index d2d7748..15ac314 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -607,6 +607,73 @@ Object *object_new(const char *typename);
>  Object *object_new_with_type(Type type);
>  
>  /**
> + * object_new_propv:
> + * @typename:  The name of the type of the object to instantiate.
> + * @parent: the parent object
> + * @id: The unique ID of the object
> + * @errp: pointer to error object
> + * @...: list of property names and values
> + *
> + * This function with initialize a new object using heap allocated memory.

Grammar. ("will"?)

> + * The returned object has a reference count of 1, and will be freed when
> + * the last reference is dropped.
> + *
> + * The @id parameter will be used when registering the object as a
> + * child of @parent in the objects hierarchy.

s/objects hierarchy/composition tree/

> + *
> + * The variadic parameters are a list of pairs of (propname, propvalue)
> + * strings. The propname of NULL indicates the end of the property

%NULL

> + * list. If the object implements the user creatable interface, the
> + * object will be marked complete once all the properties have been
> + * processed.
> + *
> + *   Error *err = NULL;
> + *   Object *obj;
> + *
> + *   obj = object_new_propv(TYPE_MEMORY_BACKEND_FILE,
> + *                          container_get(object_get_root(), "/objects")

If this is used in multiple places, please introduce a helper like I did
for /machine. The reason being avoiding hardcoded string paths.

> + *                          "hostmem0",
> + *                          &err,
> + *                          "share", "yes",
> + *                          "mem-path", "/dev/shm/somefile",
> + *                          "prealloc", "yes",
> + *                          "size", "1048576",
> + *                          NULL);
> + *
> + *   if (!obj) {
> + *     g_printerr("Cannot create memory backend: %s\n",
> + *                error_get_pretty(err));
> + *   }

Please see in the top of the file for examples how to enclose sample code.

> + *
> + * The returned object will have one stable reference maintained
> + * for as long as it is present in the object hierarchy.
> + *
> + * Returns: The newly allocated, instantiated & initialized object.
> + */
> +Object *object_new_propv(const char *typename,
> +                         Object *parent,
> +                         const char *id,
> +                         Error **errp,
> +                         ...)
> +    __attribute__((sentinel));

First time I see this in QEMU - is it safe to use unconditionally?
(clang, older gcc, etc.)

> +
> +/**
> + * object_new_proplist:
> + * @typename:  The name of the type of the object to instantiate.
> + * @parent: the parent object
> + * @id: The unique ID of the object
> + * @errp: pointer to error object
> + * @vargs: list of property names and values
> + *
> + * See object_new_propv for documentation.

Needs to be object_new_propv() for referencing.

> + */
> +Object *object_new_proplist(const char *typename,
> +                            Object *parent,
> +                            const char *id,
> +                            Error **errp,
> +                            va_list vargs);
> +
> +/**
>   * object_initialize_with_type:
>   * @data: A pointer to the memory to be used for the object.
>   * @size: The maximum size available at @data for the object.
> diff --git a/qom/object.c b/qom/object.c
> index b8dff43..2115542 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -11,6 +11,7 @@
>   */
>  
>  #include "qom/object.h"
> +#include "qom/object_interfaces.h"
>  #include "qemu-common.h"
>  #include "qapi/visitor.h"
>  #include "qapi-visit.h"
> @@ -439,6 +440,71 @@ Object *object_new(const char *typename)
>      return object_new_with_type(ti);
>  }
>  
> +Object *object_new_propv(const char *typename,
> +                         Object *parent,
> +                         const char *id,
> +                         Error **errp,
> +                         ...)
> +{
> +    va_list vargs;
> +    Object *obj;
> +
> +    va_start(vargs, errp);
> +    obj = object_new_proplist(typename, parent, id, errp, vargs);
> +    va_end(vargs);
> +
> +    return obj;
> +}
> +
> +Object *object_new_proplist(const char *typename,
> +                            Object *parent,
> +                            const char *id,
> +                            Error **errp,
> +                            va_list vargs)
> +{
> +    Object *obj;
> +    const char *propname;
> +
> +    obj = object_new(typename);
> +
> +    if (object_class_is_abstract(object_get_class(obj))) {

This check seems too late. If the type is abstract, object_new() will
have aborted.

> +        error_setg(errp, "object type '%s' is abstract", typename);
> +        goto error;
> +    }
> +
> +    propname = va_arg(vargs, char *);
> +    while (propname != NULL) {
> +        const char *value = va_arg(vargs, char *);
> +
> +        g_assert(value != NULL);
> +        object_property_parse(obj, value, propname, errp);
> +        if (*errp) {

This pattern is wrong. You need a local Error *err = NULL;, otherwise
you may be deferencing NULL.

> +            goto error;
> +        }
> +        propname = va_arg(vargs, char *);
> +    }
> +
> +    object_property_add_child(parent, id, obj, errp);
> +    if (*errp) {
> +        goto error;
> +    }
> +
> +    if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> +        user_creatable_complete(obj, errp);
> +        if (*errp) {
> +            object_unparent(obj);
> +            goto error;
> +        }
> +    }
> +
> +    object_unref(OBJECT(obj));
> +    return obj;
> +
> + error:

Intentionally indented?

> +    object_unref(obj);
> +    return NULL;
> +}
> +
>  Object *object_dynamic_cast(Object *obj, const char *typename)
>  {
>      if (obj && object_class_dynamic_cast(object_get_class(obj), typename)) {
> diff --git a/tests/.gitignore b/tests/.gitignore
> index 0dcb618..dc813c2 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -5,6 +5,7 @@ check-qjson
>  check-qlist
>  check-qstring
>  check-qom-interface
> +check-qom-proplist
>  rcutorture
>  test-aio
>  test-bitops
> diff --git a/tests/Makefile b/tests/Makefile
> index 309e869..e0a831c 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -68,6 +68,8 @@ check-unit-y += tests/test-bitops$(EXESUF)
>  check-unit-$(CONFIG_HAS_GLIB_SUBPROCESS_TESTS) += tests/test-qdev-global-props$(EXESUF)
>  check-unit-y += tests/check-qom-interface$(EXESUF)
>  gcov-files-check-qom-interface-y = qom/object.c
> +check-unit-y += tests/check-qom-proplist$(EXESUF)
> +gcov-files-check-qom-proplist-y = qom/object.c
>  check-unit-y += tests/test-qemu-opts$(EXESUF)
>  gcov-files-test-qemu-opts-y = qom/test-qemu-opts.c
>  check-unit-y += tests/test-write-threshold$(EXESUF)
> @@ -240,7 +242,7 @@ test-qapi-obj-y = tests/test-qapi-visit.o tests/test-qapi-types.o \
>  
>  $(test-obj-y): QEMU_INCLUDES += -Itests
>  QEMU_CFLAGS += -I$(SRC_PATH)/tests
> -qom-core-obj = qom/object.o qom/qom-qobject.o qom/container.o
> +qom-core-obj = qom/object.o qom/qom-qobject.o qom/container.o qom/object_interfaces.o
>  
>  tests/check-qint$(EXESUF): tests/check-qint.o libqemuutil.a
>  tests/check-qstring$(EXESUF): tests/check-qstring.o libqemuutil.a
> @@ -249,6 +251,7 @@ tests/check-qlist$(EXESUF): tests/check-qlist.o libqemuutil.a
>  tests/check-qfloat$(EXESUF): tests/check-qfloat.o libqemuutil.a
>  tests/check-qjson$(EXESUF): tests/check-qjson.o libqemuutil.a libqemustub.a
>  tests/check-qom-interface$(EXESUF): tests/check-qom-interface.o $(qom-core-obj) libqemuutil.a libqemustub.a
> +tests/check-qom-proplist$(EXESUF): tests/check-qom-proplist.o $(qom-core-obj) libqemuutil.a libqemustub.a
>  tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(block-obj-y) libqemuutil.a libqemustub.a
>  tests/test-aio$(EXESUF): tests/test-aio.o $(block-obj-y) libqemuutil.a libqemustub.a
>  tests/test-rfifolock$(EXESUF): tests/test-rfifolock.o libqemuutil.a libqemustub.a
> diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
> new file mode 100644
> index 0000000..9f16cdb
> --- /dev/null
> +++ b/tests/check-qom-proplist.c
> @@ -0,0 +1,190 @@
> +/*
> + * Copyright (C) 2015 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * <http://www.gnu.org/licenses/>.
> + *
> + * Author: Daniel P. Berrange <berrange@redhat.com>
> + */
> +
> +#include <glib.h>
> +
> +#include "qom/object.h"
> +#include "qemu/module.h"
> +
> +
> +#define TYPE_DUMMY "qemu:dummy"

Is colon considered valid in a type name?

> +
> +typedef struct DummyObject DummyObject;
> +typedef struct DummyObjectClass DummyObjectClass;
> +
> +#define DUMMY_OBJECT(obj)                               \
> +    OBJECT_CHECK(DummyObject, (obj), TYPE_DUMMY)
> +
> +struct DummyObject {
> +    Object parent;

parent_obj for consistency please.

> +
> +    bool bv;
> +    char *sv;
> +};
> +
> +struct DummyObjectClass {
> +    ObjectClass parent;

parent_class for consistency. If you copied these, please indicate from
where so that we can fix that.

> +};
> +
> +
> +static void dummy_set_bv(Object *obj,
> +                         bool value,
> +                         Error **errp)
> +{
> +    DummyObject *dobj = DUMMY_OBJECT(obj);
> +
> +    dobj->bv = value;
> +}
> +
> +static bool dummy_get_bv(Object *obj,
> +                         Error **errp)
> +{
> +    DummyObject *dobj = DUMMY_OBJECT(obj);
> +
> +    return dobj->bv;
> +}
> +
> +
> +static void dummy_set_sv(Object *obj,
> +                         const char *value,
> +                         Error **errp)
> +{
> +    DummyObject *dobj = DUMMY_OBJECT(obj);
> +
> +    g_free(dobj->sv);
> +    dobj->sv = g_strdup(value);
> +}
> +
> +static char *dummy_get_sv(Object *obj,
> +                          Error **errp)
> +{
> +    DummyObject *dobj = DUMMY_OBJECT(obj);
> +
> +    return g_strdup(dobj->sv);
> +}
> +
> +
> +static void dummy_init(Object *obj)
> +{
> +    object_property_add_bool(obj, "bv",
> +                             dummy_get_bv,
> +                             dummy_set_bv,
> +                             NULL);
> +    object_property_add_str(obj, "sv",
> +                            dummy_get_sv,
> +                            dummy_set_sv,
> +                            NULL);
> +}
> +
> +static void dummy_finalize(Object *obj)
> +{
> +    DummyObject *dobj = DUMMY_OBJECT(obj);
> +
> +    g_free(dobj->sv);
> +}
> +
> +
> +static const TypeInfo dummy_info = {
> +    .name          = TYPE_DUMMY,
> +    .parent        = TYPE_OBJECT,
> +    .instance_size = sizeof(DummyObject),
> +    .instance_init = dummy_init,
> +    .instance_finalize = dummy_finalize,
> +    .class_size = sizeof(DummyObjectClass),
> +};
> +
> +static void test_dummy_createv(void)
> +{
> +    Error *err = NULL;
> +    Object *parent = container_get(object_get_root(),
> +                                   "/objects");
> +    DummyObject *dobj = DUMMY_OBJECT(
> +        object_new_propv(TYPE_DUMMY,
> +                         parent,
> +                         "dummy0",
> +                         &err,
> +                         "bv", "yes",
> +                         "sv", "Hiss hiss hiss",
> +                         NULL));
> +
> +    g_assert(dobj != NULL);

I believe DUMMY_OBJECT() would assert in that case already. There is
object_dynamic_cast() in case that is undesired.

> +    g_assert(err == NULL);
> +    g_assert(g_str_equal(dobj->sv, "Hiss hiss hiss"));

Isn't there a GTester string comparison function for this that outputs
both strings in case of a mismatch?

> +    g_assert(dobj->bv == true);
> +
> +    g_assert(object_resolve_path_component(parent, "dummy0")
> +             == OBJECT(dobj));
> +
> +    object_unparent(OBJECT(dobj));
> +}
> +
> +
> +static Object *new_helper(Error **errp,
> +                          Object *parent,
> +                          ...)
> +{
> +    va_list vargs;
> +    Object *obj;
> +
> +    va_start(vargs, parent);
> +    obj = object_new_proplist(TYPE_DUMMY,
> +                              parent,
> +                              "dummy0",
> +                              errp,
> +                              vargs);
> +    va_end(vargs);
> +    return obj;
> +}
> +
> +static void test_dummy_createlist(void)
> +{
> +    Error *err = NULL;
> +    Object *parent = container_get(object_get_root(),
> +                                   "/objects");
> +    DummyObject *dobj = DUMMY_OBJECT(
> +        new_helper(&err,
> +                   parent,
> +                   "bv", "yes",
> +                   "sv", "Hiss hiss hiss",
> +                   NULL));
> +
> +    g_assert(dobj != NULL);
> +    g_assert(err == NULL);
> +    g_assert(g_str_equal(dobj->sv, "Hiss hiss hiss"));
> +    g_assert(dobj->bv == true);
> +
> +    g_assert(object_resolve_path_component(parent, "dummy0")
> +             == OBJECT(dobj));
> +
> +    object_unparent(OBJECT(dobj));
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    g_test_init(&argc, &argv, NULL);
> +
> +    module_call_init(MODULE_INIT_QOM);
> +    type_register_static(&dummy_info);
> +
> +    g_test_add_func("/qom/proplist/createlist", test_dummy_createlist);
> +    g_test_add_func("/qom/proplist/createv", test_dummy_createv);
> +
> +    return g_test_run();
> +}

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v3 4/7] qom: add object_new_propv / object_new_proplist constructors

[Paolo Bonzini]

On 08/05/2015 19:10, Andreas Färber wrote:
>>    Error *err = NULL;
>>    Object *obj;
>>    obj = object_new_propv(TYPE_MEMORY_BACKEND_FILE,
>>                           "/objects",
> 
> This is not an Object*. ;) I like it better as it's implemented below,
> but cf. above for mixing this Error**-ing operation with object_new().

Right, this was my main request on review and I had fixed up the commit
message in the pull request.

I'm certainly okay with a separate object_set_props function (better:
object_parse_props) and object_parse_propv for the va_list case.

Paolo

>>                           "hostmem0",
>>                           &err,
>>                           "share", "yes",
>>                           "mem-path", "/dev/shm/somefile",
>>                           "prealloc", "yes",
>>                           "size", "1048576",
>>                           NULL);
>>
>> Note all property values are passed in string form and will
>> be parsed into their required data types.
>>
>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>> ---
>>  include/qom/object.h       |  67 ++++++++++++++++
>>  qom/object.c               |  66 ++++++++++++++++
>>  tests/.gitignore           |   1 +
>>  tests/Makefile             |   5 +-
>>  tests/check-qom-proplist.c | 190 +++++++++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 328 insertions(+), 1 deletion(-)
>>  create mode 100644 tests/check-qom-proplist.c
>>
>> diff --git a/include/qom/object.h b/include/qom/object.h
>> index d2d7748..15ac314 100644
>> --- a/include/qom/object.h
>> +++ b/include/qom/object.h
>> @@ -607,6 +607,73 @@ Object *object_new(const char *typename);
>>  Object *object_new_with_type(Type type);
>>  
>>  /**
>> + * object_new_propv:
>> + * @typename:  The name of the type of the object to instantiate.
>> + * @parent: the parent object
>> + * @id: The unique ID of the object
>> + * @errp: pointer to error object
>> + * @...: list of property names and values
>> + *
>> + * This function with initialize a new object using heap allocated memory.
> 
> Grammar. ("will"?)
> 
>> + * The returned object has a reference count of 1, and will be freed when
>> + * the last reference is dropped.
>> + *
>> + * The @id parameter will be used when registering the object as a
>> + * child of @parent in the objects hierarchy.
> 
> s/objects hierarchy/composition tree/
> 
>> + *
>> + * The variadic parameters are a list of pairs of (propname, propvalue)
>> + * strings. The propname of NULL indicates the end of the property
> 
> %NULL
> 
>> + * list. If the object implements the user creatable interface, the
>> + * object will be marked complete once all the properties have been
>> + * processed.
>> + *
>> + *   Error *err = NULL;
>> + *   Object *obj;
>> + *
>> + *   obj = object_new_propv(TYPE_MEMORY_BACKEND_FILE,
>> + *                          container_get(object_get_root(), "/objects")
> 
> If this is used in multiple places, please introduce a helper like I did
> for /machine. The reason being avoiding hardcoded string paths.
> 
>> + *                          "hostmem0",
>> + *                          &err,
>> + *                          "share", "yes",
>> + *                          "mem-path", "/dev/shm/somefile",
>> + *                          "prealloc", "yes",
>> + *                          "size", "1048576",
>> + *                          NULL);
>> + *
>> + *   if (!obj) {
>> + *     g_printerr("Cannot create memory backend: %s\n",
>> + *                error_get_pretty(err));
>> + *   }
> 
> Please see in the top of the file for examples how to enclose sample code.
> 
>> + *
>> + * The returned object will have one stable reference maintained
>> + * for as long as it is present in the object hierarchy.
>> + *
>> + * Returns: The newly allocated, instantiated & initialized object.
>> + */
>> +Object *object_new_propv(const char *typename,
>> +                         Object *parent,
>> +                         const char *id,
>> +                         Error **errp,
>> +                         ...)
>> +    __attribute__((sentinel));
> 
> First time I see this in QEMU - is it safe to use unconditionally?
> (clang, older gcc, etc.)
> 
>> +
>> +/**
>> + * object_new_proplist:
>> + * @typename:  The name of the type of the object to instantiate.
>> + * @parent: the parent object
>> + * @id: The unique ID of the object
>> + * @errp: pointer to error object
>> + * @vargs: list of property names and values
>> + *
>> + * See object_new_propv for documentation.
> 
> Needs to be object_new_propv() for referencing.
> 
>> + */
>> +Object *object_new_proplist(const char *typename,
>> +                            Object *parent,
>> +                            const char *id,
>> +                            Error **errp,
>> +                            va_list vargs);
>> +
>> +/**
>>   * object_initialize_with_type:
>>   * @data: A pointer to the memory to be used for the object.
>>   * @size: The maximum size available at @data for the object.
>> diff --git a/qom/object.c b/qom/object.c
>> index b8dff43..2115542 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -11,6 +11,7 @@
>>   */
>>  
>>  #include "qom/object.h"
>> +#include "qom/object_interfaces.h"
>>  #include "qemu-common.h"
>>  #include "qapi/visitor.h"
>>  #include "qapi-visit.h"
>> @@ -439,6 +440,71 @@ Object *object_new(const char *typename)
>>      return object_new_with_type(ti);
>>  }
>>  
>> +Object *object_new_propv(const char *typename,
>> +                         Object *parent,
>> +                         const char *id,
>> +                         Error **errp,
>> +                         ...)
>> +{
>> +    va_list vargs;
>> +    Object *obj;
>> +
>> +    va_start(vargs, errp);
>> +    obj = object_new_proplist(typename, parent, id, errp, vargs);
>> +    va_end(vargs);
>> +
>> +    return obj;
>> +}
>> +
>> +Object *object_new_proplist(const char *typename,
>> +                            Object *parent,
>> +                            const char *id,
>> +                            Error **errp,
>> +                            va_list vargs)
>> +{
>> +    Object *obj;
>> +    const char *propname;
>> +
>> +    obj = object_new(typename);
>> +
>> +    if (object_class_is_abstract(object_get_class(obj))) {
> 
> This check seems too late. If the type is abstract, object_new() will
> have aborted.
> 
>> +        error_setg(errp, "object type '%s' is abstract", typename);
>> +        goto error;
>> +    }
>> +
>> +    propname = va_arg(vargs, char *);
>> +    while (propname != NULL) {
>> +        const char *value = va_arg(vargs, char *);
>> +
>> +        g_assert(value != NULL);
>> +        object_property_parse(obj, value, propname, errp);
>> +        if (*errp) {
> 
> This pattern is wrong. You need a local Error *err = NULL;, otherwise
> you may be deferencing NULL.
> 
>> +            goto error;
>> +        }
>> +        propname = va_arg(vargs, char *);
>> +    }
>> +
>> +    object_property_add_child(parent, id, obj, errp);
>> +    if (*errp) {
>> +        goto error;
>> +    }
>> +
>> +    if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
>> +        user_creatable_complete(obj, errp);
>> +        if (*errp) {
>> +            object_unparent(obj);
>> +            goto error;
>> +        }
>> +    }
>> +
>> +    object_unref(OBJECT(obj));
>> +    return obj;
>> +
>> + error:
> 
> Intentionally indented?
> 
>> +    object_unref(obj);
>> +    return NULL;
>> +}
>> +
>>  Object *object_dynamic_cast(Object *obj, const char *typename)
>>  {
>>      if (obj && object_class_dynamic_cast(object_get_class(obj), typename)) {
>> diff --git a/tests/.gitignore b/tests/.gitignore
>> index 0dcb618..dc813c2 100644
>> --- a/tests/.gitignore
>> +++ b/tests/.gitignore
>> @@ -5,6 +5,7 @@ check-qjson
>>  check-qlist
>>  check-qstring
>>  check-qom-interface
>> +check-qom-proplist
>>  rcutorture
>>  test-aio
>>  test-bitops
>> diff --git a/tests/Makefile b/tests/Makefile
>> index 309e869..e0a831c 100644
>> --- a/tests/Makefile
>> +++ b/tests/Makefile
>> @@ -68,6 +68,8 @@ check-unit-y += tests/test-bitops$(EXESUF)
>>  check-unit-$(CONFIG_HAS_GLIB_SUBPROCESS_TESTS) += tests/test-qdev-global-props$(EXESUF)
>>  check-unit-y += tests/check-qom-interface$(EXESUF)
>>  gcov-files-check-qom-interface-y = qom/object.c
>> +check-unit-y += tests/check-qom-proplist$(EXESUF)
>> +gcov-files-check-qom-proplist-y = qom/object.c
>>  check-unit-y += tests/test-qemu-opts$(EXESUF)
>>  gcov-files-test-qemu-opts-y = qom/test-qemu-opts.c
>>  check-unit-y += tests/test-write-threshold$(EXESUF)
>> @@ -240,7 +242,7 @@ test-qapi-obj-y = tests/test-qapi-visit.o tests/test-qapi-types.o \
>>  
>>  $(test-obj-y): QEMU_INCLUDES += -Itests
>>  QEMU_CFLAGS += -I$(SRC_PATH)/tests
>> -qom-core-obj = qom/object.o qom/qom-qobject.o qom/container.o
>> +qom-core-obj = qom/object.o qom/qom-qobject.o qom/container.o qom/object_interfaces.o
>>  
>>  tests/check-qint$(EXESUF): tests/check-qint.o libqemuutil.a
>>  tests/check-qstring$(EXESUF): tests/check-qstring.o libqemuutil.a
>> @@ -249,6 +251,7 @@ tests/check-qlist$(EXESUF): tests/check-qlist.o libqemuutil.a
>>  tests/check-qfloat$(EXESUF): tests/check-qfloat.o libqemuutil.a
>>  tests/check-qjson$(EXESUF): tests/check-qjson.o libqemuutil.a libqemustub.a
>>  tests/check-qom-interface$(EXESUF): tests/check-qom-interface.o $(qom-core-obj) libqemuutil.a libqemustub.a
>> +tests/check-qom-proplist$(EXESUF): tests/check-qom-proplist.o $(qom-core-obj) libqemuutil.a libqemustub.a
>>  tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(block-obj-y) libqemuutil.a libqemustub.a
>>  tests/test-aio$(EXESUF): tests/test-aio.o $(block-obj-y) libqemuutil.a libqemustub.a
>>  tests/test-rfifolock$(EXESUF): tests/test-rfifolock.o libqemuutil.a libqemustub.a
>> diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
>> new file mode 100644
>> index 0000000..9f16cdb
>> --- /dev/null
>> +++ b/tests/check-qom-proplist.c
>> @@ -0,0 +1,190 @@
>> +/*
>> + * Copyright (C) 2015 Red Hat, Inc.
>> + *
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library.  If not, see
>> + * <http://www.gnu.org/licenses/>.
>> + *
>> + * Author: Daniel P. Berrange <berrange@redhat.com>
>> + */
>> +
>> +#include <glib.h>
>> +
>> +#include "qom/object.h"
>> +#include "qemu/module.h"
>> +
>> +
>> +#define TYPE_DUMMY "qemu:dummy"
> 
> Is colon considered valid in a type name?
> 
>> +
>> +typedef struct DummyObject DummyObject;
>> +typedef struct DummyObjectClass DummyObjectClass;
>> +
>> +#define DUMMY_OBJECT(obj)                               \
>> +    OBJECT_CHECK(DummyObject, (obj), TYPE_DUMMY)
>> +
>> +struct DummyObject {
>> +    Object parent;
> 
> parent_obj for consistency please.
> 
>> +
>> +    bool bv;
>> +    char *sv;
>> +};
>> +
>> +struct DummyObjectClass {
>> +    ObjectClass parent;
> 
> parent_class for consistency. If you copied these, please indicate from
> where so that we can fix that.
> 
>> +};
>> +
>> +
>> +static void dummy_set_bv(Object *obj,
>> +                         bool value,
>> +                         Error **errp)
>> +{
>> +    DummyObject *dobj = DUMMY_OBJECT(obj);
>> +
>> +    dobj->bv = value;
>> +}
>> +
>> +static bool dummy_get_bv(Object *obj,
>> +                         Error **errp)
>> +{
>> +    DummyObject *dobj = DUMMY_OBJECT(obj);
>> +
>> +    return dobj->bv;
>> +}
>> +
>> +
>> +static void dummy_set_sv(Object *obj,
>> +                         const char *value,
>> +                         Error **errp)
>> +{
>> +    DummyObject *dobj = DUMMY_OBJECT(obj);
>> +
>> +    g_free(dobj->sv);
>> +    dobj->sv = g_strdup(value);
>> +}
>> +
>> +static char *dummy_get_sv(Object *obj,
>> +                          Error **errp)
>> +{
>> +    DummyObject *dobj = DUMMY_OBJECT(obj);
>> +
>> +    return g_strdup(dobj->sv);
>> +}
>> +
>> +
>> +static void dummy_init(Object *obj)
>> +{
>> +    object_property_add_bool(obj, "bv",
>> +                             dummy_get_bv,
>> +                             dummy_set_bv,
>> +                             NULL);
>> +    object_property_add_str(obj, "sv",
>> +                            dummy_get_sv,
>> +                            dummy_set_sv,
>> +                            NULL);
>> +}
>> +
>> +static void dummy_finalize(Object *obj)
>> +{
>> +    DummyObject *dobj = DUMMY_OBJECT(obj);
>> +
>> +    g_free(dobj->sv);
>> +}
>> +
>> +
>> +static const TypeInfo dummy_info = {
>> +    .name          = TYPE_DUMMY,
>> +    .parent        = TYPE_OBJECT,
>> +    .instance_size = sizeof(DummyObject),
>> +    .instance_init = dummy_init,
>> +    .instance_finalize = dummy_finalize,
>> +    .class_size = sizeof(DummyObjectClass),
>> +};
>> +
>> +static void test_dummy_createv(void)
>> +{
>> +    Error *err = NULL;
>> +    Object *parent = container_get(object_get_root(),
>> +                                   "/objects");
>> +    DummyObject *dobj = DUMMY_OBJECT(
>> +        object_new_propv(TYPE_DUMMY,
>> +                         parent,
>> +                         "dummy0",
>> +                         &err,
>> +                         "bv", "yes",
>> +                         "sv", "Hiss hiss hiss",
>> +                         NULL));
>> +
>> +    g_assert(dobj != NULL);
> 
> I believe DUMMY_OBJECT() would assert in that case already. There is
> object_dynamic_cast() in case that is undesired.
> 
>> +    g_assert(err == NULL);
>> +    g_assert(g_str_equal(dobj->sv, "Hiss hiss hiss"));
> 
> Isn't there a GTester string comparison function for this that outputs
> both strings in case of a mismatch?
> 
>> +    g_assert(dobj->bv == true);
>> +
>> +    g_assert(object_resolve_path_component(parent, "dummy0")
>> +             == OBJECT(dobj));
>> +
>> +    object_unparent(OBJECT(dobj));
>> +}
>> +
>> +
>> +static Object *new_helper(Error **errp,
>> +                          Object *parent,
>> +                          ...)
>> +{
>> +    va_list vargs;
>> +    Object *obj;
>> +
>> +    va_start(vargs, parent);
>> +    obj = object_new_proplist(TYPE_DUMMY,
>> +                              parent,
>> +                              "dummy0",
>> +                              errp,
>> +                              vargs);
>> +    va_end(vargs);
>> +    return obj;
>> +}
>> +
>> +static void test_dummy_createlist(void)
>> +{
>> +    Error *err = NULL;
>> +    Object *parent = container_get(object_get_root(),
>> +                                   "/objects");
>> +    DummyObject *dobj = DUMMY_OBJECT(
>> +        new_helper(&err,
>> +                   parent,
>> +                   "bv", "yes",
>> +                   "sv", "Hiss hiss hiss",
>> +                   NULL));
>> +
>> +    g_assert(dobj != NULL);
>> +    g_assert(err == NULL);
>> +    g_assert(g_str_equal(dobj->sv, "Hiss hiss hiss"));
>> +    g_assert(dobj->bv == true);
>> +
>> +    g_assert(object_resolve_path_component(parent, "dummy0")
>> +             == OBJECT(dobj));
>> +
>> +    object_unparent(OBJECT(dobj));
>> +}
>> +
>> +int main(int argc, char **argv)
>> +{
>> +    g_test_init(&argc, &argv, NULL);
>> +
>> +    module_call_init(MODULE_INIT_QOM);
>> +    type_register_static(&dummy_info);
>> +
>> +    g_test_add_func("/qom/proplist/createlist", test_dummy_createlist);
>> +    g_test_add_func("/qom/proplist/createv", test_dummy_createv);
>> +
>> +    return g_test_run();
>> +}
> 
> Regards,
> Andreas
> 

Re: [Qemu-devel] [PATCH v3 4/7] qom: add object_new_propv / object_new_proplist constructors

[Andreas Färber]

Am 08.05.2015 um 19:18 schrieb Paolo Bonzini:
> On 08/05/2015 19:10, Andreas Färber wrote:
>>>    Error *err = NULL;
>>>    Object *obj;
>>>    obj = object_new_propv(TYPE_MEMORY_BACKEND_FILE,
>>>                           "/objects",
>>
>> This is not an Object*. ;) I like it better as it's implemented below,
>> but cf. above for mixing this Error**-ing operation with object_new().
> 
> Right, this was my main request on review

Hm, didn't make it to the list then...? Only the one reply to 0/7.

Andreas

> and I had fixed up the commit
> message in the pull request.
> 
> I'm certainly okay with a separate object_set_props function (better:
> object_parse_props) and object_parse_propv for the va_list case.
> 
> Paolo

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v3 4/7] qom: add object_new_propv / object_new_proplist constructors

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1237 bytes --]

On 05/08/2015 11:10 AM, Andreas Färber wrote:
> Hi Daniel/Paolo,
> 
> Am 01.05.2015 um 12:30 schrieb Daniel P. Berrange:
>> It is reasonably common to want to create an object, set a
>> number of properties, register it in the hierarchy and then
>> mark it as complete (if a user creatable type). This requires
>> quite a lot of error prone, verbose, boilerplate code to achieve.
>>

>> +
>> +    object_unref(OBJECT(obj));
>> +    return obj;
>> +
>> + error:
> 
> Intentionally indented?

Yes. Emacs c-mode defaults to indenting like this on purpose, in order
to leave column 1 reserved for the start of a function.  Besides, things
like 'diff -p' search for content in column 1, and if top-level labels
are not indented to column 2, then they get interpreted as function
names, making the diff a bit less useful.

Libvirt has gone one step further and enforces this indentation style
during its 'make syntax-check'; I'm sure if we wanted to do likewise in
qemu, we could patch scripts/checkpatch.pl to enforce a particular
style.  But right now, I'm personally okay with not worrying about it.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 4/7] qom: add object_new_propv / object_new_proplist constructors

[Daniel P. Berrange]

On Fri, May 08, 2015 at 07:10:49PM +0200, Andreas Färber wrote:
> Hi Daniel/Paolo,
> 
> Am 01.05.2015 um 12:30 schrieb Daniel P. Berrange:
> > It is reasonably common to want to create an object, set a
> > number of properties, register it in the hierarchy and then
> > mark it as complete (if a user creatable type). This requires
> > quite a lot of error prone, verbose, boilerplate code to achieve.
> > 
> > The object_new_propv / object_new_proplist constructors will
> > simplify this task by performing all required steps in one go,
> > accepting the property names/values as variadic args.
> 
> With this I disagree. I can see the virtue of adding properties in one
> go via some handy varargs function. But,
> 
> 1) The function does something different from what its name implies to
> me. It does not create a prop or proplist - instead of adding them it
> sets existing ones. Suggest object_new_with_props()?

Sure, with_props() looks fine.

> 2) You seem to mix up *v and non-v functions. v is with va_list usually,
> compare tests/libqtest.h.

Ok, I didn't see that qemu had a convention on that, so will change
to match.

> 3) Object construction is a tricky thing to get right. Anthony chose to
> be stricter than C++ and not let object_new() fail, one of the reasons
> we have the distinct realize step. Can we keep the two separate? qdev
> with all its convenience helpers didn't mix those either.
> I.e., use object_new() without Error** followed by object_set_props() or
> anything with Error**. That tells you if there's an Error* you need to
> unref the object. Otherwise it's in an unknown state.

I don't really think that forcing the callers to call new + set_props
separately is really makng it more reliable - in fact the contrary - it
means that the callers have more complex boilerplate code which they all
have to tediously duplicate in exactly the same way. With the single
object_new_with_props call, you know that if it returns NULL then it
failed and you have no cleanup that you need todo which is about as
reliable as it gets.

That said, I can see the value in having a standalone object_set_props()
method as a general feature. So I will add that, and simply make the
object_new_with_props method call object_new + object_set_props + 

> 4) What's the use case for this? I'm concerned about encouraging people
> to hardcode properties like this, when doing it in C can let the
> compiler detect any mismatches.

I use it in the VNC server when I convert it to use generic TLS encryption
code over to use the QCryptoTLSCreds object - it reduced a 100+ line
method into just two calls to object_new_propv. See vnc_display_create_creds()
in this RFC patch:

  https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02062.html


Then, I've got a bunch of unit tests related to that series which are
using it, again to reduce the amount of code it takes to create and
set props on this TLS creds object.

> > 
> > Usage would be:
> > 
> >    Error *err = NULL;
> >    Object *obj;
> >    obj = object_new_propv(TYPE_MEMORY_BACKEND_FILE,
> >                           "/objects",
> 
> This is not an Object*. ;) I like it better as it's implemented below,
> but cf. above for mixing this Error**-ing operation with object_new().

Yep, that's a docs mistake.

> 
> >                           "hostmem0",
> >                           &err,
> >                           "share", "yes",
> >                           "mem-path", "/dev/shm/somefile",
> >                           "prealloc", "yes",
> >                           "size", "1048576",
> >                           NULL);
> > 
> > Note all property values are passed in string form and will
> > be parsed into their required data types.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  include/qom/object.h       |  67 ++++++++++++++++
> >  qom/object.c               |  66 ++++++++++++++++
> >  tests/.gitignore           |   1 +
> >  tests/Makefile             |   5 +-
> >  tests/check-qom-proplist.c | 190 +++++++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 328 insertions(+), 1 deletion(-)
> >  create mode 100644 tests/check-qom-proplist.c
> > 
> > diff --git a/include/qom/object.h b/include/qom/object.h
> > index d2d7748..15ac314 100644
> > --- a/include/qom/object.h
> > +++ b/include/qom/object.h
> > @@ -607,6 +607,73 @@ Object *object_new(const char *typename);
> >  Object *object_new_with_type(Type type);
> >  
> >  /**
> > + * object_new_propv:
> > + * @typename:  The name of the type of the object to instantiate.
> > + * @parent: the parent object
> > + * @id: The unique ID of the object
> > + * @errp: pointer to error object
> > + * @...: list of property names and values
> > + *
> > + * This function with initialize a new object using heap allocated memory.
> 
> Grammar. ("will"?)
> 
> > + * The returned object has a reference count of 1, and will be freed when
> > + * the last reference is dropped.
> > + *
> > + * The @id parameter will be used when registering the object as a
> > + * child of @parent in the objects hierarchy.
> 
> s/objects hierarchy/composition tree/
> 
> > + *
> > + * The variadic parameters are a list of pairs of (propname, propvalue)
> > + * strings. The propname of NULL indicates the end of the property
> 
> %NULL
> 
> > + * list. If the object implements the user creatable interface, the
> > + * object will be marked complete once all the properties have been
> > + * processed.
> > + *
> > + *   Error *err = NULL;
> > + *   Object *obj;
> > + *
> > + *   obj = object_new_propv(TYPE_MEMORY_BACKEND_FILE,
> > + *                          container_get(object_get_root(), "/objects")
> 
> If this is used in multiple places, please introduce a helper like I did
> for /machine. The reason being avoiding hardcoded string paths.

Sure, will do that.

> 
> > + *                          "hostmem0",
> > + *                          &err,
> > + *                          "share", "yes",
> > + *                          "mem-path", "/dev/shm/somefile",
> > + *                          "prealloc", "yes",
> > + *                          "size", "1048576",
> > + *                          NULL);
> > + *
> > + *   if (!obj) {
> > + *     g_printerr("Cannot create memory backend: %s\n",
> > + *                error_get_pretty(err));
> > + *   }
> 
> Please see in the top of the file for examples how to enclose sample code.

Ok.

> 
> > + *
> > + * The returned object will have one stable reference maintained
> > + * for as long as it is present in the object hierarchy.
> > + *
> > + * Returns: The newly allocated, instantiated & initialized object.
> > + */
> > +Object *object_new_propv(const char *typename,
> > +                         Object *parent,
> > +                         const char *id,
> > +                         Error **errp,
> > +                         ...)
> > +    __attribute__((sentinel));
> 
> First time I see this in QEMU - is it safe to use unconditionally?
> (clang, older gcc, etc.)

I'm actually not sure - will look into this.

> > +
> > +/**
> > + * object_new_proplist:
> > + * @typename:  The name of the type of the object to instantiate.
> > + * @parent: the parent object
> > + * @id: The unique ID of the object
> > + * @errp: pointer to error object
> > + * @vargs: list of property names and values
> > + *
> > + * See object_new_propv for documentation.
> 
> Needs to be object_new_propv() for referencing.

Ok

> 
> > + */
> > +Object *object_new_proplist(const char *typename,
> > +                            Object *parent,
> > +                            const char *id,
> > +                            Error **errp,
> > +                            va_list vargs);
> > +
> > +/**
> >   * object_initialize_with_type:
> >   * @data: A pointer to the memory to be used for the object.
> >   * @size: The maximum size available at @data for the object.
> > diff --git a/qom/object.c b/qom/object.c
> > index b8dff43..2115542 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -11,6 +11,7 @@
> >   */
> >  
> >  #include "qom/object.h"
> > +#include "qom/object_interfaces.h"
> >  #include "qemu-common.h"
> >  #include "qapi/visitor.h"
> >  #include "qapi-visit.h"
> > @@ -439,6 +440,71 @@ Object *object_new(const char *typename)
> >      return object_new_with_type(ti);
> >  }
> >  
> > +Object *object_new_propv(const char *typename,
> > +                         Object *parent,
> > +                         const char *id,
> > +                         Error **errp,
> > +                         ...)
> > +{
> > +    va_list vargs;
> > +    Object *obj;
> > +
> > +    va_start(vargs, errp);
> > +    obj = object_new_proplist(typename, parent, id, errp, vargs);
> > +    va_end(vargs);
> > +
> > +    return obj;
> > +}
> > +
> > +Object *object_new_proplist(const char *typename,
> > +                            Object *parent,
> > +                            const char *id,
> > +                            Error **errp,
> > +                            va_list vargs)
> > +{
> > +    Object *obj;
> > +    const char *propname;
> > +
> > +    obj = object_new(typename);
> > +
> > +    if (object_class_is_abstract(object_get_class(obj))) {
> 
> This check seems too late. If the type is abstract, object_new() will
> have aborted.

Ah, I didn't realize that it did that.

> 
> > +        error_setg(errp, "object type '%s' is abstract", typename);
> > +        goto error;
> > +    }
> > +
> > +    propname = va_arg(vargs, char *);
> > +    while (propname != NULL) {
> > +        const char *value = va_arg(vargs, char *);
> > +
> > +        g_assert(value != NULL);
> > +        object_property_parse(obj, value, propname, errp);
> > +        if (*errp) {
> 
> This pattern is wrong. You need a local Error *err = NULL;, otherwise
> you may be deferencing NULL.
> 
> > +            goto error;
> > +        }
> > +        propname = va_arg(vargs, char *);
> > +    }
> > +
> > +    object_property_add_child(parent, id, obj, errp);
> > +    if (*errp) {
> > +        goto error;
> > +    }
> > +
> > +    if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> > +        user_creatable_complete(obj, errp);
> > +        if (*errp) {
> > +            object_unparent(obj);
> > +            goto error;
> > +        }
> > +    }
> > +
> > +    object_unref(OBJECT(obj));
> > +    return obj;
> > +
> > + error:
> 
> Intentionally indented?

> > diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
> > new file mode 100644
> > index 0000000..9f16cdb
> > --- /dev/null
> > +++ b/tests/check-qom-proplist.c


> > +#define TYPE_DUMMY "qemu:dummy"
> 
> Is colon considered valid in a type name?

Nothing complained, but I can trivially remove the colon.

> > +#define DUMMY_OBJECT(obj)                               \
> > +    OBJECT_CHECK(DummyObject, (obj), TYPE_DUMMY)
> > +
> > +struct DummyObject {
> > +    Object parent;
> 
> parent_obj for consistency please.
> 
> > +
> > +    bool bv;
> > +    char *sv;
> > +};
> > +
> > +struct DummyObjectClass {
> > +    ObjectClass parent;
> 
> parent_class for consistency. If you copied these, please indicate from
> where so that we can fix that.

This is just the naming convention used by GObject and I hadn't noticed
that QEMU had its own convention. I'll change these as you suggest.


> > +static void test_dummy_createv(void)
> > +{
> > +    Error *err = NULL;
> > +    Object *parent = container_get(object_get_root(),
> > +                                   "/objects");
> > +    DummyObject *dobj = DUMMY_OBJECT(
> > +        object_new_propv(TYPE_DUMMY,
> > +                         parent,
> > +                         "dummy0",
> > +                         &err,
> > +                         "bv", "yes",
> > +                         "sv", "Hiss hiss hiss",
> > +                         NULL));
> > +
> > +    g_assert(dobj != NULL);
> 
> I believe DUMMY_OBJECT() would assert in that case already. There is
> object_dynamic_cast() in case that is undesired.

Ah, good point.

> > +    g_assert(err == NULL);
> > +    g_assert(g_str_equal(dobj->sv, "Hiss hiss hiss"));
> 
> Isn't there a GTester string comparison function for this that outputs
> both strings in case of a mismatch?

Possibly, I'll look into that.


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

[Qemu-devel] [PATCH v3 5/7] qom: make enum string tables const-correct

[Daniel P. Berrange]

The enum string table parameters in various QOM/QAPI methods
are declared 'const char *strings[]'. This results in const
warnings if passed a variable that was declared as

   static const char * const strings[] = { .... };

Add the extra const annotation to the parameters, since
neither the string elements, nor the array itself should
ever be modified.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 include/hw/qdev-core.h      | 2 +-
 include/qapi/util.h         | 2 +-
 include/qapi/visitor-impl.h | 6 +++---
 include/qapi/visitor.h      | 2 +-
 include/qom/object.h        | 2 +-
 qapi/qapi-dealloc-visitor.c | 3 ++-
 qapi/qapi-util.c            | 2 +-
 qapi/qapi-visit-core.c      | 6 +++---
 qom/object.c                | 2 +-
 scripts/qapi-types.py       | 4 ++--
 10 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 4e673f9..913963e 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -236,7 +236,7 @@ struct Property {
 struct PropertyInfo {
     const char *name;
     const char *description;
-    const char **enum_table;
+    const char * const *enum_table;
     int (*print)(DeviceState *dev, Property *prop, char *dest, size_t len);
     ObjectPropertyAccessor *get;
     ObjectPropertyAccessor *set;
diff --git a/include/qapi/util.h b/include/qapi/util.h
index de9238b..7ad26c0 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -11,7 +11,7 @@
 #ifndef QAPI_UTIL_H
 #define QAPI_UTIL_H
 
-int qapi_enum_parse(const char *lookup[], const char *buf,
+int qapi_enum_parse(const char * const lookup[], const char *buf,
                     int max, int def, Error **errp);
 
 #endif
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 09bb0fd..f4a2f74 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -30,7 +30,7 @@ struct Visitor
     GenericList *(*next_list)(Visitor *v, GenericList **list, Error **errp);
     void (*end_list)(Visitor *v, Error **errp);
 
-    void (*type_enum)(Visitor *v, int *obj, const char *strings[],
+    void (*type_enum)(Visitor *v, int *obj, const char * const strings[],
                       const char *kind, const char *name, Error **errp);
     void (*get_next_type)(Visitor *v, int *kind, const int *qobjects,
                           const char *name, Error **errp);
@@ -59,9 +59,9 @@ struct Visitor
     void (*end_union)(Visitor *v, bool data_present, Error **errp);
 };
 
-void input_type_enum(Visitor *v, int *obj, const char *strings[],
+void input_type_enum(Visitor *v, int *obj, const char * const strings[],
                      const char *kind, const char *name, Error **errp);
-void output_type_enum(Visitor *v, int *obj, const char *strings[],
+void output_type_enum(Visitor *v, int *obj, const char * const strings[],
                       const char *kind, const char *name, Error **errp);
 
 #endif
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 5934f59..00ba104 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -43,7 +43,7 @@ void visit_optional(Visitor *v, bool *present, const char *name,
                     Error **errp);
 void visit_get_next_type(Visitor *v, int *obj, const int *qtypes,
                          const char *name, Error **errp);
-void visit_type_enum(Visitor *v, int *obj, const char *strings[],
+void visit_type_enum(Visitor *v, int *obj, const char * const strings[],
                      const char *kind, const char *name, Error **errp);
 void visit_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp);
 void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error **errp);
diff --git a/include/qom/object.h b/include/qom/object.h
index 15ac314..bf76f7a 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1020,7 +1020,7 @@ int64_t object_property_get_int(Object *obj, const char *name,
  * an enum).
  */
 int object_property_get_enum(Object *obj, const char *name,
-                             const char *strings[], Error **errp);
+                             const char * const strings[], Error **errp);
 
 /**
  * object_property_get_uint16List:
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index a14a1c7..d7f92c5 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -156,7 +156,8 @@ static void qapi_dealloc_type_size(Visitor *v, uint64_t *obj, const char *name,
 {
 }
 
-static void qapi_dealloc_type_enum(Visitor *v, int *obj, const char *strings[],
+static void qapi_dealloc_type_enum(Visitor *v, int *obj,
+                                   const char * const strings[],
                                    const char *kind, const char *name,
                                    Error **errp)
 {
diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
index 1d8fb96..bcdc94d 100644
--- a/qapi/qapi-util.c
+++ b/qapi/qapi-util.c
@@ -14,7 +14,7 @@
 #include "qapi/error.h"
 #include "qapi/util.h"
 
-int qapi_enum_parse(const char *lookup[], const char *buf,
+int qapi_enum_parse(const char * const lookup[], const char *buf,
                     int max, int def, Error **errp)
 {
     int i;
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index b66b93a..a18ba16 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -89,7 +89,7 @@ void visit_get_next_type(Visitor *v, int *obj, const int *qtypes,
     }
 }
 
-void visit_type_enum(Visitor *v, int *obj, const char *strings[],
+void visit_type_enum(Visitor *v, int *obj, const char * const strings[],
                      const char *kind, const char *name, Error **errp)
 {
     v->type_enum(v, obj, strings, kind, name, errp);
@@ -260,7 +260,7 @@ void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp)
     v->type_number(v, obj, name, errp);
 }
 
-void output_type_enum(Visitor *v, int *obj, const char *strings[],
+void output_type_enum(Visitor *v, int *obj, const char * const strings[],
                       const char *kind, const char *name,
                       Error **errp)
 {
@@ -279,7 +279,7 @@ void output_type_enum(Visitor *v, int *obj, const char *strings[],
     visit_type_str(v, &enum_str, name, errp);
 }
 
-void input_type_enum(Visitor *v, int *obj, const char *strings[],
+void input_type_enum(Visitor *v, int *obj, const char * const strings[],
                      const char *kind, const char *name,
                      Error **errp)
 {
diff --git a/qom/object.c b/qom/object.c
index 2115542..077a5fe 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1027,7 +1027,7 @@ int64_t object_property_get_int(Object *obj, const char *name,
 }
 
 int object_property_get_enum(Object *obj, const char *name,
-                             const char *strings[], Error **errp)
+                             const char * const strings[], Error **errp)
 {
     StringOutputVisitor *sov;
     StringInputVisitor *siv;
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index db87218..c8d6db6 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -118,7 +118,7 @@ struct %(name)s
 
 def generate_enum_lookup(name, values):
     ret = mcgen('''
-const char *%(name)s_lookup[] = {
+const char * const %(name)s_lookup[] = {
 ''',
                          name=name)
     i = 0
@@ -140,7 +140,7 @@ const char *%(name)s_lookup[] = {
 
 def generate_enum(name, values):
     lookup_decl = mcgen('''
-extern const char *%(name)s_lookup[];
+extern const char * const %(name)s_lookup[];
 ''',
                 name=name)
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH v3 5/7] qom: make enum string tables const-correct

[Andreas Färber]

Am 01.05.2015 um 12:30 schrieb Daniel P. Berrange:
> The enum string table parameters in various QOM/QAPI methods
> are declared 'const char *strings[]'. This results in const
> warnings if passed a variable that was declared as
> 
>    static const char * const strings[] = { .... };
> 
> Add the extra const annotation to the parameters, since
> neither the string elements, nor the array itself should
> ever be modified.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  include/hw/qdev-core.h      | 2 +-
>  include/qapi/util.h         | 2 +-
>  include/qapi/visitor-impl.h | 6 +++---
>  include/qapi/visitor.h      | 2 +-
>  include/qom/object.h        | 2 +-
>  qapi/qapi-dealloc-visitor.c | 3 ++-
>  qapi/qapi-util.c            | 2 +-
>  qapi/qapi-visit-core.c      | 6 +++---
>  qom/object.c                | 2 +-
>  scripts/qapi-types.py       | 4 ++--
>  10 files changed, 16 insertions(+), 15 deletions(-)

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

[Qemu-devel] [PATCH v3 6/7] qom: add a object_property_add_enum helper method

[Daniel P. Berrange]

A QOM property can be parsed as enum using the visit_type_enum()
helper method, but this forces callers to use the more complex
generic object_property_add() method when registering it. It
also requires that users of that object have access to the
string map when they want to read the property value.

This patch introduces a specialized object_property_add_enum()
method which simplifies the use of enum properties, so the
setters/getters directly get passed the int value.

  typedef enum {
     MYDEV_TYPE_FROG,
     MYDEV_TYPE_ALLIGATOR,
     MYDEV_TYPE_PLATYPUS,

     MYDEV_TYPE_LAST
  } MyDevType;

Then provide a table of enum <-> string mappings

  static const char *const mydevtypemap[MYDEV_TYPE_LAST + 1] = {
     [MYDEV_TYPE_FROG] = "frog",
     [MYDEV_TYPE_ALLIGATOR] = "alligator",
     [MYDEV_TYPE_PLATYPUS] = "platypus",
     [MYDEV_TYPE_LAST] = NULL,
  };

Assuming an object struct of

   typedef struct {
      Object parent;
      MyDevType devtype;
      ...other fields...
   } MyDev;

The property can then be registered as follows:

   static int mydev_prop_get_devtype(Object *obj,
                                     Error **errp G_GNUC_UNUSED)
   {
       MyDev *dev = MYDEV(obj);

       return dev->devtype;
   }

   static void mydev_prop_set_devtype(Object *obj,
                                      int value,
                                      Error **errp G_GNUC_UNUSED)
   {
       MyDev *dev = MYDEV(obj);

       dev->endpoint = value;
   }

   object_property_add_enum(obj, "devtype",
                            mydevtypemap, "MyDevType",
                            mydev_prop_get_devtype,
                            mydev_prop_set_devtype,
                            NULL);

Note there is no need to check the range of 'value' in
the setter, because the string->enum conversion code will
have already done that and reported an error as required.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 include/qom/object.h       | 19 ++++++++++++
 qom/object.c               | 58 ++++++++++++++++++++++++++++++++++++
 tests/check-qom-proplist.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 151 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index bf76f7a..f6a2a9d 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1271,6 +1271,25 @@ void object_property_add_bool(Object *obj, const char *name,
                               Error **errp);
 
 /**
+ * object_property_add_enum:
+ * @obj: the object to add a property to
+ * @name: the name of the property
+ * @typename: the name of the enum data type
+ * @get: the getter or NULL if the property is write-only.
+ * @set: the setter or NULL if the property is read-only
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Add a enum property using getters/setters.  This function will add a
+ * property of type 'enum'.
+ */
+void object_property_add_enum(Object *obj, const char *name,
+                              const char *typename,
+                              const char * const *strings,
+                              int (*get)(Object *, Error **),
+                              void (*set)(Object *, int, Error **),
+                              Error **errp);
+
+/**
  * object_property_add_tm:
  * @obj: the object to add a property to
  * @name: the name of the property
diff --git a/qom/object.c b/qom/object.c
index 077a5fe..ba0e4b8 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1609,6 +1609,64 @@ void object_property_add_bool(Object *obj, const char *name,
     }
 }
 
+typedef struct EnumProperty {
+    const char * const *strings;
+    int (*get)(Object *, Error **);
+    void (*set)(Object *, int, Error **);
+} EnumProperty;
+
+static void property_get_enum(Object *obj, Visitor *v, void *opaque,
+                              const char *name, Error **errp)
+{
+    EnumProperty *prop = opaque;
+    int value;
+
+    value = prop->get(obj, errp);
+    visit_type_enum(v, &value, prop->strings, NULL, name, errp);
+}
+
+static void property_set_enum(Object *obj, Visitor *v, void *opaque,
+                              const char *name, Error **errp)
+{
+    EnumProperty *prop = opaque;
+    int value;
+
+    visit_type_enum(v, &value, prop->strings, NULL, name, errp);
+    prop->set(obj, value, errp);
+}
+
+static void property_release_enum(Object *obj, const char *name,
+                                  void *opaque)
+{
+    EnumProperty *prop = opaque;
+    g_free(prop);
+}
+
+void object_property_add_enum(Object *obj, const char *name,
+                              const char *typename,
+                              const char * const *strings,
+                              int (*get)(Object *, Error **),
+                              void (*set)(Object *, int, Error **),
+                              Error **errp)
+{
+    Error *local_err = NULL;
+    EnumProperty *prop = g_malloc0(sizeof(*prop));
+
+    prop->strings = strings;
+    prop->get = get;
+    prop->set = set;
+
+    object_property_add(obj, name, typename,
+                        get ? property_get_enum : NULL,
+                        set ? property_set_enum : NULL,
+                        property_release_enum,
+                        prop, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        g_free(prop);
+    }
+}
+
 typedef struct TMProperty {
     void (*get)(Object *, struct tm *, Error **);
 } TMProperty;
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 9f16cdb..de142e3 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -32,10 +32,28 @@ typedef struct DummyObjectClass DummyObjectClass;
 #define DUMMY_OBJECT(obj)                               \
     OBJECT_CHECK(DummyObject, (obj), TYPE_DUMMY)
 
+typedef enum DummyAnimal DummyAnimal;
+
+enum DummyAnimal {
+    DUMMY_FROG,
+    DUMMY_ALLIGATOR,
+    DUMMY_PLATYPUS,
+
+    DUMMY_LAST,
+};
+
+static const char *const dummyanimalmap[DUMMY_LAST + 1] = {
+    [DUMMY_FROG] = "frog",
+    [DUMMY_ALLIGATOR] = "alligator",
+    [DUMMY_PLATYPUS] = "platypus",
+    [DUMMY_LAST] = NULL,
+};
+
 struct DummyObject {
     Object parent;
 
     bool bv;
+    DummyAnimal av;
     char *sv;
 };
 
@@ -62,6 +80,24 @@ static bool dummy_get_bv(Object *obj,
 }
 
 
+static void dummy_set_av(Object *obj,
+                         int value,
+                         Error **errp)
+{
+    DummyObject *dobj = DUMMY_OBJECT(obj);
+
+    dobj->av = value;
+}
+
+static int dummy_get_av(Object *obj,
+                        Error **errp)
+{
+    DummyObject *dobj = DUMMY_OBJECT(obj);
+
+    return dobj->av;
+}
+
+
 static void dummy_set_sv(Object *obj,
                          const char *value,
                          Error **errp)
@@ -91,6 +127,12 @@ static void dummy_init(Object *obj)
                             dummy_get_sv,
                             dummy_set_sv,
                             NULL);
+    object_property_add_enum(obj, "av",
+                             "DummyAnimal",
+                             dummyanimalmap,
+                             dummy_get_av,
+                             dummy_set_av,
+                             NULL);
 }
 
 static void dummy_finalize(Object *obj)
@@ -122,12 +164,14 @@ static void test_dummy_createv(void)
                          &err,
                          "bv", "yes",
                          "sv", "Hiss hiss hiss",
+                         "av", "platypus",
                          NULL));
 
     g_assert(dobj != NULL);
     g_assert(err == NULL);
     g_assert(g_str_equal(dobj->sv, "Hiss hiss hiss"));
     g_assert(dobj->bv == true);
+    g_assert(dobj->av == DUMMY_PLATYPUS);
 
     g_assert(object_resolve_path_component(parent, "dummy0")
              == OBJECT(dobj));
@@ -163,12 +207,14 @@ static void test_dummy_createlist(void)
                    parent,
                    "bv", "yes",
                    "sv", "Hiss hiss hiss",
+                   "av", "platypus",
                    NULL));
 
     g_assert(dobj != NULL);
     g_assert(err == NULL);
     g_assert(g_str_equal(dobj->sv, "Hiss hiss hiss"));
     g_assert(dobj->bv == true);
+    g_assert(dobj->av == DUMMY_PLATYPUS);
 
     g_assert(object_resolve_path_component(parent, "dummy0")
              == OBJECT(dobj));
@@ -176,6 +222,33 @@ static void test_dummy_createlist(void)
     object_unparent(OBJECT(dobj));
 }
 
+static void test_dummy_badenum(void)
+{
+    Error *err = NULL;
+    Object *parent = container_get(object_get_root(),
+                                   "/objects");
+    DummyObject *dobj = DUMMY_OBJECT(
+        object_new_propv(TYPE_DUMMY,
+                         parent,
+                         "dummy0",
+                         &err,
+                         "bv", "yes",
+                         "sv", "Hiss hiss hiss",
+                         "av", "yeti",
+                         NULL));
+
+    g_assert(dobj == NULL);
+    g_assert(err != NULL);
+    g_assert(g_str_equal(error_get_pretty(err),
+                         "Invalid parameter 'yeti'"));
+
+    g_assert(object_resolve_path_component(parent, "dummy0")
+             == NULL);
+
+    error_free(err);
+}
+
+
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
@@ -185,6 +258,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/badenum", test_dummy_badenum);
 
     return g_test_run();
 }
-- 
2.1.0

Re: [Qemu-devel] [PATCH v3 6/7] qom: add a object_property_add_enum helper method

[Andreas Färber]

Am 01.05.2015 um 12:30 schrieb Daniel P. Berrange:
> A QOM property can be parsed as enum using the visit_type_enum()
> helper method, but this forces callers to use the more complex
> generic object_property_add() method when registering it. It
> also requires that users of that object have access to the
> string map when they want to read the property value.
> 
> This patch introduces a specialized object_property_add_enum()
> method which simplifies the use of enum properties, so the
> setters/getters directly get passed the int value.
> 
>   typedef enum {
>      MYDEV_TYPE_FROG,
>      MYDEV_TYPE_ALLIGATOR,
>      MYDEV_TYPE_PLATYPUS,
> 
>      MYDEV_TYPE_LAST
>   } MyDevType;
> 
> Then provide a table of enum <-> string mappings
> 
>   static const char *const mydevtypemap[MYDEV_TYPE_LAST + 1] = {
>      [MYDEV_TYPE_FROG] = "frog",
>      [MYDEV_TYPE_ALLIGATOR] = "alligator",
>      [MYDEV_TYPE_PLATYPUS] = "platypus",
>      [MYDEV_TYPE_LAST] = NULL,
>   };
> 
> Assuming an object struct of
> 
>    typedef struct {
>       Object parent;
>       MyDevType devtype;
>       ...other fields...
>    } MyDev;
> 
> The property can then be registered as follows:
> 
>    static int mydev_prop_get_devtype(Object *obj,
>                                      Error **errp G_GNUC_UNUSED)
>    {
>        MyDev *dev = MYDEV(obj);
> 
>        return dev->devtype;
>    }
> 
>    static void mydev_prop_set_devtype(Object *obj,
>                                       int value,
>                                       Error **errp G_GNUC_UNUSED)
>    {
>        MyDev *dev = MYDEV(obj);
> 
>        dev->endpoint = value;
>    }
> 
>    object_property_add_enum(obj, "devtype",
>                             mydevtypemap, "MyDevType",
>                             mydev_prop_get_devtype,
>                             mydev_prop_set_devtype,
>                             NULL);
> 
> Note there is no need to check the range of 'value' in
> the setter, because the string->enum conversion code will
> have already done that and reported an error as required.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  include/qom/object.h       | 19 ++++++++++++
>  qom/object.c               | 58 ++++++++++++++++++++++++++++++++++++
>  tests/check-qom-proplist.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 151 insertions(+)

Looks good in general. Some minor nits below, and one limitation
possibly worth mentioning in the second paragraph of the commit message:
It assumes a 1:1 mapping. I do guess most ones are, but I remember some
CPUID bits having different names for the same values, for instance.

> diff --git a/include/qom/object.h b/include/qom/object.h
> index bf76f7a..f6a2a9d 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1271,6 +1271,25 @@ void object_property_add_bool(Object *obj, const char *name,
>                                Error **errp);
>  
>  /**
> + * object_property_add_enum:
> + * @obj: the object to add a property to
> + * @name: the name of the property
> + * @typename: the name of the enum data type
> + * @get: the getter or NULL if the property is write-only.

%NULL

> + * @set: the setter or NULL if the property is read-only
> + * @errp: if an error occurs, a pointer to an area to store the error
> + *
> + * Add a enum property using getters/setters.  This function will add a
> + * property of type 'enum'.

This is slightly ambiguous, as I understand it the type we're actually
using is the one in @typename, not "enum"?

> + */
> +void object_property_add_enum(Object *obj, const char *name,
> +                              const char *typename,
> +                              const char * const *strings,
> +                              int (*get)(Object *, Error **),
> +                              void (*set)(Object *, int, Error **),
> +                              Error **errp);
> +
> +/**
>   * object_property_add_tm:
>   * @obj: the object to add a property to
>   * @name: the name of the property
> diff --git a/qom/object.c b/qom/object.c
> index 077a5fe..ba0e4b8 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1609,6 +1609,64 @@ void object_property_add_bool(Object *obj, const char *name,
>      }
>  }
>  
> +typedef struct EnumProperty {
> +    const char * const *strings;
> +    int (*get)(Object *, Error **);
> +    void (*set)(Object *, int, Error **);
> +} EnumProperty;
> +
> +static void property_get_enum(Object *obj, Visitor *v, void *opaque,
> +                              const char *name, Error **errp)
> +{
> +    EnumProperty *prop = opaque;
> +    int value;
> +
> +    value = prop->get(obj, errp);
> +    visit_type_enum(v, &value, prop->strings, NULL, name, errp);
> +}
> +
> +static void property_set_enum(Object *obj, Visitor *v, void *opaque,
> +                              const char *name, Error **errp)
> +{
> +    EnumProperty *prop = opaque;
> +    int value;
> +
> +    visit_type_enum(v, &value, prop->strings, NULL, name, errp);
> +    prop->set(obj, value, errp);
> +}
> +
> +static void property_release_enum(Object *obj, const char *name,
> +                                  void *opaque)
> +{
> +    EnumProperty *prop = opaque;
> +    g_free(prop);
> +}
> +
> +void object_property_add_enum(Object *obj, const char *name,
> +                              const char *typename,
> +                              const char * const *strings,
> +                              int (*get)(Object *, Error **),
> +                              void (*set)(Object *, int, Error **),
> +                              Error **errp)
> +{
> +    Error *local_err = NULL;
> +    EnumProperty *prop = g_malloc0(sizeof(*prop));
> +
> +    prop->strings = strings;
> +    prop->get = get;
> +    prop->set = set;

Since all three fields are set, we could also use g_malloc() as
micro-optimization.

> +
> +    object_property_add(obj, name, typename,
> +                        get ? property_get_enum : NULL,
> +                        set ? property_set_enum : NULL,
> +                        property_release_enum,
> +                        prop, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        g_free(prop);
> +    }
> +}
> +
>  typedef struct TMProperty {
>      void (*get)(Object *, struct tm *, Error **);
>  } TMProperty;
> diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
> index 9f16cdb..de142e3 100644
> --- a/tests/check-qom-proplist.c
> +++ b/tests/check-qom-proplist.c
> @@ -32,10 +32,28 @@ typedef struct DummyObjectClass DummyObjectClass;
>  #define DUMMY_OBJECT(obj)                               \
>      OBJECT_CHECK(DummyObject, (obj), TYPE_DUMMY)
>  
> +typedef enum DummyAnimal DummyAnimal;
> +
> +enum DummyAnimal {
> +    DUMMY_FROG,
> +    DUMMY_ALLIGATOR,
> +    DUMMY_PLATYPUS,
> +
> +    DUMMY_LAST,
> +};
> +
> +static const char *const dummyanimalmap[DUMMY_LAST + 1] = {

dummy_animal_map would be slightly easier to read.

> +    [DUMMY_FROG] = "frog",
> +    [DUMMY_ALLIGATOR] = "alligator",
> +    [DUMMY_PLATYPUS] = "platypus",
> +    [DUMMY_LAST] = NULL,
> +};
> +
>  struct DummyObject {
>      Object parent;
>  
>      bool bv;
> +    DummyAnimal av;
>      char *sv;
>  };
>  
> @@ -62,6 +80,24 @@ static bool dummy_get_bv(Object *obj,
>  }
>  
>  
> +static void dummy_set_av(Object *obj,
> +                         int value,
> +                         Error **errp)
> +{
> +    DummyObject *dobj = DUMMY_OBJECT(obj);
> +
> +    dobj->av = value;
> +}
> +
> +static int dummy_get_av(Object *obj,
> +                        Error **errp)
> +{
> +    DummyObject *dobj = DUMMY_OBJECT(obj);
> +
> +    return dobj->av;
> +}
> +
> +
>  static void dummy_set_sv(Object *obj,
>                           const char *value,
>                           Error **errp)
> @@ -91,6 +127,12 @@ static void dummy_init(Object *obj)
>                              dummy_get_sv,
>                              dummy_set_sv,
>                              NULL);
> +    object_property_add_enum(obj, "av",
> +                             "DummyAnimal",
> +                             dummyanimalmap,
> +                             dummy_get_av,
> +                             dummy_set_av,
> +                             NULL);
>  }
>  
>  static void dummy_finalize(Object *obj)
> @@ -122,12 +164,14 @@ static void test_dummy_createv(void)
>                           &err,
>                           "bv", "yes",
>                           "sv", "Hiss hiss hiss",
> +                         "av", "platypus",
>                           NULL));
>  
>      g_assert(dobj != NULL);
>      g_assert(err == NULL);
>      g_assert(g_str_equal(dobj->sv, "Hiss hiss hiss"));
>      g_assert(dobj->bv == true);
> +    g_assert(dobj->av == DUMMY_PLATYPUS);
>  
>      g_assert(object_resolve_path_component(parent, "dummy0")
>               == OBJECT(dobj));
> @@ -163,12 +207,14 @@ static void test_dummy_createlist(void)
>                     parent,
>                     "bv", "yes",
>                     "sv", "Hiss hiss hiss",
> +                   "av", "platypus",
>                     NULL));
>  
>      g_assert(dobj != NULL);
>      g_assert(err == NULL);
>      g_assert(g_str_equal(dobj->sv, "Hiss hiss hiss"));
>      g_assert(dobj->bv == true);
> +    g_assert(dobj->av == DUMMY_PLATYPUS);
>  
>      g_assert(object_resolve_path_component(parent, "dummy0")
>               == OBJECT(dobj));
> @@ -176,6 +222,33 @@ static void test_dummy_createlist(void)
>      object_unparent(OBJECT(dobj));
>  }
>  
> +static void test_dummy_badenum(void)
> +{
> +    Error *err = NULL;
> +    Object *parent = container_get(object_get_root(),
> +                                   "/objects");
> +    DummyObject *dobj = DUMMY_OBJECT(
> +        object_new_propv(TYPE_DUMMY,
> +                         parent,
> +                         "dummy0",
> +                         &err,
> +                         "bv", "yes",
> +                         "sv", "Hiss hiss hiss",
> +                         "av", "yeti",
> +                         NULL));
> +
> +    g_assert(dobj == NULL);

Superfluous.

> +    g_assert(err != NULL);
> +    g_assert(g_str_equal(error_get_pretty(err),
> +                         "Invalid parameter 'yeti'"));

Same question as in previous test: alternatives?

> +
> +    g_assert(object_resolve_path_component(parent, "dummy0")
> +             == NULL);
> +
> +    error_free(err);
> +}
> +
> +
>  int main(int argc, char **argv)
>  {
>      g_test_init(&argc, &argv, NULL);
> @@ -185,6 +258,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/badenum", test_dummy_badenum);
>  
>      return g_test_run();
>  }

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v3 6/7] qom: add a object_property_add_enum helper method

[Daniel P. Berrange]

On Fri, May 08, 2015 at 07:45:10PM +0200, Andreas Färber wrote:
> Am 01.05.2015 um 12:30 schrieb Daniel P. Berrange:
> 
> Looks good in general. Some minor nits below, and one limitation
> possibly worth mentioning in the second paragraph of the commit message:
> It assumes a 1:1 mapping. I do guess most ones are, but I remember some
> CPUID bits having different names for the same values, for instance.

Worst case, in that edge case, we can simply not use this stricter
enum property type - just carry on with existing custom property

> > diff --git a/include/qom/object.h b/include/qom/object.h
> > index bf76f7a..f6a2a9d 100644
> > --- a/include/qom/object.h
> > +++ b/include/qom/object.h
> > @@ -1271,6 +1271,25 @@ void object_property_add_bool(Object *obj, const char *name,
> >                                Error **errp);
> >  
> >  /**
> > + * object_property_add_enum:
> > + * @obj: the object to add a property to
> > + * @name: the name of the property
> > + * @typename: the name of the enum data type
> > + * @get: the getter or NULL if the property is write-only.
> 
> %NULL
> 
> > + * @set: the setter or NULL if the property is read-only
> > + * @errp: if an error occurs, a pointer to an area to store the error
> > + *
> > + * Add a enum property using getters/setters.  This function will add a
> > + * property of type 'enum'.
> 
> This is slightly ambiguous, as I understand it the type we're actually
> using is the one in @typename, not "enum"?

Yeah, you are right - this doc mistake is left over from a previous
version before Paolo asked me to add the @typename parameter.

> > diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
> > index 9f16cdb..de142e3 100644
> > --- a/tests/check-qom-proplist.c
> > +++ b/tests/check-qom-proplist.c
> > @@ -32,10 +32,28 @@ typedef struct DummyObjectClass DummyObjectClass;
> >  #define DUMMY_OBJECT(obj)                               \
> >      OBJECT_CHECK(DummyObject, (obj), TYPE_DUMMY)
> >  
> > +typedef enum DummyAnimal DummyAnimal;
> > +
> > +enum DummyAnimal {
> > +    DUMMY_FROG,
> > +    DUMMY_ALLIGATOR,
> > +    DUMMY_PLATYPUS,
> > +
> > +    DUMMY_LAST,
> > +};
> > +
> > +static const char *const dummyanimalmap[DUMMY_LAST + 1] = {
> 
> dummy_animal_map would be slightly easier to read.

Sure

> > +static void test_dummy_badenum(void)
> > +{
> > +    Error *err = NULL;
> > +    Object *parent = container_get(object_get_root(),
> > +                                   "/objects");
> > +    DummyObject *dobj = DUMMY_OBJECT(
> > +        object_new_propv(TYPE_DUMMY,
> > +                         parent,
> > +                         "dummy0",
> > +                         &err,
> > +                         "bv", "yes",
> > +                         "sv", "Hiss hiss hiss",
> > +                         "av", "yeti",
> > +                         NULL));
> > +
> > +    g_assert(dobj == NULL);
> 
> Superfluous.
> 
> > +    g_assert(err != NULL);
> > +    g_assert(g_str_equal(error_get_pretty(err),
> > +                         "Invalid parameter 'yeti'"));
> 
> Same question as in previous test: alternatives?

Yep, will check

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

[Qemu-devel] [PATCH v3 7/7] qom: don't pass string table to object_get_enum method

[Daniel P. Berrange]

Now that properties can be explicitly registered as an enum
type, there is no need to pass the string table to the
object_get_enum method. The object property registration
already has a pointer to the string table.

In changing this method signature, the hostmem backend object
has to be converted to use the new enum property registration
code, which simplifies it somewhat.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 backends/hostmem.c         | 22 ++++++++--------------
 include/qom/object.h       |  4 ++--
 numa.c                     |  2 +-
 qom/object.c               | 32 ++++++++++++++++++++++++--------
 tests/check-qom-proplist.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 81 insertions(+), 25 deletions(-)

diff --git a/backends/hostmem.c b/backends/hostmem.c
index f6db33c..7d74be0 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -113,24 +113,17 @@ host_memory_backend_set_host_nodes(Object *obj, Visitor *v, void *opaque,
 #endif
 }
 
-static void
-host_memory_backend_get_policy(Object *obj, Visitor *v, void *opaque,
-                               const char *name, Error **errp)
+static int
+host_memory_backend_get_policy(Object *obj, Error **errp G_GNUC_UNUSED)
 {
     HostMemoryBackend *backend = MEMORY_BACKEND(obj);
-    int policy = backend->policy;
-
-    visit_type_enum(v, &policy, HostMemPolicy_lookup, NULL, name, errp);
+    return backend->policy;
 }
 
 static void
-host_memory_backend_set_policy(Object *obj, Visitor *v, void *opaque,
-                               const char *name, Error **errp)
+host_memory_backend_set_policy(Object *obj, int policy, Error **errp)
 {
     HostMemoryBackend *backend = MEMORY_BACKEND(obj);
-    int policy;
-
-    visit_type_enum(v, &policy, HostMemPolicy_lookup, NULL, name, errp);
     backend->policy = policy;
 
 #ifndef CONFIG_NUMA
@@ -252,9 +245,10 @@ static void host_memory_backend_init(Object *obj)
     object_property_add(obj, "host-nodes", "int",
                         host_memory_backend_get_host_nodes,
                         host_memory_backend_set_host_nodes, NULL, NULL, NULL);
-    object_property_add(obj, "policy", "HostMemPolicy",
-                        host_memory_backend_get_policy,
-                        host_memory_backend_set_policy, NULL, NULL, NULL);
+    object_property_add_enum(obj, "policy", "HostMemPolicy",
+                             HostMemPolicy_lookup,
+                             host_memory_backend_get_policy,
+                             host_memory_backend_set_policy, NULL);
 }
 
 MemoryRegion *
diff --git a/include/qom/object.h b/include/qom/object.h
index f6a2a9d..fc347b9 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1012,7 +1012,7 @@ int64_t object_property_get_int(Object *obj, const char *name,
  * object_property_get_enum:
  * @obj: the object
  * @name: the name of the property
- * @strings: strings corresponding to enums
+ * @typename: the name of the enum data type
  * @errp: returns an error if this function fails
  *
  * Returns: the value of the property, converted to an integer, or
@@ -1020,7 +1020,7 @@ int64_t object_property_get_int(Object *obj, const char *name,
  * an enum).
  */
 int object_property_get_enum(Object *obj, const char *name,
-                             const char * const strings[], Error **errp);
+                             const char *typename, Error **errp);
 
 /**
  * object_property_get_uint16List:
diff --git a/numa.c b/numa.c
index c975fb2..a64279a 100644
--- a/numa.c
+++ b/numa.c
@@ -457,7 +457,7 @@ static int query_memdev(Object *obj, void *opaque)
 
         m->value->policy = object_property_get_enum(obj,
                                                     "policy",
-                                                    HostMemPolicy_lookup,
+                                                    "HostMemPolicy",
                                                     &err);
         if (err) {
             goto error;
diff --git a/qom/object.c b/qom/object.c
index ba0e4b8..6d2a2a9 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1026,13 +1026,35 @@ int64_t object_property_get_int(Object *obj, const char *name,
     return retval;
 }
 
+typedef struct EnumProperty {
+    const char * const *strings;
+    int (*get)(Object *, Error **);
+    void (*set)(Object *, int, Error **);
+} EnumProperty;
+
+
 int object_property_get_enum(Object *obj, const char *name,
-                             const char * const strings[], Error **errp)
+                             const char *typename, Error **errp)
 {
     StringOutputVisitor *sov;
     StringInputVisitor *siv;
     char *str;
     int ret;
+    ObjectProperty *prop = object_property_find(obj, name, errp);
+    EnumProperty *enumprop;
+
+    if (prop == NULL) {
+        return 0;
+    }
+
+    if (!g_str_equal(prop->type, typename)) {
+        error_setg(errp, "Property %s on %s is not '%s' enum type",
+                   name, object_class_get_name(
+                       object_get_class(obj)), typename);
+        return 0;
+    }
+
+    enumprop = prop->opaque;
 
     sov = string_output_visitor_new(false);
     object_property_get(obj, string_output_get_visitor(sov), name, errp);
@@ -1040,7 +1062,7 @@ int object_property_get_enum(Object *obj, const char *name,
     siv = string_input_visitor_new(str);
     string_output_visitor_cleanup(sov);
     visit_type_enum(string_input_get_visitor(siv),
-                    &ret, strings, NULL, name, errp);
+                    &ret, enumprop->strings, NULL, name, errp);
 
     g_free(str);
     string_input_visitor_cleanup(siv);
@@ -1609,12 +1631,6 @@ void object_property_add_bool(Object *obj, const char *name,
     }
 }
 
-typedef struct EnumProperty {
-    const char * const *strings;
-    int (*get)(Object *, Error **);
-    void (*set)(Object *, int, Error **);
-} EnumProperty;
-
 static void property_get_enum(Object *obj, Visitor *v, void *opaque,
                               const char *name, Error **errp)
 {
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index de142e3..d5cd38b 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -249,6 +249,51 @@ static void test_dummy_badenum(void)
 }
 
 
+
+static void test_dummy_getenum(void)
+{
+    Error *err = NULL;
+    int val;
+    Object *parent = container_get(object_get_root(),
+                                   "/objects");
+    DummyObject *dobj = DUMMY_OBJECT(
+        object_new_propv(TYPE_DUMMY,
+                         parent,
+                         "dummy0",
+                         &err,
+                         "av", "platypus",
+                         NULL));
+
+    g_assert(dobj != NULL);
+    g_assert(err == NULL);
+    g_assert(dobj->av == DUMMY_PLATYPUS);
+
+    val = object_property_get_enum(OBJECT(dobj),
+                                   "av",
+                                   "DummyAnimal",
+                                   &err);
+    g_assert(err == NULL);
+    g_assert(val == DUMMY_PLATYPUS);
+
+    /* A bad enum type name */
+    val = object_property_get_enum(OBJECT(dobj),
+                                   "av",
+                                   "BadAnimal",
+                                   &err);
+    g_assert(err != NULL);
+    error_free(err);
+    err = NULL;
+
+    /* A non-enum property name */
+    val = object_property_get_enum(OBJECT(dobj),
+                                   "iv",
+                                   "DummyAnimal",
+                                   &err);
+    g_assert(err != NULL);
+    error_free(err);
+}
+
+
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
@@ -259,6 +304,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/badenum", test_dummy_badenum);
+    g_test_add_func("/qom/proplist/getenum", test_dummy_getenum);
 
     return g_test_run();
 }
-- 
2.1.0

Re: [Qemu-devel] [PATCH v3 7/7] qom: don't pass string table to object_get_enum method

[Andreas Färber]

Am 01.05.2015 um 12:30 schrieb Daniel P. Berrange:
> Now that properties can be explicitly registered as an enum
> type, there is no need to pass the string table to the
> object_get_enum method. The object property registration
> already has a pointer to the string table.
> 
> In changing this method signature, the hostmem backend object
> has to be converted to use the new enum property registration
> code, which simplifies it somewhat.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  backends/hostmem.c         | 22 ++++++++--------------
>  include/qom/object.h       |  4 ++--
>  numa.c                     |  2 +-
>  qom/object.c               | 32 ++++++++++++++++++++++++--------
>  tests/check-qom-proplist.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 81 insertions(+), 25 deletions(-)
> 
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index f6db33c..7d74be0 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -113,24 +113,17 @@ host_memory_backend_set_host_nodes(Object *obj, Visitor *v, void *opaque,
>  #endif
>  }
>  
> -static void
> -host_memory_backend_get_policy(Object *obj, Visitor *v, void *opaque,
> -                               const char *name, Error **errp)
> +static int
> +host_memory_backend_get_policy(Object *obj, Error **errp G_GNUC_UNUSED)
>  {
>      HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> -    int policy = backend->policy;
> -
> -    visit_type_enum(v, &policy, HostMemPolicy_lookup, NULL, name, errp);
> +    return backend->policy;
>  }
>  
>  static void
> -host_memory_backend_set_policy(Object *obj, Visitor *v, void *opaque,
> -                               const char *name, Error **errp)
> +host_memory_backend_set_policy(Object *obj, int policy, Error **errp)
>  {
>      HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> -    int policy;
> -
> -    visit_type_enum(v, &policy, HostMemPolicy_lookup, NULL, name, errp);
>      backend->policy = policy;
>  
>  #ifndef CONFIG_NUMA
> @@ -252,9 +245,10 @@ static void host_memory_backend_init(Object *obj)
>      object_property_add(obj, "host-nodes", "int",
>                          host_memory_backend_get_host_nodes,
>                          host_memory_backend_set_host_nodes, NULL, NULL, NULL);
> -    object_property_add(obj, "policy", "HostMemPolicy",
> -                        host_memory_backend_get_policy,
> -                        host_memory_backend_set_policy, NULL, NULL, NULL);
> +    object_property_add_enum(obj, "policy", "HostMemPolicy",
> +                             HostMemPolicy_lookup,
> +                             host_memory_backend_get_policy,
> +                             host_memory_backend_set_policy, NULL);
>  }
>  
>  MemoryRegion *
> diff --git a/include/qom/object.h b/include/qom/object.h
> index f6a2a9d..fc347b9 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1012,7 +1012,7 @@ int64_t object_property_get_int(Object *obj, const char *name,
>   * object_property_get_enum:
>   * @obj: the object
>   * @name: the name of the property
> - * @strings: strings corresponding to enums
> + * @typename: the name of the enum data type
>   * @errp: returns an error if this function fails
>   *
>   * Returns: the value of the property, converted to an integer, or
> @@ -1020,7 +1020,7 @@ int64_t object_property_get_int(Object *obj, const char *name,
>   * an enum).
>   */
>  int object_property_get_enum(Object *obj, const char *name,
> -                             const char * const strings[], Error **errp);
> +                             const char *typename, Error **errp);
>  
>  /**
>   * object_property_get_uint16List:
> diff --git a/numa.c b/numa.c
> index c975fb2..a64279a 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -457,7 +457,7 @@ static int query_memdev(Object *obj, void *opaque)
>  
>          m->value->policy = object_property_get_enum(obj,
>                                                      "policy",
> -                                                    HostMemPolicy_lookup,
> +                                                    "HostMemPolicy",
>                                                      &err);
>          if (err) {
>              goto error;
> diff --git a/qom/object.c b/qom/object.c
> index ba0e4b8..6d2a2a9 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1026,13 +1026,35 @@ int64_t object_property_get_int(Object *obj, const char *name,
>      return retval;
>  }
>  
> +typedef struct EnumProperty {
> +    const char * const *strings;
> +    int (*get)(Object *, Error **);
> +    void (*set)(Object *, int, Error **);

Since get and set and moved unchanged, I would prefer placing it in the
final destination in the original patch to avoid churn.

> +} EnumProperty;
> +
> +
>  int object_property_get_enum(Object *obj, const char *name,
> -                             const char * const strings[], Error **errp)
> +                             const char *typename, Error **errp)
>  {
>      StringOutputVisitor *sov;
>      StringInputVisitor *siv;
>      char *str;
>      int ret;
> +    ObjectProperty *prop = object_property_find(obj, name, errp);
> +    EnumProperty *enumprop;
> +
> +    if (prop == NULL) {
> +        return 0;
> +    }
> +
> +    if (!g_str_equal(prop->type, typename)) {
> +        error_setg(errp, "Property %s on %s is not '%s' enum type",
> +                   name, object_class_get_name(
> +                       object_get_class(obj)), typename);
> +        return 0;
> +    }
> +
> +    enumprop = prop->opaque;
>  
>      sov = string_output_visitor_new(false);
>      object_property_get(obj, string_output_get_visitor(sov), name, errp);
> @@ -1040,7 +1062,7 @@ int object_property_get_enum(Object *obj, const char *name,
>      siv = string_input_visitor_new(str);
>      string_output_visitor_cleanup(sov);
>      visit_type_enum(string_input_get_visitor(siv),
> -                    &ret, strings, NULL, name, errp);
> +                    &ret, enumprop->strings, NULL, name, errp);
>  
>      g_free(str);
>      string_input_visitor_cleanup(siv);
> @@ -1609,12 +1631,6 @@ void object_property_add_bool(Object *obj, const char *name,
>      }
>  }
>  
> -typedef struct EnumProperty {
> -    const char * const *strings;
> -    int (*get)(Object *, Error **);
> -    void (*set)(Object *, int, Error **);
> -} EnumProperty;
> -
>  static void property_get_enum(Object *obj, Visitor *v, void *opaque,
>                                const char *name, Error **errp)
>  {
> diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
> index de142e3..d5cd38b 100644
> --- a/tests/check-qom-proplist.c
> +++ b/tests/check-qom-proplist.c
> @@ -249,6 +249,51 @@ static void test_dummy_badenum(void)
>  }
>  
>  
> +
> +static void test_dummy_getenum(void)
> +{
> +    Error *err = NULL;
> +    int val;
> +    Object *parent = container_get(object_get_root(),
> +                                   "/objects");
> +    DummyObject *dobj = DUMMY_OBJECT(
> +        object_new_propv(TYPE_DUMMY,
> +                         parent,
> +                         "dummy0",
> +                         &err,
> +                         "av", "platypus",
> +                         NULL));
> +
> +    g_assert(dobj != NULL);
> +    g_assert(err == NULL);
> +    g_assert(dobj->av == DUMMY_PLATYPUS);
> +
> +    val = object_property_get_enum(OBJECT(dobj),
> +                                   "av",
> +                                   "DummyAnimal",
> +                                   &err);
> +    g_assert(err == NULL);

Is there any significant difference between g_assert()'ing on error and
passing &error_abort?

> +    g_assert(val == DUMMY_PLATYPUS);
> +
> +    /* A bad enum type name */
> +    val = object_property_get_enum(OBJECT(dobj),
> +                                   "av",
> +                                   "BadAnimal",
> +                                   &err);
> +    g_assert(err != NULL);
> +    error_free(err);
> +    err = NULL;
> +
> +    /* A non-enum property name */
> +    val = object_property_get_enum(OBJECT(dobj),
> +                                   "iv",
> +                                   "DummyAnimal",
> +                                   &err);
> +    g_assert(err != NULL);
> +    error_free(err);
> +}
> +
> +
>  int main(int argc, char **argv)
>  {
>      g_test_init(&argc, &argv, NULL);
> @@ -259,6 +304,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/badenum", test_dummy_badenum);
> +    g_test_add_func("/qom/proplist/getenum", test_dummy_getenum);
>  
>      return g_test_run();
>  }

Otherwise looks good (apart from the dependencies).

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v3 7/7] qom: don't pass string table to object_get_enum method

[Daniel P. Berrange]

On Fri, May 08, 2015 at 07:54:48PM +0200, Andreas Färber wrote:
> Am 01.05.2015 um 12:30 schrieb Daniel P. Berrange:
> > Now that properties can be explicitly registered as an enum
> > type, there is no need to pass the string table to the
> > object_get_enum method. The object property registration
> > already has a pointer to the string table.
> > 
> > In changing this method signature, the hostmem backend object
> > has to be converted to use the new enum property registration
> > code, which simplifies it somewhat.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  backends/hostmem.c         | 22 ++++++++--------------
> >  include/qom/object.h       |  4 ++--
> >  numa.c                     |  2 +-
> >  qom/object.c               | 32 ++++++++++++++++++++++++--------
> >  tests/check-qom-proplist.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 81 insertions(+), 25 deletions(-)
> > diff --git a/qom/object.c b/qom/object.c
> > index ba0e4b8..6d2a2a9 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -1026,13 +1026,35 @@ int64_t object_property_get_int(Object *obj, const char *name,
> >      return retval;
> >  }
> >  
> > +typedef struct EnumProperty {
> > +    const char * const *strings;
> > +    int (*get)(Object *, Error **);
> > +    void (*set)(Object *, int, Error **);
> 
> Since get and set and moved unchanged, I would prefer placing it in the
> final destination in the original patch to avoid churn.

Yep, easy to do.

> > diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
> > index de142e3..d5cd38b 100644
> > --- a/tests/check-qom-proplist.c
> > +++ b/tests/check-qom-proplist.c
> > @@ -249,6 +249,51 @@ static void test_dummy_badenum(void)
> >  }
> >  
> >  
> > +
> > +static void test_dummy_getenum(void)
> > +{
> > +    Error *err = NULL;
> > +    int val;
> > +    Object *parent = container_get(object_get_root(),
> > +                                   "/objects");
> > +    DummyObject *dobj = DUMMY_OBJECT(
> > +        object_new_propv(TYPE_DUMMY,
> > +                         parent,
> > +                         "dummy0",
> > +                         &err,
> > +                         "av", "platypus",
> > +                         NULL));
> > +
> > +    g_assert(dobj != NULL);
> > +    g_assert(err == NULL);
> > +    g_assert(dobj->av == DUMMY_PLATYPUS);
> > +
> > +    val = object_property_get_enum(OBJECT(dobj),
> > +                                   "av",
> > +                                   "DummyAnimal",
> > +                                   &err);
> > +    g_assert(err == NULL);
> 
> Is there any significant difference between g_assert()'ing on error and
> passing &error_abort?

I didn't know about &error_abort until now :-) I will use that.

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

Re: [Qemu-devel] [PATCH v3 0/7] qom: misc fixes & enhancements to support TLS work

[Paolo Bonzini]

On 01/05/2015 12:29, Daniel P. Berrange wrote:
> This series contains the 7 generic QOM API fixes and enhancements
> that I previously posted as part of the large series refactoring
> and extending the TLS support in QEMU:
> 
>   https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02038.html
> 
> I'm sending it separately, since the patches are reasonably well
> self-contained and thus hopefully suitable for quicker review and
> merge.
> 
> Changed in v3:
> 
>  - Fix test suite additions for change in object_new_propv API
> 
> Changed in v2:
> 
>  - Pass "Object * parent" instead of "char *path" paremeter
>  - Rely on stable reference from parent to keep new object alive
>  - Use object_unparent() where appropriate
> 
> Daniel P. Berrange (7):
>   qom: fix typename of 'policy' enum property in hostmem obj
>   qom: document user creatable object types in help text
>   qom: create objects in two phases
>   qom: add object_new_propv / object_new_proplist constructors
>   qom: make enum string tables const-correct
>   qom: add a object_property_add_enum helper method
>   qom: don't pass string table to object_get_enum method
> 
>  backends/hostmem.c          |  22 ++--
>  include/hw/qdev-core.h      |   2 +-
>  include/qapi/util.h         |   2 +-
>  include/qapi/visitor-impl.h |   6 +-
>  include/qapi/visitor.h      |   2 +-
>  include/qom/object.h        |  90 ++++++++++++-
>  numa.c                      |   2 +-
>  qapi/qapi-dealloc-visitor.c |   3 +-
>  qapi/qapi-util.c            |   2 +-
>  qapi/qapi-visit-core.c      |   6 +-
>  qemu-options.hx             |  70 +++++++---
>  qom/object.c                | 144 +++++++++++++++++++-
>  scripts/qapi-types.py       |   4 +-
>  tests/.gitignore            |   1 +
>  tests/Makefile              |   5 +-
>  tests/check-qom-proplist.c  | 310 ++++++++++++++++++++++++++++++++++++++++++++
>  vl.c                        |  40 +++++-
>  17 files changed, 661 insertions(+), 50 deletions(-)
>  create mode 100644 tests/check-qom-proplist.c
> 

Thanks, queued for 2.4.

Paolo

Re: [Qemu-devel] [PATCH v3 0/7] qom: misc fixes & enhancements to support TLS work

[Andreas Färber]

Am 05.05.2015 um 12:33 schrieb Paolo Bonzini:
> 
> 
> On 01/05/2015 12:29, Daniel P. Berrange wrote:
>> This series contains the 7 generic QOM API fixes and enhancements
>> that I previously posted as part of the large series refactoring
>> and extending the TLS support in QEMU:
>>
>>   https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02038.html
>>
>> I'm sending it separately, since the patches are reasonably well
>> self-contained and thus hopefully suitable for quicker review and
>> merge.
>>
>> Changed in v3:
>>
>>  - Fix test suite additions for change in object_new_propv API
>>
>> Changed in v2:
>>
>>  - Pass "Object * parent" instead of "char *path" paremeter
>>  - Rely on stable reference from parent to keep new object alive
>>  - Use object_unparent() where appropriate
>>
>> Daniel P. Berrange (7):
>>   qom: fix typename of 'policy' enum property in hostmem obj
>>   qom: document user creatable object types in help text
>>   qom: create objects in two phases
>>   qom: add object_new_propv / object_new_proplist constructors
>>   qom: make enum string tables const-correct
>>   qom: add a object_property_add_enum helper method
>>   qom: don't pass string table to object_get_enum method
>>
>>  backends/hostmem.c          |  22 ++--
>>  include/hw/qdev-core.h      |   2 +-
>>  include/qapi/util.h         |   2 +-
>>  include/qapi/visitor-impl.h |   6 +-
>>  include/qapi/visitor.h      |   2 +-
>>  include/qom/object.h        |  90 ++++++++++++-
>>  numa.c                      |   2 +-
>>  qapi/qapi-dealloc-visitor.c |   3 +-
>>  qapi/qapi-util.c            |   2 +-
>>  qapi/qapi-visit-core.c      |   6 +-
>>  qemu-options.hx             |  70 +++++++---
>>  qom/object.c                | 144 +++++++++++++++++++-
>>  scripts/qapi-types.py       |   4 +-
>>  tests/.gitignore            |   1 +
>>  tests/Makefile              |   5 +-
>>  tests/check-qom-proplist.c  | 310 ++++++++++++++++++++++++++++++++++++++++++++
>>  vl.c                        |  40 +++++-
>>  17 files changed, 661 insertions(+), 50 deletions(-)
>>  create mode 100644 tests/check-qom-proplist.c
>>
> 
> Thanks, queued for 2.4.

Objection. None of the QOM patches have been reviewed by me and you knew
I would be back today.

Andreas

> 
> Paolo
> 


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v3 0/7] qom: misc fixes & enhancements to support TLS work

[Paolo Bonzini]

On 05/05/2015 17:37, Andreas Färber wrote:
>> > Thanks, queued for 2.4.
> Objection. None of the QOM patches have been reviewed by me and you knew
> I would be back today.

No, I didn't.  I didn't even know you were away, I thought you were
simply busy with something else.

Paolo

Re: [Qemu-devel] [PATCH v3 0/7] qom: misc fixes & enhancements to support TLS work

[Andreas Färber]

Am 08.05.2015 um 14:31 schrieb Paolo Bonzini:
> On 05/05/2015 17:37, Andreas Färber wrote:
>>>> Thanks, queued for 2.4.
>> Objection. None of the QOM patches have been reviewed by me and you knew
>> I would be back today.
> 
> No, I didn't.  I didn't even know you were away, I thought you were
> simply busy with something else.

On the QOM CPU link<> thread that you participated in.

Btw I see no reason to add artificial link<> properties for ".." at all,
we should simply implement that as part of path resolution rather than
wasting memory with pointers to self.

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH v3 0/7] qom: misc fixes & enhancements to support TLS work

[Paolo Bonzini]

On 08/05/2015 14:34, Andreas Färber wrote:
> > > Objection. None of the QOM patches have been reviewed by me and you knew
> > > I would be back today.
> > 
> > No, I didn't.  I didn't even know you were away, I thought you were
> > simply busy with something else.
> 
> On the QOM CPU link<> thread that you participated in.

Sorry I missed that.

> Btw I see no reason to add artificial link<> properties for ".." at all,
> we should simply implement that as part of path resolution rather than
> wasting memory with pointers to self.

Well, "ls -al" shows "." and "..".  But I'll implement
object_property_add_const_link without using ".", so that discussion can
be bypassed.

Paolo