All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v4] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties
@ 2016-01-22 12:15 Valentin Rakush
  2016-01-22 13:00 ` Daniel P. Berrange
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Valentin Rakush @ 2016-01-22 12:15 UTC (permalink / raw)
  To: qemu-devel; +Cc: armbru, lcapitulino, asmetanin, den, Valentin Rakush, afaerber

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>
---
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                | 32 ++++++++++++++++++++++++++++++++
 qom/object.c         |  7 +++++++
 8 files changed, 130 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..643e4bc 100644
--- a/qmp.c
+++ b/qmp.c
@@ -460,6 +460,38 @@ 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

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

* Re: [Qemu-devel] [PATCH v4] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties
  2016-01-22 12:15 [Qemu-devel] [PATCH v4] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties Valentin Rakush
@ 2016-01-22 13:00 ` Daniel P. Berrange
  2016-01-22 13:16 ` Denis V. Lunev
  2016-01-22 19:02 ` Eric Blake
  2 siblings, 0 replies; 7+ messages in thread
From: Daniel P. Berrange @ 2016-01-22 13:00 UTC (permalink / raw)
  To: Valentin Rakush; +Cc: armbru, qemu-devel, lcapitulino, asmetanin, den, afaerber

On Fri, Jan 22, 2016 at 03:15:55PM +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>
> ---
> 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                | 32 ++++++++++++++++++++++++++++++++
>  qom/object.c         |  7 +++++++
>  8 files changed, 130 insertions(+)

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

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

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

* Re: [Qemu-devel] [PATCH v4] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties
  2016-01-22 12:15 [Qemu-devel] [PATCH v4] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties Valentin Rakush
  2016-01-22 13:00 ` Daniel P. Berrange
@ 2016-01-22 13:16 ` Denis V. Lunev
  2016-01-22 19:02 ` Eric Blake
  2 siblings, 0 replies; 7+ messages in thread
From: Denis V. Lunev @ 2016-01-22 13:16 UTC (permalink / raw)
  To: Valentin Rakush, qemu-devel; +Cc: armbru, lcapitulino, asmetanin, afaerber

On 01/22/2016 03:15 PM, 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>
> ---
> 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                | 32 ++++++++++++++++++++++++++++++++
>   qom/object.c         |  7 +++++++
>   8 files changed, 130 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..643e4bc 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -460,6 +460,38 @@ 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)
>   {
irbis ~/src/qemu $ scripts/checkpatch.pl 
0001-qom-qmp-hmp-qapi-create-qom-type-prop-list-for-class.patch
WARNING: line over 80 characters
#194: FILE: qmp.c:463:
+ObjectPropertyInfoList *qmp_qom_type_prop_list(const char *typename, 
Error **errp)

ERROR: "foo* bar" should be "foo *bar"
#198: FILE: qmp.c:467:
+    ObjectProperty* prop;

ERROR: that open brace { should be on the previous line
#212: FILE: qmp.c:481:
+        if (entry)
+        {

total: 2 errors, 1 warnings, 175 lines checked

0001-qom-qmp-hmp-qapi-create-qom-type-prop-list-for-class.patch has 
style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
irbis ~/src/qemu $

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

* Re: [Qemu-devel] [PATCH v4] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties
  2016-01-22 12:15 [Qemu-devel] [PATCH v4] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties Valentin Rakush
  2016-01-22 13:00 ` Daniel P. Berrange
  2016-01-22 13:16 ` Denis V. Lunev
@ 2016-01-22 19:02 ` Eric Blake
  2016-01-22 20:20   ` Valentin Rakush
  2 siblings, 1 reply; 7+ messages in thread
From: Eric Blake @ 2016-01-22 19:02 UTC (permalink / raw)
  To: Valentin Rakush, qemu-devel; +Cc: armbru, lcapitulino, asmetanin, den, afaerber

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

On 01/22/2016 05:15 AM, 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>
> ---

> +++ b/hmp-commands.hx
> @@ -1734,6 +1734,19 @@ Print QOM properties of object at location @var{path}
>  ETEXI
>  
>      {
> +        .name       = "qom-type-prop-list",

To be consistent with most existing HMP commands, this should use
qmp_type_prop_list (only the QMP version should use '-').

For that matter, should this really be a new top-level command, or
should it be a subcommand of the 'info' command?

The QMP command looks fine, though.

-- 
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 --]

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

* Re: [Qemu-devel] [PATCH v4] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties
  2016-01-22 19:02 ` Eric Blake
@ 2016-01-22 20:20   ` Valentin Rakush
  2016-01-25 17:43     ` Eric Blake
  0 siblings, 1 reply; 7+ messages in thread
From: Valentin Rakush @ 2016-01-22 20:20 UTC (permalink / raw)
  To: Eric Blake
  Cc: Markus Armbruster, qemu-devel, lcapitulino, asmetanin,
	Denis V. Lunev, Andreas Färber

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

