[Qemu-devel] [PATCH v5] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties

[Qemu-devel] [PATCH v5] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties

[Valentin Rakush]

This patch adds support for qom-type-prop-list command to list object
class properties. A later patch will use this functionality to
implement x86_64-cpu properties.

Signed-off-by: Valentin Rakush <valentin.rakush@gmail.com>
Cc: Luiz Capitulino <lcapitulino@redhat.com>
Cc: Eric Blake <eblake@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Andreas Färber <afaerber@suse.de>
Cc: Daniel P. Berrange <berrange@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
---
V5: fixes after scripts/checkpatch.pl
V4: review fixes
 - the typename argument in the hmp command changed to be mandatory
V3: commit message fix
 - commit message changed to reflect actual command name
V2: Fixes after first review
 - changed command name from qom-type-list to qom-type-prop-list
 - changed memory allocation from g_malloc0 to g_new0
 - changed parameter name from path to typename
 - fixed wordings and comments
 - fixed source code formatting
 - registered the command in monitor

 hmp-commands.hx      | 13 +++++++++++++
 hmp.c                | 21 +++++++++++++++++++++
 hmp.h                |  1 +
 include/qom/object.h | 31 +++++++++++++++++++++++++++++++
 qapi-schema.json     | 19 +++++++++++++++++++
 qmp-commands.hx      |  6 ++++++
 qmp.c                | 31 +++++++++++++++++++++++++++++++
 qom/object.c         |  7 +++++++
 8 files changed, 129 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index bb52e4d..ee4d1e2 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1734,6 +1734,19 @@ Print QOM properties of object at location @var{path}
 ETEXI
 
     {
+        .name       = "qom-type-prop-list",
+        .args_type  = "typename:s",
+        .params     = "typename",
+        .help       = "list QOM class properties",
+        .mhandler.cmd  = hmp_qom_type_prop_list,
+    },
+
+STEXI
+@item qom-type-prop-list [@var{typename}]
+Print QOM properties of the type @var{typename}
+ETEXI
+
+    {
         .name       = "qom-set",
         .args_type  = "path:s,property:s,value:s",
         .params     = "path property value",
diff --git a/hmp.c b/hmp.c
index 54f2620..6de9bf0 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2052,6 +2052,27 @@ void hmp_qom_list(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &err);
 }
 