Hi Eric, hi Daniel,

Re dashes in the command name

AFAICC, the QOM related command in HMP use dash "-". For example, qom-list
and qom-set. If we will change dash "-" to underscore "_" then
qom_type_prop_list will be consistent with old HMP commands (created before
year 2013), but will _not_ be consistent with QOM commands created after
2013. Which is not nice and may be misleading.

If we want to have consistent naming of all HMP commands, then we should
rename all QOM commands to replace "-" to "_". But in this case we can
brake compatibility in possible scripts that already use these commands.
For example, scripts/qmp/qom-fuse

I would leave name qom-type-prop-list with dashes, so it will be consistent
with other two QOM commands and would refactor all QOM commands names if
possible.

Re subcommand of the info command

Also from hmp-command.hx I can see that info command shows various
information about the _system_state_ The qom-type-prop-list shows
properties of the class type. They can be changed during runtime, but I am
not sure if they can be referred as a system state. From another side
command like "info class x86_64-cpu" could take less typing, but this will
be inconsistent with QMP version of the command.

For this reasons I would leave qom-type-prop-list as it is right now.

Daniel have reviewed this patch already but found my error in the
parameters of the HMP command. I have fixed this error and tested command
with monitor.  But would it be ok to add QMP and HMP tests to this patch?
Or may be submit tests with another patch, because this one is already
reviewed? I do not see much QMP/HMP tests so I am hesitating if this is a
good idea.

Thank you,
Valentin






On Fri, Jan 22, 2016 at 10:02 PM, Eric Blake <eblake@redhat.com> wrote:

> On 01/22/2016 05:15 AM, 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>
> > ---
>
> > +++ b/hmp-commands.hx
> > @@ -1734,6 +1734,19 @@ Print QOM properties of object at location
> @var{path}
> >  ETEXI
> >
> >      {
> > +        .name       = "qom-type-prop-list",
>
> To be consistent with most existing HMP commands, this should use
> qmp_type_prop_list (only the QMP version should use '-').
>
> For that matter, should this really be a new top-level command, or
> should it be a subcommand of the 'info' command?
>
> The QMP command looks fine, though.
>
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
>


-- 
Best Regards,
Valentin Rakush.

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

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

* Re: [Qemu-devel] [PATCH v4] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties
  2016-01-22 20:20   ` Valentin Rakush
@ 2016-01-25 17:43     ` Eric Blake
  2016-01-26  9:08       ` Valentin Rakush
  0 siblings, 1 reply; 7+ messages in thread
From: Eric Blake @ 2016-01-25 17:43 UTC (permalink / raw)
  To: Valentin Rakush
  Cc: Markus Armbruster, qemu-devel, lcapitulino, asmetanin,
	Denis V. Lunev, Andreas Färber

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

On 01/22/2016 01:20 PM, Valentin Rakush wrote:
> Hi Eric, hi Daniel,
> 
> Re dashes in the command name
> 
> AFAICC, the QOM related command in HMP use dash "-". For example, qom-list
> and qom-set. If we will change dash "-" to underscore "_" then
> qom_type_prop_list will be consistent with old HMP commands (created before
> year 2013), but will _not_ be consistent with QOM commands created after
> 2013. Which is not nice and may be misleading.

Well, HMP is not set in stone. We can rename the HMP command to qom_list
and qom_set, if we want consistency so that _all_ HMP commands prefer _.

> 
> If we want to have consistent naming of all HMP commands, then we should
> rename all QOM commands to replace "-" to "_". But in this case we can
> brake compatibility in possible scripts that already use these commands.
> For example, scripts/qmp/qom-fuse

Any script using HMP is already broken, because HMP is not designed for
scriptability.  Scripts should be using QMP, and QMP has stricter rules
about not arbitrarily changing names.

> 
> I would leave name qom-type-prop-list with dashes, so it will be consistent
> with other two QOM commands and would refactor all QOM commands names if
> possible.

I'm not asking you to change the QMP name, just the HMP name.

> 
> Re subcommand of the info command
> 
> Also from hmp-command.hx I can see that info command shows various
> information about the _system_state_ The qom-type-prop-list shows
> properties of the class type. They can be changed during runtime, but I am
> not sure if they can be referred as a system state. From another side
> command like "info class x86_64-cpu" could take less typing, but this will
> be inconsistent with QMP version of the command.

We aren't aiming for equivalence between QMP and HMP.  It's fine for the
HMP command to be higher-level, have more smarts, and have more
consistency with other HMP commands.

> 
> For this reasons I would leave qom-type-prop-list as it is right now.

Since HMP is not scriptable, I am not going to hold up the patch on
bikeshedding how the HMP command is spelled.  I just wanted to point out
the difference in conventions, and that you are adding an exception to
the conventions.

> 
> Daniel have reviewed this patch already but found my error in the
> parameters of the HMP command. I have fixed this error and tested command
> with monitor.  But would it be ok to add QMP and HMP tests to this patch?
> Or may be submit tests with another patch, because this one is already
> reviewed? I do not see much QMP/HMP tests so I am hesitating if this is a
> good idea.

Another idea is to add ONLY a QMP command, and omit the functionality
from HMP altogether.  If someone finds the HMP interface important, they
can submit a later patch to add it on top of the QMP command.

-- 
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 --]

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

* Re: [Qemu-devel] [PATCH v4] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties
  2016-01-25 17:43     ` Eric Blake
@ 2016-01-26  9:08       ` Valentin Rakush
  0 siblings, 0 replies; 7+ messages in thread
From: Valentin Rakush @ 2016-01-26  9:08 UTC (permalink / raw)
  To: Eric Blake
  Cc: Markus Armbruster, qemu-devel, lcapitulino, asmetanin,
	Denis V. Lunev, Andreas Färber

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

Hi Eric,

On Mon, Jan 25, 2016 at 8:43 PM, Eric Blake <eblake@redhat.com> wrote:

> On 01/22/2016 01:20 PM, Valentin Rakush wrote:
> > Hi Eric, hi Daniel,
> >
> > Re dashes in the command name
> >
> > AFAICC, the QOM related command in HMP use dash "-". For example,
> qom-list
> > and qom-set. If we will change dash "-" to underscore "_" then
> > qom_type_prop_list will be consistent with old HMP commands (created
> before
> > year 2013), but will _not_ be consistent with QOM commands created after
> > 2013. Which is not nice and may be misleading.
>
> Well, HMP is not set in stone. We can rename the HMP command to qom_list
> and qom_set, if we want consistency so that _all_ HMP commands prefer _.
>
> >
> > If we want to have consistent naming of all HMP commands, then we should
> > rename all QOM commands to replace "-" to "_". But in this case we can
> > brake compatibility in possible scripts that already use these commands.
> > For example, scripts/qmp/qom-fuse
>
> Any script using HMP is already broken, because HMP is not designed for
> scriptability.  Scripts should be using QMP, and QMP has stricter rules
> about not arbitrarily changing names.
>
>
The HMP and QMP has same command qom-list. When I searched for this command
then I found that it is used in scripts (QMP only) and did not look
further. My fault.
Thank you for explanations.


> >
> > I would leave name qom-type-prop-list with dashes, so it will be
> consistent
> > with other two QOM commands and would refactor all QOM commands names if
> > possible.
>
> I'm not asking you to change the QMP name, just the HMP name.
>
> >
> > Re subcommand of the info command
> >
> > Also from hmp-command.hx I can see that info command shows various
> > information about the _system_state_ The qom-type-prop-list shows
> > properties of the class type. They can be changed during runtime, but I
> am
> > not sure if they can be referred as a system state. From another side
> > command like "info class x86_64-cpu" could take less typing, but this
> will
> > be inconsistent with QMP version of the command.
>
> We aren't aiming for equivalence between QMP and HMP.  It's fine for the
> HMP command to be higher-level, have more smarts, and have more
> consistency with other HMP commands.
>
> >
> > For this reasons I would leave qom-type-prop-list as it is right now.
>
> Since HMP is not scriptable, I am not going to hold up the patch on
> bikeshedding how the HMP command is spelled.  I just wanted to point out
> the difference in conventions, and that you are adding an exception to
> the conventions.
>

The qom-list breaks this conventions and this is why I did mistake when
implemented qom-type-prop-list.


> >
> > Daniel have reviewed this patch already but found my error in the
> > parameters of the HMP command. I have fixed this error and tested command
> > with monitor.  But would it be ok to add QMP and HMP tests to this patch?
> > Or may be submit tests with another patch, because this one is already
> > reviewed? I do not see much QMP/HMP tests so I am hesitating if this is a
> > good idea.
>
> Another idea is to add ONLY a QMP command, and omit the functionality
> from HMP altogether.  If someone finds the HMP interface important, they
> can submit a later patch to add it on top of the QMP command.
>

There were no requirements to add HMP command, but I was implementing
qom-type-prop-list based on the qom-list (as an example). The qom-list
command
has QMP and HMP commands which spells similar and this is a reason why
I implemented HMP version of the qom-type-prop-list.

Because there were no such requirement to add HMP qom-type-prop-list
command
I think it would be better to remove HMP version of qom-type-prop-list. I
will do this in
next version of this patch.

Thank you.


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


-- 
Best Regards,
Valentin Rakush.

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

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

end of thread, other threads:[~2016-01-26  9:08 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-22 12:15 [Qemu-devel] [PATCH v4] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties Valentin Rakush
2016-01-22 13:00 ` Daniel P. Berrange
2016-01-22 13:16 ` Denis V. Lunev
2016-01-22 19:02 ` Eric Blake
2016-01-22 20:20   ` Valentin Rakush
2016-01-25 17:43     ` Eric Blake
2016-01-26  9:08       ` Valentin Rakush

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