+void hmp_qom_type_prop_list(Monitor *mon, const QDict *qdict)
+{
+    const char *typename = qdict_get_try_str(qdict, "typename");
+    ObjectPropertyInfoList *list;
+    Error *err = NULL;
+
+    list = qmp_qom_type_prop_list(typename, &err);
+    if (!err) {
+        ObjectPropertyInfoList *start = list;
+        while (list) {
+            ObjectPropertyInfo *value = list->value;
+
+            monitor_printf(mon, "%s (%s)\n",
+                           value->name, value->type);
+            list = list->next;
+        }
+        qapi_free_ObjectPropertyInfoList(start);
+    }
+    hmp_handle_error(mon, &err);
+}
+
 void hmp_qom_set(Monitor *mon, const QDict *qdict)
 {
     const char *path = qdict_get_str(qdict, "path");
diff --git a/hmp.h b/hmp.h
index a8c5b5a..8c12ebe 100644
--- a/hmp.h
+++ b/hmp.h
@@ -103,6 +103,7 @@ void hmp_object_del(Monitor *mon, const QDict *qdict);
 void hmp_info_memdev(Monitor *mon, const QDict *qdict);
 void hmp_info_memory_devices(Monitor *mon, const QDict *qdict);
 void hmp_qom_list(Monitor *mon, const QDict *qdict);
+void hmp_qom_type_prop_list(Monitor *mon, const QDict *qdict);
 void hmp_qom_set(Monitor *mon, const QDict *qdict);
 void object_add_completion(ReadLineState *rs, int nb_args, const char *str);
 void object_del_completion(ReadLineState *rs, int nb_args, const char *str);
diff --git a/include/qom/object.h b/include/qom/object.h
index d0dafe9..0c8379d 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1013,6 +1013,37 @@ void object_property_iter_init(ObjectPropertyIterator *iter,
  */
 ObjectProperty *object_property_iter_next(ObjectPropertyIterator *iter);
 
+/**
+ * object_class_property_iter_init:
+ * @klass: the class owning the properties to be iterated over
+ *
+ * Initializes an iterator for traversing all properties
+ * registered against a class type and all parent classes.
+ *
+ * It is forbidden to modify the property list while iterating,
+ * whether removing or adding properties.
+ *
+ * NB For getting next property in the list the object related
+ * function object_property_iter_next is still used.
+ *
+ * Typical usage pattern would be
+ *
+ * <example>
+ *   <title>Using object class property iterators</title>
+ *   <programlisting>
+ *   ObjectProperty *prop;
+ *   ObjectPropertyIterator iter;
+ *
+ *   object_class property_iter_init(&iter, obj);
+ *   while ((prop = object_property_iter_next(&iter))) {
+ *     ... do something with prop ...
+ *   }
+ *   </programlisting>
+ * </example>
+ */
+void object_class_property_iter_init(ObjectPropertyIterator *iter,
+                                     ObjectClass *klass);
+
 void object_unparent(Object *obj);
 
 /**
diff --git a/qapi-schema.json b/qapi-schema.json
index b3038b2..2e960db 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4081,3 +4081,22 @@
 ##
 { 'enum': 'ReplayMode',
   'data': [ 'none', 'record', 'play' ] }
+
+##
+# @qom-type-prop-list:
+#
+# This command will list any properties of an object class
+# given its typename.
+#
+# @typename: the typename of the class. See @qom-list-types to check
+#            available typenames.
+#
+# Returns: a list of @ObjectPropertyInfo that describe the properties
+#          of the class.
+#
+# Since: 2.6
+##
+{ 'command': 'qom-type-prop-list',
+  'data': { 'typename': 'str' },
+  'returns': [ 'ObjectPropertyInfo' ] }
+
diff --git a/qmp-commands.hx b/qmp-commands.hx
index db072a6..d3be962 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3789,6 +3789,12 @@ EQMP
     },
 
     {
+        .name       = "qom-type-prop-list",
+        .args_type  = "typename:s",
+        .mhandler.cmd_new = qmp_marshal_qom_type_prop_list,
+    },
+
+    {
         .name       = "qom-set",
 	.args_type  = "path:s,property:s,value:q",
         .mhandler.cmd_new = qmp_marshal_qom_set,
diff --git a/qmp.c b/qmp.c
index 53affe2..baf25c0 100644
--- a/qmp.c
+++ b/qmp.c
@@ -460,6 +460,37 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements,
     return ret;
 }
 
+ObjectPropertyInfoList *qmp_qom_type_prop_list(const char *typename, Error **errp)
+{
+    ObjectClass *klass;
+    ObjectPropertyInfoList *props = NULL;
+    ObjectProperty *prop;
+    ObjectPropertyIterator iter;
+
+    klass = object_class_by_name(typename);
+    if (!klass) {
+        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
+                  "Object class '%s' not found", typename);
+        return NULL;
+    }
+
+    object_class_property_iter_init(&iter, klass);
+    while ((prop = object_property_iter_next(&iter))) {
+        ObjectPropertyInfoList *entry = g_new0(ObjectPropertyInfoList, 1);
+
+        if (entry) {
+            entry->value = g_new0(ObjectPropertyInfo, 1);
+            entry->next = props;
+            props = entry;
+
+            entry->value->name = g_strdup(prop->name);
+            entry->value->type = g_strdup(prop->type);
+        }
+    }
+
+    return props;
+}
+
 /* Return a DevicePropertyInfo for a qdev property.
  *
  * If a qdev property with the given name does not exist, use the given default
diff --git a/qom/object.c b/qom/object.c
index 5ff97ab..cabd91d 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -994,6 +994,13 @@ ObjectProperty *object_property_find(Object *obj, const char *name,
     return NULL;
 }
 
+void object_class_property_iter_init(ObjectPropertyIterator *iter,
+                                     ObjectClass *klass)
+{
+    g_hash_table_iter_init(&iter->iter, klass->properties);
+    iter->nextclass = object_class_get_parent(klass);
+}
+
 void object_property_iter_init(ObjectPropertyIterator *iter,
                                Object *obj)
 {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v5] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties

[Eduardo Habkost]

On Mon, Jan 25, 2016 at 11:24:47AM +0300, Valentin Rakush wrote:
> This patch adds support for qom-type-prop-list command to list object
> class properties. A later patch will use this functionality to
> implement x86_64-cpu properties.
> 
> Signed-off-by: Valentin Rakush <valentin.rakush@gmail.com>
> Cc: Luiz Capitulino <lcapitulino@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Andreas Färber <afaerber@suse.de>
> Cc: Daniel P. Berrange <berrange@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> ---
[...]
> diff --git a/qmp.c b/qmp.c
> index 53affe2..baf25c0 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -460,6 +460,37 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements,
>      return ret;
>  }
>  
> +ObjectPropertyInfoList *qmp_qom_type_prop_list(const char *typename, Error **errp)
> +{
> +    ObjectClass *klass;
> +    ObjectPropertyInfoList *props = NULL;
> +    ObjectProperty *prop;
> +    ObjectPropertyIterator iter;
> +
> +    klass = object_class_by_name(typename);
> +    if (!klass) {
> +        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> +                  "Object class '%s' not found", typename);
> +        return NULL;
> +    }
> +
> +    object_class_property_iter_init(&iter, klass);
> +    while ((prop = object_property_iter_next(&iter))) {
> +        ObjectPropertyInfoList *entry = g_new0(ObjectPropertyInfoList, 1);
> +
> +        if (entry) {
> +            entry->value = g_new0(ObjectPropertyInfo, 1);
> +            entry->next = props;
> +            props = entry;
> +
> +            entry->value->name = g_strdup(prop->name);
> +            entry->value->type = g_strdup(prop->type);
> +        }
> +    }
> +
> +    return props;
> +}
> +

We already have "-device <type>,help", and it uses a completely
different mechanism for listing properties. There's no reason for
having two arbitrarily different APIs for listing properties
returning different results.

If qmp_device_list_properties() is not enough for you, please
clarify why, so we can consider improving it.


>  /* Return a DevicePropertyInfo for a qdev property.
>   *
>   * If a qdev property with the given name does not exist, use the given default
> diff --git a/qom/object.c b/qom/object.c
> index 5ff97ab..cabd91d 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -994,6 +994,13 @@ ObjectProperty *object_property_find(Object *obj, const char *name,
>      return NULL;
>  }
>  
> +void object_class_property_iter_init(ObjectPropertyIterator *iter,
> +                                     ObjectClass *klass)
> +{
> +    g_hash_table_iter_init(&iter->iter, klass->properties);
> +    iter->nextclass = object_class_get_parent(klass);
> +}
> +
>  void object_property_iter_init(ObjectPropertyIterator *iter,
>                                 Object *obj)
>  {
> -- 
> 1.8.3.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v5] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties

[Eric Blake]

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

On 01/26/2016 08:35 AM, Eduardo Habkost wrote:
> On Mon, Jan 25, 2016 at 11:24:47AM +0300, Valentin Rakush wrote:
>> This patch adds support for qom-type-prop-list command to list object
>> class properties. A later patch will use this functionality to
>> implement x86_64-cpu properties.
>>

> 
> We already have "-device <type>,help", and it uses a completely
> different mechanism for listing properties. There's no reason for
> having two arbitrarily different APIs for listing properties
> returning different results.

And the QMP counterpart of '-device <type>,help' is
'device-list-properties'.

> 
> If qmp_device_list_properties() is not enough for you, please
> clarify why, so we can consider improving 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 v5] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties

[Daniel P. Berrange]

On Tue, Jan 26, 2016 at 01:35:38PM -0200, Eduardo Habkost wrote:
> On Mon, Jan 25, 2016 at 11:24:47AM +0300, Valentin Rakush wrote:
> > This patch adds support for qom-type-prop-list command to list object
> > class properties. A later patch will use this functionality to
> > implement x86_64-cpu properties.
> > 
> > Signed-off-by: Valentin Rakush <valentin.rakush@gmail.com>
> > Cc: Luiz Capitulino <lcapitulino@redhat.com>
> > Cc: Eric Blake <eblake@redhat.com>
> > Cc: Markus Armbruster <armbru@redhat.com>
> > Cc: Andreas Färber <afaerber@suse.de>
> > Cc: Daniel P. Berrange <berrange@redhat.com>
> > Cc: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> [...]
> > diff --git a/qmp.c b/qmp.c
> > index 53affe2..baf25c0 100644
> > --- a/qmp.c
> > +++ b/qmp.c
> > @@ -460,6 +460,37 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements,
> >      return ret;
> >  }
> >  
> > +ObjectPropertyInfoList *qmp_qom_type_prop_list(const char *typename, Error **errp)
> > +{
> > +    ObjectClass *klass;
> > +    ObjectPropertyInfoList *props = NULL;
> > +    ObjectProperty *prop;
> > +    ObjectPropertyIterator iter;
> > +
> > +    klass = object_class_by_name(typename);
> > +    if (!klass) {
> > +        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> > +                  "Object class '%s' not found", typename);
> > +        return NULL;
> > +    }
> > +
> > +    object_class_property_iter_init(&iter, klass);
> > +    while ((prop = object_property_iter_next(&iter))) {
> > +        ObjectPropertyInfoList *entry = g_new0(ObjectPropertyInfoList, 1);
> > +
> > +        if (entry) {
> > +            entry->value = g_new0(ObjectPropertyInfo, 1);
> > +            entry->next = props;
> > +            props = entry;
> > +
> > +            entry->value->name = g_strdup(prop->name);
> > +            entry->value->type = g_strdup(prop->type);
> > +        }
> > +    }
> > +
> > +    return props;
> > +}
> > +
> 
> We already have "-device <type>,help", and it uses a completely
> different mechanism for listing properties. There's no reason for
> having two arbitrarily different APIs for listing properties
> returning different results.
> 
> If qmp_device_list_properties() is not enough for you, please
> clarify why, so we can consider improving it.

qmp_device_list_properties() has to actually instantiate an instance
of objects it is reporting properties against, since it is reporting
properties registered against object instances. In fact it only
reports properties against things which are TYPE_DEVICE - it'll refuse
to report other object types. Having to instantiate objects is inherantly
limiting to the command because there are some objects that cannot be
instantiated for this purpose. eg abstract objects and objects marked
"cannot_destroy_with_object_finalize_yet". Finally there is also a
performance and memory overhead in having to instantiate objects which
is best avoided.

This new API is reporting properties that are statically registered
against the *class* rather than than object instance. It is guaranteed
that you can always report these properties for any class without any
restrictions, nor any chance of side effects during instantiation.

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 v5] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties

[Eduardo Habkost]

On Tue, Jan 26, 2016 at 03:51:21PM +0000, Daniel P. Berrange wrote:
> On Tue, Jan 26, 2016 at 01:35:38PM -0200, Eduardo Habkost wrote:
> > On Mon, Jan 25, 2016 at 11:24:47AM +0300, Valentin Rakush wrote:
> > > This patch adds support for qom-type-prop-list command to list object
> > > class properties. A later patch will use this functionality to
> > > implement x86_64-cpu properties.
> > > 
> > > Signed-off-by: Valentin Rakush <valentin.rakush@gmail.com>
> > > Cc: Luiz Capitulino <lcapitulino@redhat.com>
> > > Cc: Eric Blake <eblake@redhat.com>
> > > Cc: Markus Armbruster <armbru@redhat.com>
> > > Cc: Andreas Färber <afaerber@suse.de>
> > > Cc: Daniel P. Berrange <berrange@redhat.com>
> > > Cc: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > [...]
> > > diff --git a/qmp.c b/qmp.c
> > > index 53affe2..baf25c0 100644
> > > --- a/qmp.c
> > > +++ b/qmp.c
> > > @@ -460,6 +460,37 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements,
> > >      return ret;
> > >  }
> > >  
> > > +ObjectPropertyInfoList *qmp_qom_type_prop_list(const char *typename, Error **errp)
> > > +{
> > > +    ObjectClass *klass;
> > > +    ObjectPropertyInfoList *props = NULL;
> > > +    ObjectProperty *prop;
> > > +    ObjectPropertyIterator iter;
> > > +
> > > +    klass = object_class_by_name(typename);
> > > +    if (!klass) {
> > > +        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> > > +                  "Object class '%s' not found", typename);
> > > +        return NULL;
> > > +    }
> > > +
> > > +    object_class_property_iter_init(&iter, klass);
> > > +    while ((prop = object_property_iter_next(&iter))) {
> > > +        ObjectPropertyInfoList *entry = g_new0(ObjectPropertyInfoList, 1);
> > > +
> > > +        if (entry) {
> > > +            entry->value = g_new0(ObjectPropertyInfo, 1);
> > > +            entry->next = props;
> > > +            props = entry;
> > > +
> > > +            entry->value->name = g_strdup(prop->name);
> > > +            entry->value->type = g_strdup(prop->type);
> > > +        }
> > > +    }
> > > +
> > > +    return props;
> > > +}
> > > +
> > 
> > We already have "-device <type>,help", and it uses a completely
> > different mechanism for listing properties. There's no reason for
> > having two arbitrarily different APIs for listing properties
> > returning different results.
> > 
> > If qmp_device_list_properties() is not enough for you, please
> > clarify why, so we can consider improving it.
> 
> qmp_device_list_properties() has to actually instantiate an instance
> of objects it is reporting properties against, since it is reporting
> properties registered against object instances. In fact it only
> reports properties against things which are TYPE_DEVICE - it'll refuse
> to report other object types. Having to instantiate objects is inherantly
> limiting to the command because there are some objects that cannot be
> instantiated for this purpose. eg abstract objects and objects marked
> "cannot_destroy_with_object_finalize_yet". Finally there is also a
> performance and memory overhead in having to instantiate objects which
> is best avoided.
> 
> This new API is reporting properties that are statically registered
> against the *class* rather than than object instance. It is guaranteed
> that you can always report these properties for any class without any
> restrictions, nor any chance of side effects during instantiation.

The existing implementation has its limitations, but we can
address those limitations without exporting a new API that return
arbitrarily different results (that aren't even a superset of the
existing API).

About the existing qmp_device_list_properties() limitations:

cannot_destroy_with_object_finalize_yet is supposed to eventually
go away. If there are use cases that depend on listing properties
for cannot_destroy_with_object_finalize_yet classes, we can fix
that.

The TYPE_DEVICE requirement can be removed, as long as the
non-device QOM classes are object_new()-safe like the existing
cannot_destroy_with_object_finalize_yet=false device classes
(they are supposed to be).

About having to instantiate objects: if optimizing that is so
important, we can gradually convert the existing classes to use
class-properties. While we convert them, we can even have a
doesnt_need_to_instantiate_object_to_query_properties flag to
indicate classes that were already converted. No need to export a
new API.

Abstract classes are harder, but if they are important we can
make them a special case inside the existing implementation
instead of having two APIs.

                             * * *

So, now we have enumerated the current API limitations. Can we
enumerate the real world use cases that are affected by them, so
we know which ones we need to address first?

-- 
Eduardo

Re: [Qemu-devel] [PATCH v5] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties

[Daniel P. Berrange]

On Tue, Jan 26, 2016 at 03:26:35PM -0200, Eduardo Habkost wrote:
> On Tue, Jan 26, 2016 at 03:51:21PM +0000, Daniel P. Berrange wrote:
> > On Tue, Jan 26, 2016 at 01:35:38PM -0200, Eduardo Habkost wrote:
> > > On Mon, Jan 25, 2016 at 11:24:47AM +0300, Valentin Rakush wrote:
> > > > This patch adds support for qom-type-prop-list command to list object
> > > > class properties. A later patch will use this functionality to
> > > > implement x86_64-cpu properties.
> > > > 
> > > > Signed-off-by: Valentin Rakush <valentin.rakush@gmail.com>
> > > > Cc: Luiz Capitulino <lcapitulino@redhat.com>
> > > > Cc: Eric Blake <eblake@redhat.com>
> > > > Cc: Markus Armbruster <armbru@redhat.com>
> > > > Cc: Andreas Färber <afaerber@suse.de>
> > > > Cc: Daniel P. Berrange <berrange@redhat.com>
> > > > Cc: Eduardo Habkost <ehabkost@redhat.com>
> > > > ---
> > > [...]
> > > > diff --git a/qmp.c b/qmp.c
> > > > index 53affe2..baf25c0 100644
> > > > --- a/qmp.c
> > > > +++ b/qmp.c
> > > > @@ -460,6 +460,37 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements,
> > > >      return ret;
> > > >  }
> > > >  
> > > > +ObjectPropertyInfoList *qmp_qom_type_prop_list(const char *typename, Error **errp)
> > > > +{
> > > > +    ObjectClass *klass;
> > > > +    ObjectPropertyInfoList *props = NULL;
> > > > +    ObjectProperty *prop;
> > > > +    ObjectPropertyIterator iter;
> > > > +
> > > > +    klass = object_class_by_name(typename);
> > > > +    if (!klass) {
> > > > +        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> > > > +                  "Object class '%s' not found", typename);
> > > > +        return NULL;
> > > > +    }
> > > > +
> > > > +    object_class_property_iter_init(&iter, klass);
> > > > +    while ((prop = object_property_iter_next(&iter))) {
> > > > +        ObjectPropertyInfoList *entry = g_new0(ObjectPropertyInfoList, 1);
> > > > +
> > > > +        if (entry) {
> > > > +            entry->value = g_new0(ObjectPropertyInfo, 1);
> > > > +            entry->next = props;
> > > > +            props = entry;
> > > > +
> > > > +            entry->value->name = g_strdup(prop->name);
> > > > +            entry->value->type = g_strdup(prop->type);
> > > > +        }
> > > > +    }
> > > > +
> > > > +    return props;
> > > > +}
> > > > +
> > > 
> > > We already have "-device <type>,help", and it uses a completely
> > > different mechanism for listing properties. There's no reason for
> > > having two arbitrarily different APIs for listing properties
> > > returning different results.
> > > 
> > > If qmp_device_list_properties() is not enough for you, please
> > > clarify why, so we can consider improving it.
> > 
> > qmp_device_list_properties() has to actually instantiate an instance
> > of objects it is reporting properties against, since it is reporting
> > properties registered against object instances. In fact it only
> > reports properties against things which are TYPE_DEVICE - it'll refuse
> > to report other object types. Having to instantiate objects is inherantly
> > limiting to the command because there are some objects that cannot be
> > instantiated for this purpose. eg abstract objects and objects marked
> > "cannot_destroy_with_object_finalize_yet". Finally there is also a
> > performance and memory overhead in having to instantiate objects which
> > is best avoided.
> > 
> > This new API is reporting properties that are statically registered
> > against the *class* rather than than object instance. It is guaranteed
> > that you can always report these properties for any class without any
> > restrictions, nor any chance of side effects during instantiation.
> 
> The existing implementation has its limitations, but we can
> address those limitations without exporting a new API that return
> arbitrarily different results (that aren't even a superset of the
> existing API).
> 
> About the existing qmp_device_list_properties() limitations:
> 
> cannot_destroy_with_object_finalize_yet is supposed to eventually
> go away. If there are use cases that depend on listing properties
> for cannot_destroy_with_object_finalize_yet classes, we can fix
> that.
> 
> The TYPE_DEVICE requirement can be removed, as long as the
> non-device QOM classes are object_new()-safe like the existing
> cannot_destroy_with_object_finalize_yet=false device classes
> (they are supposed to be).
> 
> About having to instantiate objects: if optimizing that is so
> important, we can gradually convert the existing classes to use
> class-properties. While we convert them, we can even have a
> doesnt_need_to_instantiate_object_to_query_properties flag to
> indicate classes that were already converted. No need to export a
> new API.
> 
> Abstract classes are harder, but if they are important we can
> make them a special case inside the existing implementation
> instead of having two APIs.
>
>                              * * *
> 
> So, now we have enumerated the current API limitations. Can we
> enumerate the real world use cases that are affected by them, so
> we know which ones we need to address first?

Being able to list properties of arbitrary non-device objects is
really the critical thing that's missing right now, with abstract
types a close second.

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 v5] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties

[Valentin Rakush]

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

Hi all,

My five cents... I am checking for possible problems in case we want to
use qmp_device_list_properties() for listing all class properties. Here are
couple concerns:

- for example, we want to list class properties for "pc-q35-2.4-machine"
typename. This is not DeviceClass, therefore we have to change
qmp_device_list_properties to accept all classes. From another side,
qmp_device_list_properties is used for "-device FOO,help" (as far as I
understand from comments in qdev-core.h). Then use case "-device FOO,help"
will lose typecheck for DeviceClass. We will probably need a separate
implementation of '-device FOO,help' to check/assert command parameters.

- if we willl use qmp_device_list_properties to list properties of all
classes, then perhaps we should rename this function to something like
qmp_type_list_properties. In this case we should refactor source code that
already uses qmp_device_list_properties. For example, libvirt is already
uses device-list-properties command.

I will do more research.

Regards,
Valentin

On Wed, Jan 27, 2016 at 1:19 AM, Daniel P. Berrange <berrange@redhat.com>
wrote:

> On Tue, Jan 26, 2016 at 03:26:35PM -0200, Eduardo Habkost wrote:
> > On Tue, Jan 26, 2016 at 03:51:21PM +0000, Daniel P. Berrange wrote:
> > > On Tue, Jan 26, 2016 at 01:35:38PM -0200, Eduardo Habkost wrote:
> > > > On Mon, Jan 25, 2016 at 11:24:47AM +0300, Valentin Rakush wrote:
> > > > > This patch adds support for qom-type-prop-list command to list
> object
> > > > > class properties. A later patch will use this functionality to
> > > > > implement x86_64-cpu properties.
> > > > >
> > > > > Signed-off-by: Valentin Rakush <valentin.rakush@gmail.com>
> > > > > Cc: Luiz Capitulino <lcapitulino@redhat.com>
> > > > > Cc: Eric Blake <eblake@redhat.com>
> > > > > Cc: Markus Armbruster <armbru@redhat.com>
> > > > > Cc: Andreas Färber <afaerber@suse.de>
> > > > > Cc: Daniel P. Berrange <berrange@redhat.com>
> > > > > Cc: Eduardo Habkost <ehabkost@redhat.com>
> > > > > ---
> > > > [...]
> > > > > diff --git a/qmp.c b/qmp.c
> > > > > index 53affe2..baf25c0 100644
> > > > > --- a/qmp.c
> > > > > +++ b/qmp.c
> > > > > @@ -460,6 +460,37 @@ ObjectTypeInfoList *qmp_qom_list_types(bool
> has_implements,
> > > > >      return ret;
> > > > >  }
> > > > >
> > > > > +ObjectPropertyInfoList *qmp_qom_type_prop_list(const char
> *typename, Error **errp)
> > > > > +{
> > > > > +    ObjectClass *klass;
> > > > > +    ObjectPropertyInfoList *props = NULL;
> > > > > +    ObjectProperty *prop;
> > > > > +    ObjectPropertyIterator iter;
> > > > > +
> > > > > +    klass = object_class_by_name(typename);
> > > > > +    if (!klass) {
> > > > > +        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> > > > > +                  "Object class '%s' not found", typename);
> > > > > +        return NULL;
> > > > > +    }
> > > > > +
> > > > > +    object_class_property_iter_init(&iter, klass);
> > > > > +    while ((prop = object_property_iter_next(&iter))) {
> > > > > +        ObjectPropertyInfoList *entry =
> g_new0(ObjectPropertyInfoList, 1);
> > > > > +
> > > > > +        if (entry) {
> > > > > +            entry->value = g_new0(ObjectPropertyInfo, 1);
> > > > > +            entry->next = props;
> > > > > +            props = entry;
> > > > > +
> > > > > +            entry->value->name = g_strdup(prop->name);
> > > > > +            entry->value->type = g_strdup(prop->type);
> > > > > +        }
> > > > > +    }
> > > > > +
> > > > > +    return props;
> > > > > +}
> > > > > +
> > > >
> > > > We already have "-device <type>,help", and it uses a completely
> > > > different mechanism for listing properties. There's no reason for
> > > > having two arbitrarily different APIs for listing properties
> > > > returning different results.
> > > >
> > > > If qmp_device_list_properties() is not enough for you, please
> > > > clarify why, so we can consider improving it.
> > >
> > > qmp_device_list_properties() has to actually instantiate an instance
> > > of objects it is reporting properties against, since it is reporting
> > > properties registered against object instances. In fact it only
> > > reports properties against things which are TYPE_DEVICE - it'll refuse
> > > to report other object types. Having to instantiate objects is
> inherantly
> > > limiting to the command because there are some objects that cannot be
> > > instantiated for this purpose. eg abstract objects and objects marked
> > > "cannot_destroy_with_object_finalize_yet". Finally there is also a
> > > performance and memory overhead in having to instantiate objects which
> > > is best avoided.
> > >
> > > This new API is reporting properties that are statically registered
> > > against the *class* rather than than object instance. It is guaranteed
> > > that you can always report these properties for any class without any
> > > restrictions, nor any chance of side effects during instantiation.
> >
> > The existing implementation has its limitations, but we can
> > address those limitations without exporting a new API that return
> > arbitrarily different results (that aren't even a superset of the
> > existing API).
> >
> > About the existing qmp_device_list_properties() limitations:
> >
> > cannot_destroy_with_object_finalize_yet is supposed to eventually
> > go away. If there are use cases that depend on listing properties
> > for cannot_destroy_with_object_finalize_yet classes, we can fix
> > that.
> >
> > The TYPE_DEVICE requirement can be removed, as long as the
> > non-device QOM classes are object_new()-safe like the existing
> > cannot_destroy_with_object_finalize_yet=false device classes
> > (they are supposed to be).
> >
> > About having to instantiate objects: if optimizing that is so
> > important, we can gradually convert the existing classes to use
> > class-properties. While we convert them, we can even have a
> > doesnt_need_to_instantiate_object_to_query_properties flag to
> > indicate classes that were already converted. No need to export a
> > new API.
> >
> > Abstract classes are harder, but if they are important we can
> > make them a special case inside the existing implementation
> > instead of having two APIs.
> >
> >                              * * *
> >
> > So, now we have enumerated the current API limitations. Can we
> > enumerate the real world use cases that are affected by them, so
> > we know which ones we need to address first?
>
> Being able to list properties of arbitrary non-device objects is
> really the critical thing that's missing right now, with abstract
> types a close second.
>
> 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
> :|
>

[-- Attachment #2: Type: text/html, Size: 9933 bytes --]

Re: [Qemu-devel] [PATCH v5] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties

[Eduardo Habkost]

On Tue, Jan 26, 2016 at 10:19:13PM +0000, Daniel P. Berrange wrote:
> On Tue, Jan 26, 2016 at 03:26:35PM -0200, Eduardo Habkost wrote:
> > On Tue, Jan 26, 2016 at 03:51:21PM +0000, Daniel P. Berrange wrote:
> > > On Tue, Jan 26, 2016 at 01:35:38PM -0200, Eduardo Habkost wrote:
> > > > On Mon, Jan 25, 2016 at 11:24:47AM +0300, Valentin Rakush wrote:
> > > > > This patch adds support for qom-type-prop-list command to list object
> > > > > class properties. A later patch will use this functionality to
> > > > > implement x86_64-cpu properties.
> > > > > 
> > > > > Signed-off-by: Valentin Rakush <valentin.rakush@gmail.com>
> > > > > Cc: Luiz Capitulino <lcapitulino@redhat.com>
> > > > > Cc: Eric Blake <eblake@redhat.com>
> > > > > Cc: Markus Armbruster <armbru@redhat.com>
> > > > > Cc: Andreas Färber <afaerber@suse.de>
> > > > > Cc: Daniel P. Berrange <berrange@redhat.com>
> > > > > Cc: Eduardo Habkost <ehabkost@redhat.com>
> > > > > ---
> > > > [...]
> > > > > diff --git a/qmp.c b/qmp.c
> > > > > index 53affe2..baf25c0 100644
> > > > > --- a/qmp.c
> > > > > +++ b/qmp.c
> > > > > @@ -460,6 +460,37 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements,
> > > > >      return ret;
> > > > >  }
> > > > >  
> > > > > +ObjectPropertyInfoList *qmp_qom_type_prop_list(const char *typename, Error **errp)
> > > > > +{
> > > > > +    ObjectClass *klass;
> > > > > +    ObjectPropertyInfoList *props = NULL;
> > > > > +    ObjectProperty *prop;
> > > > > +    ObjectPropertyIterator iter;
> > > > > +
> > > > > +    klass = object_class_by_name(typename);
> > > > > +    if (!klass) {
> > > > > +        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> > > > > +                  "Object class '%s' not found", typename);
> > > > > +        return NULL;
> > > > > +    }
> > > > > +
> > > > > +    object_class_property_iter_init(&iter, klass);
> > > > > +    while ((prop = object_property_iter_next(&iter))) {
> > > > > +        ObjectPropertyInfoList *entry = g_new0(ObjectPropertyInfoList, 1);
> > > > > +
> > > > > +        if (entry) {
> > > > > +            entry->value = g_new0(ObjectPropertyInfo, 1);
> > > > > +            entry->next = props;
> > > > > +            props = entry;
> > > > > +
> > > > > +            entry->value->name = g_strdup(prop->name);
> > > > > +            entry->value->type = g_strdup(prop->type);
> > > > > +        }
> > > > > +    }
> > > > > +
> > > > > +    return props;
> > > > > +}
> > > > > +
> > > > 
> > > > We already have "-device <type>,help", and it uses a completely
> > > > different mechanism for listing properties. There's no reason for
> > > > having two arbitrarily different APIs for listing properties
> > > > returning different results.
> > > > 
> > > > If qmp_device_list_properties() is not enough for you, please
> > > > clarify why, so we can consider improving it.
> > > 
> > > qmp_device_list_properties() has to actually instantiate an instance
> > > of objects it is reporting properties against, since it is reporting
> > > properties registered against object instances. In fact it only
> > > reports properties against things which are TYPE_DEVICE - it'll refuse
> > > to report other object types. Having to instantiate objects is inherantly
> > > limiting to the command because there are some objects that cannot be
> > > instantiated for this purpose. eg abstract objects and objects marked
> > > "cannot_destroy_with_object_finalize_yet". Finally there is also a
> > > performance and memory overhead in having to instantiate objects which
> > > is best avoided.
> > > 
> > > This new API is reporting properties that are statically registered
> > > against the *class* rather than than object instance. It is guaranteed
> > > that you can always report these properties for any class without any
> > > restrictions, nor any chance of side effects during instantiation.
> > 
> > The existing implementation has its limitations, but we can
> > address those limitations without exporting a new API that return
> > arbitrarily different results (that aren't even a superset of the
> > existing API).
> > 
> > About the existing qmp_device_list_properties() limitations:
> > 
> > cannot_destroy_with_object_finalize_yet is supposed to eventually
> > go away. If there are use cases that depend on listing properties
> > for cannot_destroy_with_object_finalize_yet classes, we can fix
> > that.
> > 
> > The TYPE_DEVICE requirement can be removed, as long as the
> > non-device QOM classes are object_new()-safe like the existing
> > cannot_destroy_with_object_finalize_yet=false device classes
> > (they are supposed to be).
> > 
> > About having to instantiate objects: if optimizing that is so
> > important, we can gradually convert the existing classes to use
> > class-properties. While we convert them, we can even have a
> > doesnt_need_to_instantiate_object_to_query_properties flag to
> > indicate classes that were already converted. No need to export a
> > new API.
> > 
> > Abstract classes are harder, but if they are important we can
> > make them a special case inside the existing implementation
> > instead of having two APIs.
> >
> >                              * * *
> > 
> > So, now we have enumerated the current API limitations. Can we
> > enumerate the real world use cases that are affected by them, so
> > we know which ones we need to address first?
> 
> Being able to list properties of arbitrary non-device objects is
> really the critical thing that's missing right now, with abstract
> types a close second.

About abstract types: I thought we didn't export any class
hierarchy information. Should we do it? I guess we wouldn't want
clients to make assumptions about the class hierarchy.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v5] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties

[Daniel P. Berrange]

On Wed, Jan 27, 2016 at 01:09:37PM -0200, Eduardo Habkost wrote:
> On Tue, Jan 26, 2016 at 10:19:13PM +0000, Daniel P. Berrange wrote:
> > On Tue, Jan 26, 2016 at 03:26:35PM -0200, Eduardo Habkost wrote:
> > > On Tue, Jan 26, 2016 at 03:51:21PM +0000, Daniel P. Berrange wrote:
> > > > On Tue, Jan 26, 2016 at 01:35:38PM -0200, Eduardo Habkost wrote:
> > > > > On Mon, Jan 25, 2016 at 11:24:47AM +0300, Valentin Rakush wrote:
> > > > > > This patch adds support for qom-type-prop-list command to list object
> > > > > > class properties. A later patch will use this functionality to
> > > > > > implement x86_64-cpu properties.
> > > > > > 
> > > > > > Signed-off-by: Valentin Rakush <valentin.rakush@gmail.com>
> > > > > > Cc: Luiz Capitulino <lcapitulino@redhat.com>
> > > > > > Cc: Eric Blake <eblake@redhat.com>
> > > > > > Cc: Markus Armbruster <armbru@redhat.com>
> > > > > > Cc: Andreas Färber <afaerber@suse.de>
> > > > > > Cc: Daniel P. Berrange <berrange@redhat.com>
> > > > > > Cc: Eduardo Habkost <ehabkost@redhat.com>
> > > > > > ---
> > > > > [...]
> > > > > > diff --git a/qmp.c b/qmp.c
> > > > > > index 53affe2..baf25c0 100644
> > > > > > --- a/qmp.c
> > > > > > +++ b/qmp.c
> > > > > > @@ -460,6 +460,37 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements,
> > > > > >      return ret;
> > > > > >  }
> > > > > >  
> > > > > > +ObjectPropertyInfoList *qmp_qom_type_prop_list(const char *typename, Error **errp)
> > > > > > +{
> > > > > > +    ObjectClass *klass;
> > > > > > +    ObjectPropertyInfoList *props = NULL;
> > > > > > +    ObjectProperty *prop;
> > > > > > +    ObjectPropertyIterator iter;
> > > > > > +
> > > > > > +    klass = object_class_by_name(typename);
> > > > > > +    if (!klass) {
> > > > > > +        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> > > > > > +                  "Object class '%s' not found", typename);
> > > > > > +        return NULL;
> > > > > > +    }
> > > > > > +
> > > > > > +    object_class_property_iter_init(&iter, klass);
> > > > > > +    while ((prop = object_property_iter_next(&iter))) {
> > > > > > +        ObjectPropertyInfoList *entry = g_new0(ObjectPropertyInfoList, 1);
> > > > > > +
> > > > > > +        if (entry) {
> > > > > > +            entry->value = g_new0(ObjectPropertyInfo, 1);
> > > > > > +            entry->next = props;
> > > > > > +            props = entry;
> > > > > > +
> > > > > > +            entry->value->name = g_strdup(prop->name);
> > > > > > +            entry->value->type = g_strdup(prop->type);
> > > > > > +        }
> > > > > > +    }
> > > > > > +
> > > > > > +    return props;
> > > > > > +}
> > > > > > +
> > > > > 
> > > > > We already have "-device <type>,help", and it uses a completely
> > > > > different mechanism for listing properties. There's no reason for
> > > > > having two arbitrarily different APIs for listing properties
> > > > > returning different results.
> > > > > 
> > > > > If qmp_device_list_properties() is not enough for you, please
> > > > > clarify why, so we can consider improving it.
> > > > 
> > > > qmp_device_list_properties() has to actually instantiate an instance
> > > > of objects it is reporting properties against, since it is reporting
> > > > properties registered against object instances. In fact it only
> > > > reports properties against things which are TYPE_DEVICE - it'll refuse
> > > > to report other object types. Having to instantiate objects is inherantly
> > > > limiting to the command because there are some objects that cannot be
> > > > instantiated for this purpose. eg abstract objects and objects marked
> > > > "cannot_destroy_with_object_finalize_yet". Finally there is also a
> > > > performance and memory overhead in having to instantiate objects which
> > > > is best avoided.
> > > > 
> > > > This new API is reporting properties that are statically registered
> > > > against the *class* rather than than object instance. It is guaranteed
> > > > that you can always report these properties for any class without any
> > > > restrictions, nor any chance of side effects during instantiation.
> > > 
> > > The existing implementation has its limitations, but we can
> > > address those limitations without exporting a new API that return
> > > arbitrarily different results (that aren't even a superset of the
> > > existing API).
> > > 
> > > About the existing qmp_device_list_properties() limitations:
> > > 
> > > cannot_destroy_with_object_finalize_yet is supposed to eventually
> > > go away. If there are use cases that depend on listing properties
> > > for cannot_destroy_with_object_finalize_yet classes, we can fix
> > > that.
> > > 
> > > The TYPE_DEVICE requirement can be removed, as long as the
> > > non-device QOM classes are object_new()-safe like the existing
> > > cannot_destroy_with_object_finalize_yet=false device classes
> > > (they are supposed to be).
> > > 
> > > About having to instantiate objects: if optimizing that is so
> > > important, we can gradually convert the existing classes to use
> > > class-properties. While we convert them, we can even have a
> > > doesnt_need_to_instantiate_object_to_query_properties flag to
> > > indicate classes that were already converted. No need to export a
> > > new API.
> > > 
> > > Abstract classes are harder, but if they are important we can
> > > make them a special case inside the existing implementation
> > > instead of having two APIs.
> > >
> > >                              * * *
> > > 
> > > So, now we have enumerated the current API limitations. Can we
> > > enumerate the real world use cases that are affected by them, so
> > > we know which ones we need to address first?
> > 
> > Being able to list properties of arbitrary non-device objects is
> > really the critical thing that's missing right now, with abstract
> > types a close second.
> 
> About abstract types: I thought we didn't export any class
> hierarchy information. Should we do it? I guess we wouldn't want
> clients to make assumptions about the class hierarchy.

Actually I guess as long as there is one non-abstract subclass we can
query it doesn't matter.

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 v5] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties

[Valentin Rakush]

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

Hi Eduardo, hi Daniel,

I checked most of the classes that are used for x86_64 qemu simulation with
this command line:
x86_64-softmmu/qemu-system-x86_64 -qmp tcp:localhost:4444,server,nowait
-machine pc -cpu core2duo

Here are some of the classes that cannot provide properties with
device_list_properties call:
/object/machine/generic-pc-machine/pc-0.13-machine
/object/bus/i2c-bus
/interface/user-creatable
/object/tls-creds/tls-creds-anon
/object/memory-backend/memory-backend-file
/object/qemu:memory-region
/object/rng-backend/rng-random
/object/tpm-backend/tpm-passthrough
/object/tls-creds/tls-creds-x509
/object/secret

They cannot provide properties because these classes cannot be casted to
TYPE_DEVICE. This is done intentionally because TYPE_DEVICE has its own
properties. Also TYPE_MACHINE has own properties of type GlobalProperty.
Here are two ways (AFAICS):
- we refactor TYPE_DEVICE and TYPE_MACHINE so they store their properties
in the ObjectClass properties.
- we change device_list_properties so it process different classes
differently.

The disadvantage of the second approach, is that it is complicating code in
favor of simplifying qapi interface. I like first approach with
refactoring, although it is more complex. The first approach should put all
properties in the base classes and then use this properties everywhere
(command line help, qmp etc.) The simplest way the refactoring can be done,
is by moving TYPE_DEVICE properties to ObjectClass and merging them somehow
with TYPE_MACHINE GlobalProperty. Then we will use these properties for all
other types of classes.

Of course, we can leave device_list_properties as it is and use
qom-type-prop-list instead.

What do you think? Does these design options make sense for you?


Thank you,
Valentin






On Wed, Jan 27, 2016 at 6:23 PM, Daniel P. Berrange <berrange@redhat.com>
wrote:

> On Wed, Jan 27, 2016 at 01:09:37PM -0200, Eduardo Habkost wrote:
> > On Tue, Jan 26, 2016 at 10:19:13PM +0000, Daniel P. Berrange wrote:
> > > On Tue, Jan 26, 2016 at 03:26:35PM -0200, Eduardo Habkost wrote:
> > > > On Tue, Jan 26, 2016 at 03:51:21PM +0000, Daniel P. Berrange wrote:
> > > > > On Tue, Jan 26, 2016 at 01:35:38PM -0200, Eduardo Habkost wrote:
> > > > > > On Mon, Jan 25, 2016 at 11:24:47AM +0300, Valentin Rakush wrote:
> > > > > > > This patch adds support for qom-type-prop-list command to list
> object
> > > > > > > class properties. A later patch will use this functionality to
> > > > > > > implement x86_64-cpu properties.
> > > > > > >
> > > > > > > Signed-off-by: Valentin Rakush <valentin.rakush@gmail.com>
> > > > > > > Cc: Luiz Capitulino <lcapitulino@redhat.com>
> > > > > > > Cc: Eric Blake <eblake@redhat.com>
> > > > > > > Cc: Markus Armbruster <armbru@redhat.com>
> > > > > > > Cc: Andreas Färber <afaerber@suse.de>
> > > > > > > Cc: Daniel P. Berrange <berrange@redhat.com>
> > > > > > > Cc: Eduardo Habkost <ehabkost@redhat.com>
> > > > > > > ---
> > > > > > [...]
> > > > > > > diff --git a/qmp.c b/qmp.c
> > > > > > > index 53affe2..baf25c0 100644
> > > > > > > --- a/qmp.c
> > > > > > > +++ b/qmp.c
> > > > > > > @@ -460,6 +460,37 @@ ObjectTypeInfoList
> *qmp_qom_list_types(bool has_implements,
> > > > > > >      return ret;
> > > > > > >  }
> > > > > > >
> > > > > > > +ObjectPropertyInfoList *qmp_qom_type_prop_list(const char
> *typename, Error **errp)
> > > > > > > +{
> > > > > > > +    ObjectClass *klass;
> > > > > > > +    ObjectPropertyInfoList *props = NULL;
> > > > > > > +    ObjectProperty *prop;
> > > > > > > +    ObjectPropertyIterator iter;
> > > > > > > +
> > > > > > > +    klass = object_class_by_name(typename);
> > > > > > > +    if (!klass) {
> > > > > > > +        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> > > > > > > +                  "Object class '%s' not found", typename);
> > > > > > > +        return NULL;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    object_class_property_iter_init(&iter, klass);
> > > > > > > +    while ((prop = object_property_iter_next(&iter))) {
> > > > > > > +        ObjectPropertyInfoList *entry =
> g_new0(ObjectPropertyInfoList, 1);
> > > > > > > +
> > > > > > > +        if (entry) {
> > > > > > > +            entry->value = g_new0(ObjectPropertyInfo, 1);
> > > > > > > +            entry->next = props;
> > > > > > > +            props = entry;
> > > > > > > +
> > > > > > > +            entry->value->name = g_strdup(prop->name);
> > > > > > > +            entry->value->type = g_strdup(prop->type);
> > > > > > > +        }
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    return props;
> > > > > > > +}
> > > > > > > +
> > > > > >
> > > > > > We already have "-device <type>,help", and it uses a completely
> > > > > > different mechanism for listing properties. There's no reason for
> > > > > > having two arbitrarily different APIs for listing properties
> > > > > > returning different results.
> > > > > >
> > > > > > If qmp_device_list_properties() is not enough for you, please
> > > > > > clarify why, so we can consider improving it.
> > > > >
> > > > > qmp_device_list_properties() has to actually instantiate an
> instance
> > > > > of objects it is reporting properties against, since it is
> reporting
> > > > > properties registered against object instances. In fact it only
> > > > > reports properties against things which are TYPE_DEVICE - it'll
> refuse
> > > > > to report other object types. Having to instantiate objects is
> inherantly
> > > > > limiting to the command because there are some objects that cannot
> be
> > > > > instantiated for this purpose. eg abstract objects and objects
> marked
> > > > > "cannot_destroy_with_object_finalize_yet". Finally there is also a
> > > > > performance and memory overhead in having to instantiate objects
> which
> > > > > is best avoided.
> > > > >
> > > > > This new API is reporting properties that are statically registered
> > > > > against the *class* rather than than object instance. It is
> guaranteed
> > > > > that you can always report these properties for any class without
> any
> > > > > restrictions, nor any chance of side effects during instantiation.
> > > >
> > > > The existing implementation has its limitations, but we can
> > > > address those limitations without exporting a new API that return
> > > > arbitrarily different results (that aren't even a superset of the
> > > > existing API).
> > > >
> > > > About the existing qmp_device_list_properties() limitations:
> > > >
> > > > cannot_destroy_with_object_finalize_yet is supposed to eventually
> > > > go away. If there are use cases that depend on listing properties
> > > > for cannot_destroy_with_object_finalize_yet classes, we can fix
> > > > that.
> > > >
> > > > The TYPE_DEVICE requirement can be removed, as long as the
> > > > non-device QOM classes are object_new()-safe like the existing
> > > > cannot_destroy_with_object_finalize_yet=false device classes
> > > > (they are supposed to be).
> > > >
> > > > About having to instantiate objects: if optimizing that is so
> > > > important, we can gradually convert the existing classes to use
> > > > class-properties. While we convert them, we can even have a
> > > > doesnt_need_to_instantiate_object_to_query_properties flag to
> > > > indicate classes that were already converted. No need to export a
> > > > new API.
> > > >
> > > > Abstract classes are harder, but if they are important we can
> > > > make them a special case inside the existing implementation
> > > > instead of having two APIs.
> > > >
> > > >                              * * *
> > > >
> > > > So, now we have enumerated the current API limitations. Can we
> > > > enumerate the real world use cases that are affected by them, so
> > > > we know which ones we need to address first?
> > >
> > > Being able to list properties of arbitrary non-device objects is
> > > really the critical thing that's missing right now, with abstract
> > > types a close second.
> >
> > About abstract types: I thought we didn't export any class
> > hierarchy information. Should we do it? I guess we wouldn't want
> > clients to make assumptions about the class hierarchy.
>
> Actually I guess as long as there is one non-abstract subclass we can
> query it doesn't matter.
>
> 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
> :|
>



-- 
Best Regards,
Valentin Rakush.

[-- Attachment #2: Type: text/html, Size: 12662 bytes --]

Re: [Qemu-devel] [PATCH v5] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties

[Eduardo Habkost]

On Fri, Jan 29, 2016 at 01:03:38PM +0300, Valentin Rakush wrote:
> Hi Eduardo, hi Daniel,
> 
> I checked most of the classes that are used for x86_64 qemu simulation with
> this command line:
> x86_64-softmmu/qemu-system-x86_64 -qmp tcp:localhost:4444,server,nowait
> -machine pc -cpu core2duo
> 
> Here are some of the classes that cannot provide properties with
> device_list_properties call:
> /object/machine/generic-pc-machine/pc-0.13-machine
> /object/bus/i2c-bus
> /interface/user-creatable
> /object/tls-creds/tls-creds-anon
> /object/memory-backend/memory-backend-file
> /object/qemu:memory-region
> /object/rng-backend/rng-random
> /object/tpm-backend/tpm-passthrough
> /object/tls-creds/tls-creds-x509
> /object/secret
> 
> They cannot provide properties because these classes cannot be casted to
> TYPE_DEVICE. This is done intentionally because TYPE_DEVICE has its own
> properties.

Can you clarify what you mean by "TYPE_DEVICE has its own
properties"? TYPE_DEVICE properties are registered as normal QOM
properties.

We can still add a new command that's not specific for
TYPE_DEVICE (if necessary). The point is that it shouldn't return
arbitrarily different (and incomplete) data from the existing
mechanism to list properties.

In other words, I don't see why the output of "qom-type-prop-list
<type>" can't be as good as the output of "device-list-properties
<type>". If we make return only class-properties, it will be less
complete and less useful.


> Also TYPE_MACHINE has own properties of type GlobalProperty.

I don't understand what you mean, here. GlobalProperties are not
machine properties, they are just property=value pairs to be
registered as global properties. They are unrelated to the
properties TYPE_MACHINE actually has.

> Here are two ways (AFAICS):
> - we refactor TYPE_DEVICE and TYPE_MACHINE so they store their properties
> in the ObjectClass properties.

Too many classes need to be converted. We would still need
something to use during the transiation.

> - we change device_list_properties so it process different classes
> differently.

Could you clarify what you mean by "process different classes
differently"?

A third option is to just use object_new(), like
qmp_device_list_properties() already does.

> 
> The disadvantage of the second approach, is that it is complicating code in
> favor of simplifying qapi interface. I like first approach with
> refactoring, although it is more complex. The first approach should put all
> properties in the base classes and then use this properties everywhere
> (command line help, qmp etc.) The simplest way the refactoring can be done,
> is by moving TYPE_DEVICE properties to ObjectClass and merging them somehow
> with TYPE_MACHINE GlobalProperty. Then we will use these properties for all
> other types of classes.
> 
> Of course, we can leave device_list_properties as it is and use
> qom-type-prop-list instead.
> 
> What do you think? Does these design options make sense for you?

We can add a new command if we don't want to change how
device-list-properties work. But first I would like to understand
the actual reasons for the new command, because we can't argue
about it if we don't know what the command output will be used
for. How exactly would callers qom-type-prop-list use that
information?

I see 3 cases where property names are used:

1) QMP QOM commands (qom-get/qom-set):

These properties are available using qom-list, already.

2) -device/device_add options:

These properties are available in device-list-properties,
already.

3) -object/object-add options:

In this case, if you want to return complete data, you only have
two options: a) convert all TYPE_USER_CREATABLE classes to use
class-properties; or b) use the same approach used by
qmp_device_list_properties() (object_new() followed by
enumeration of properteis).

4) -machine options:

This is similar to -object: the list will be incomplete unless
all machine subclasses are converted to use only
class-properties, or the new command uses object_new().

5) -cpu options:

Ditto. the list will be incomplete unless all CPU subclasses are
converted to use only class-properties, or the new command uses
object_new().


That doesn't mean we don't want to convert other classes to use
class-properties later to simplify internal QEMU code. But if you
want to propose a new QMP command, let's make one that returns
useful data for real use cases.

I am not sure the list above is complete, so I would like to
understand how exactly the data you want to return will be used.
So for each of the classes you mentioned, I would like to know:

> /object/machine/generic-pc-machine/pc-0.13-machine

What exactly do you think the caller use the output of
"qom-type-prop-list pc-0.13-machine" for? How exactly? Would it
use them in the QEMU command-line? In other QMP commands? Which
ones?

> /object/bus/i2c-bus

Ditto, what exactly do you tink the caller would do with the
output of "qom-type-prop-list i2c-bus"?

> /interface/user-creatable

user-creatable is an interface. If you want to allow interfaces
to register class-properties, you probably need special code for
them during object creation.

Also, see my question about abstract classes in my previous reply
to Daniel. We can deal with interfaces and abstract classes
later, as we don't even expose class hierarchy information to the
outside (AFAIK).

> /object/tls-creds/tls-creds-anon

What exactly do you tink the caller would do with the output of
"qom-type-prop-list tls-creds-anon"?

> /object/memory-backend/memory-backend-file
> /object/qemu:memory-region
> /object/rng-backend/rng-random
> /object/tpm-backend/tpm-passthrough
> /object/tls-creds/tls-creds-x509
> /object/secret

Ditto for each of the classes above.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v5] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties

[Valentin Rakush]

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

Hi Eduardo,

I will try to answer some of your questions at this email and will answer
other questions later.

> Can you clarify what you mean by "TYPE_DEVICE has its own
> properties"? TYPE_DEVICE properties are registered as normal QOM
> properties.

It is possible that I do not understand object model correctly....

This commit 16bf7f522a2f adds GHashTable *properties; to the ObjectClass
struct in the include/qom/object.h
The typedef struct DeviceClass from include/hw/qdev-core.h is inherited
from ObjectClass. Also DeviceClass has it own properties
Property *props.

In the device_list_properties we call

static DevicePropertyInfo *make_device_property_info

Which tries to downcast class to DEVICE_CLASS

for (prop = DEVICE_CLASS(klass)->props; prop && prop->name; prop++) {

So we are using Property *props, defined in the DeviceClass, but we do not
use GHashTable * properties, defined in the ObjectClass. Here I mean that
DeviceClass has its own properties.

> I don't understand what you mean, here. GlobalProperties are not
> machine properties, they are just property=value pairs to be
> registered as global properties. They are unrelated to the
> properties TYPE_MACHINE actually has.

Same here. The struct MachineClass is defined in the include/hw/boards.h It
has a member GlobalProperty *compat_props;
But after commit 16bf7f522a2f it would be better to use ObjectClass
properties. IMHO. I did not check how compat_props are used in the code yet.

> Could you clarify what you mean by "process different classes
> differently"?

In the list_device_properties function we should have several conditional
statements like

if (machine = object_class_dynamic_cast(class, TYPE_MACHINE)) {
/* process machine properties using MachineClass GlobalProperty
*compat_props; */
}
else if (machine = object_class_dynamic_cast(class, TYPE_DEVICE)) {
/* process device class properties, using DeviceClass Property *props; */
}
else if (machine = object_class_dynamic_cast(class, TYPE_CPU)) {
/* process CPU, using ObjectClass GHashTable *properties; */
}

> 5) -cpu options:
>
> Ditto. the list will be incomplete unless all CPU subclasses are
> converted to use only class-properties, or the new command uses
> object_new().

This is a use case that I initially tried to implement.

Regards,
Valentin

On Fri, Jan 29, 2016 at 6:28 PM, Eduardo Habkost <ehabkost@redhat.com>
wrote:

> On Fri, Jan 29, 2016 at 01:03:38PM +0300, Valentin Rakush wrote:
> > Hi Eduardo, hi Daniel,
> >
> > I checked most of the classes that are used for x86_64 qemu simulation
> with
> > this command line:
> > x86_64-softmmu/qemu-system-x86_64 -qmp tcp:localhost:4444,server,nowait
> > -machine pc -cpu core2duo
> >
> > Here are some of the classes that cannot provide properties with
> > device_list_properties call:
> > /object/machine/generic-pc-machine/pc-0.13-machine
> > /object/bus/i2c-bus
> > /interface/user-creatable
> > /object/tls-creds/tls-creds-anon
> > /object/memory-backend/memory-backend-file
> > /object/qemu:memory-region
> > /object/rng-backend/rng-random
> > /object/tpm-backend/tpm-passthrough
> > /object/tls-creds/tls-creds-x509
> > /object/secret
> >
> > They cannot provide properties because these classes cannot be casted to
> > TYPE_DEVICE. This is done intentionally because TYPE_DEVICE has its own
> > properties.
>
> Can you clarify what you mean by "TYPE_DEVICE has its own
> properties"? TYPE_DEVICE properties are registered as normal QOM
> properties.
>
> We can still add a new command that's not specific for
> TYPE_DEVICE (if necessary). The point is that it shouldn't return
> arbitrarily different (and incomplete) data from the existing
> mechanism to list properties.
>
> In other words, I don't see why the output of "qom-type-prop-list
> <type>" can't be as good as the output of "device-list-properties
> <type>". If we make return only class-properties, it will be less
> complete and less useful.
>
>
> > Also TYPE_MACHINE has own properties of type GlobalProperty.
>
> I don't understand what you mean, here. GlobalProperties are not
> machine properties, they are just property=value pairs to be
> registered as global properties. They are unrelated to the
> properties TYPE_MACHINE actually has.
>
> > Here are two ways (AFAICS):
> > - we refactor TYPE_DEVICE and TYPE_MACHINE so they store their properties
> > in the ObjectClass properties.
>
> Too many classes need to be converted. We would still need
> something to use during the transiation.
>
> > - we change device_list_properties so it process different classes
> > differently.
>
> Could you clarify what you mean by "process different classes
> differently"?
>
> A third option is to just use object_new(), like
> qmp_device_list_properties() already does.
>
> >
> > The disadvantage of the second approach, is that it is complicating code
> in
> > favor of simplifying qapi interface. I like first approach with
> > refactoring, although it is more complex. The first approach should put
> all
> > properties in the base classes and then use this properties everywhere
> > (command line help, qmp etc.) The simplest way the refactoring can be
> done,
> > is by moving TYPE_DEVICE properties to ObjectClass and merging them
> somehow
> > with TYPE_MACHINE GlobalProperty. Then we will use these properties for
> all
> > other types of classes.
> >
> > Of course, we can leave device_list_properties as it is and use
> > qom-type-prop-list instead.
> >
> > What do you think? Does these design options make sense for you?
>
> We can add a new command if we don't want to change how
> device-list-properties work. But first I would like to understand
> the actual reasons for the new command, because we can't argue
> about it if we don't know what the command output will be used
> for. How exactly would callers qom-type-prop-list use that
> information?
>
> I see 3 cases where property names are used:
>
> 1) QMP QOM commands (qom-get/qom-set):
>
> These properties are available using qom-list, already.
>
> 2) -device/device_add options:
>
> These properties are available in device-list-properties,
> already.
>
> 3) -object/object-add options:
>
> In this case, if you want to return complete data, you only have
> two options: a) convert all TYPE_USER_CREATABLE classes to use
> class-properties; or b) use the same approach used by
> qmp_device_list_properties() (object_new() followed by
> enumeration of properteis).
>
> 4) -machine options:
>
> This is similar to -object: the list will be incomplete unless
> all machine subclasses are converted to use only
> class-properties, or the new command uses object_new().
>
> 5) -cpu options:
>
> Ditto. the list will be incomplete unless all CPU subclasses are
> converted to use only class-properties, or the new command uses
> object_new().
>
>
> That doesn't mean we don't want to convert other classes to use
> class-properties later to simplify internal QEMU code. But if you
> want to propose a new QMP command, let's make one that returns
> useful data for real use cases.
>
> I am not sure the list above is complete, so I would like to
> understand how exactly the data you want to return will be used.
> So for each of the classes you mentioned, I would like to know:
>
> > /object/machine/generic-pc-machine/pc-0.13-machine
>
> What exactly do you think the caller use the output of
> "qom-type-prop-list pc-0.13-machine" for? How exactly? Would it
> use them in the QEMU command-line? In other QMP commands? Which
> ones?
>
> > /object/bus/i2c-bus
>
> Ditto, what exactly do you tink the caller would do with the
> output of "qom-type-prop-list i2c-bus"?
>
> > /interface/user-creatable
>
> user-creatable is an interface. If you want to allow interfaces
> to register class-properties, you probably need special code for
> them during object creation.
>
> Also, see my question about abstract classes in my previous reply
> to Daniel. We can deal with interfaces and abstract classes
> later, as we don't even expose class hierarchy information to the
> outside (AFAIK).
>
> > /object/tls-creds/tls-creds-anon
>
> What exactly do you tink the caller would do with the output of
> "qom-type-prop-list tls-creds-anon"?
>
> > /object/memory-backend/memory-backend-file
> > /object/qemu:memory-region
> > /object/rng-backend/rng-random
> > /object/tpm-backend/tpm-passthrough
> > /object/tls-creds/tls-creds-x509
> > /object/secret
>
> Ditto for each of the classes above.
>
> --
> Eduardo
>



-- 
Best Regards,
Valentin Rakush.

[-- Attachment #2: Type: text/html, Size: 12245 bytes --]

Re: [Qemu-devel] [PATCH v5] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties

[Eduardo Habkost]

On Sun, Jan 31, 2016 at 04:40:54PM +0300, Valentin Rakush wrote:
> Hi Eduardo,
> 
> I will try to answer some of your questions at this email and will answer
> other questions later.
> 
> > Can you clarify what you mean by "TYPE_DEVICE has its own
> > properties"? TYPE_DEVICE properties are registered as normal QOM
> > properties.
> 
> It is possible that I do not understand object model correctly....
> 
> This commit 16bf7f522a2f adds GHashTable *properties; to the ObjectClass
> struct in the include/qom/object.h
> The typedef struct DeviceClass from include/hw/qdev-core.h is inherited
> from ObjectClass. Also DeviceClass has it own properties
> Property *props.
> 
> In the device_list_properties we call
> 
> static DevicePropertyInfo *make_device_property_info
> 
> Which tries to downcast class to DEVICE_CLASS
> 
> for (prop = DEVICE_CLASS(klass)->props; prop && prop->name; prop++) {
> 
> So we are using Property *props, defined in the DeviceClass, but we do not
> use GHashTable * properties, defined in the ObjectClass. Here I mean that
> DeviceClass has its own properties.

Oh, I misunderstood you. I was talking about object properties,
the ones at Object::properties. Yes, in this case we have
duplication between DeviceClass::props and
ObjectClass::properties.

> 
> > I don't understand what you mean, here. GlobalProperties are not
> > machine properties, they are just property=value pairs to be
> > registered as global properties. They are unrelated to the
> > properties TYPE_MACHINE actually has.
> 
> Same here. The struct MachineClass is defined in the include/hw/boards.h It
> has a member GlobalProperty *compat_props;
> But after commit 16bf7f522a2f it would be better to use ObjectClass
> properties. IMHO. I did not check how compat_props are used in the code yet.

In this case it's different: ObjectClass::compat_props are not
machine properties. They are just property=value pairs to be
registered as global properties when running the machine. They
will never appear in qom-type-prop-list because they are a
completely different thing.

> 
> > Could you clarify what you mean by "process different classes
> > differently"?
> 
> In the list_device_properties function we should have several conditional
> statements like
> 
> if (machine = object_class_dynamic_cast(class, TYPE_MACHINE)) {
> /* process machine properties using MachineClass GlobalProperty
> *compat_props; */
> }
> else if (machine = object_class_dynamic_cast(class, TYPE_DEVICE)) {
> /* process device class properties, using DeviceClass Property *props; */
> }
> else if (machine = object_class_dynamic_cast(class, TYPE_CPU)) {
> /* process CPU, using ObjectClass GHashTable *properties; */
> }

You don't have to, if you just do object_new() like
qmp_device_list_properties() does. Both ObjectClass::properties
and DeviceClas::props are translated to object instance
properties (Object::properties).

> 
> > 5) -cpu options:
> >
> > Ditto. the list will be incomplete unless all CPU subclasses are
> > converted to use only class-properties, or the new command uses
> > object_new().
> 
> This is a use case that I initially tried to implement.

This use case can be implemented easily using object_new(), like
qmp_device_list_properties() already does.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v5] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties

[Valentin Rakush]

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

Hi Eduardo,

thank you for your explanations.

> You don't have to, if you just do object_new() like
> qmp_device_list_properties() does. Both ObjectClass::properties
> and DeviceClas::props are translated to object instance
> properties (Object::properties).

I should foresee these. Qemu has the object oriented approach implemented
in C.
I missed this point. Thank you for explanation.

I did the following changes in the target-i386/cpu.c

     dc->props = host_x86_cpu_properties;
     /* Reason: host_x86_cpu_initfn() dies when !kvm_enabled() */
-    dc->cannot_destroy_with_object_finalize_yet = true;
+    dc->cannot_destroy_with_object_finalize_yet = false;

and

     /*
      * Reason: x86_cpu_initfn() calls cpu_exec_init(), which saves the
      * object in cpus -> dangling pointer after final object_unref().
      */
-    dc->cannot_destroy_with_object_finalize_yet = true;
+    dc->cannot_destroy_with_object_finalize_yet = false;

and now I can add class properties to the target-i386/cpu.c from this patch
http://lists.nongnu.org/archive/html/qemu-devel/2015-08/msg03117.html
and then command "qemu-system-x86_64 -device core2duo-x86_64-cpu,help"
will show me the class properties.

However according to this patch
http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg05013.html
I am checking how to make cpu class not to fail in device-list-properties
call
throught QMP interface.

Basically my goal is to put class properties into the target-i386/cpu.c and
print them when necessary. And after our discussion
qmp_device_list_properties
is a already available for this but cannot_destroy_with_object_finalize_yet
flag should be set to false.

Please let me know if this is wrong approach.

Thank you,
Valentin

On Tue, Feb 2, 2016 at 6:55 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Sun, Jan 31, 2016 at 04:40:54PM +0300, Valentin Rakush wrote:
> > Hi Eduardo,
> >
> > I will try to answer some of your questions at this email and will answer
> > other questions later.
> >
> > > Can you clarify what you mean by "TYPE_DEVICE has its own
> > > properties"? TYPE_DEVICE properties are registered as normal QOM
> > > properties.
> >
> > It is possible that I do not understand object model correctly....
> >
> > This commit 16bf7f522a2f adds GHashTable *properties; to the ObjectClass
> > struct in the include/qom/object.h
> > The typedef struct DeviceClass from include/hw/qdev-core.h is inherited
> > from ObjectClass. Also DeviceClass has it own properties
> > Property *props.
> >
> > In the device_list_properties we call
> >
> > static DevicePropertyInfo *make_device_property_info
> >
> > Which tries to downcast class to DEVICE_CLASS
> >
> > for (prop = DEVICE_CLASS(klass)->props; prop && prop->name; prop++) {
> >
> > So we are using Property *props, defined in the DeviceClass, but we do
> not
> > use GHashTable * properties, defined in the ObjectClass. Here I mean that
> > DeviceClass has its own properties.
>
> Oh, I misunderstood you. I was talking about object properties,
> the ones at Object::properties. Yes, in this case we have
> duplication between DeviceClass::props and
> ObjectClass::properties.
>
> >
> > > I don't understand what you mean, here. GlobalProperties are not
> > > machine properties, they are just property=value pairs to be
> > > registered as global properties. They are unrelated to the
> > > properties TYPE_MACHINE actually has.
> >
> > Same here. The struct MachineClass is defined in the include/hw/boards.h
> It
> > has a member GlobalProperty *compat_props;
> > But after commit 16bf7f522a2f it would be better to use ObjectClass
> > properties. IMHO. I did not check how compat_props are used in the code
> yet.
>
> In this case it's different: ObjectClass::compat_props are not
> machine properties. They are just property=value pairs to be
> registered as global properties when running the machine. They
> will never appear in qom-type-prop-list because they are a
> completely different thing.
>
> >
> > > Could you clarify what you mean by "process different classes
> > > differently"?
> >
> > In the list_device_properties function we should have several conditional
> > statements like
> >
> > if (machine = object_class_dynamic_cast(class, TYPE_MACHINE)) {
> > /* process machine properties using MachineClass GlobalProperty
> > *compat_props; */
> > }
> > else if (machine = object_class_dynamic_cast(class, TYPE_DEVICE)) {
> > /* process device class properties, using DeviceClass Property *props; */
> > }
> > else if (machine = object_class_dynamic_cast(class, TYPE_CPU)) {
> > /* process CPU, using ObjectClass GHashTable *properties; */
> > }
>
> You don't have to, if you just do object_new() like
> qmp_device_list_properties() does. Both ObjectClass::properties
> and DeviceClas::props are translated to object instance
> properties (Object::properties).
>
> >
> > > 5) -cpu options:
> > >
> > > Ditto. the list will be incomplete unless all CPU subclasses are
> > > converted to use only class-properties, or the new command uses
> > > object_new().
> >
> > This is a use case that I initially tried to implement.
>
> This use case can be implemented easily using object_new(), like
> qmp_device_list_properties() already does.
>
> --
> Eduardo
>

[-- Attachment #2: Type: text/html, Size: 7084 bytes --]