[Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

At present, people have no way to know they should
have a specific format for alias properties.

 Example:

before output:

virtio-blk-pci.physical_block_size=uint16
virtio-blk-pci.logical_block_size=uint16
virtio-blk-pci.drive=str

after output applied this patch series:

virtio-blk-pci.physical_block_size=blocksize
virtio-blk-pci.logical_block_size=blocksize
virtio-blk-pci.drive=drive

Gonglei (3):
  qom: add error handler for object alias property
  qom: add target object poniter for alias property in ObjectProperty
  qmp: print real legacy_name for alias property

 include/qom/object.h |  3 +++
 qmp.c                | 68 ++++++++++++++++++++++++++++++++++++----------------
 qom/object.c         | 10 +++++++-
 3 files changed, 59 insertions(+), 22 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH 1/3] qom: add error handler for object alias property

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

object_property_add_alias() is called at some
places at present. And its parameter errp may not NULL,
such as
 object_property_add_alias(obj, "iothread", OBJECT(&dev->vdev),"iothread",
                              &error_abort);
This patch add error handler for security.

Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 qom/object.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/qom/object.c b/qom/object.c
index da0919a..e7b16a1 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1634,6 +1634,7 @@ void object_property_add_alias(Object *obj, const char *name,
     ObjectProperty *op;
     ObjectProperty *target_prop;
     gchar *prop_type;
+    Error *local_err = NULL;
 
     target_prop = object_property_find(target_obj, target_name, errp);
     if (!target_prop) {
@@ -1655,9 +1656,14 @@ void object_property_add_alias(Object *obj, const char *name,
                              property_get_alias,
                              property_set_alias,
                              property_release_alias,
-                             prop, errp);
+                             prop, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        goto out;
+    }
     op->resolve = property_resolve_alias;
 
+out:
     g_free(prop_type);
 }
 
-- 
1.7.12.4

[Qemu-devel] [PATCH 2/3] qom: add target object poniter for alias property in ObjectProperty

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

In this way, we can use target object and get its qdev
property legacy_name etc.

Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 include/qom/object.h | 3 +++
 qom/object.c         | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index 8a05a81..b8113cb 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -344,6 +344,9 @@ typedef struct ObjectProperty
     ObjectPropertyRelease *release;
     void *opaque;
 
+    bool is_alias;
+    Object *target_obj;
+
     QTAILQ_ENTRY(ObjectProperty) node;
 } ObjectProperty;
 
diff --git a/qom/object.c b/qom/object.c
index e7b16a1..5e919b8 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1662,6 +1662,8 @@ void object_property_add_alias(Object *obj, const char *name,
         goto out;
     }
     op->resolve = property_resolve_alias;
+    op->is_alias = true;
+    op->target_obj = target_obj;
 
 out:
     g_free(prop_type);
-- 
1.7.12.4

[Qemu-devel] [PATCH 3/3] qmp: print real legacy_name for alias property

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

In this way, we can get the real legacy_name for
alias properties, and avoide confusing people because of
property type. People have no way to know they should
have a specific format.

 Example:

before output:

virtio-blk-pci.physical_block_size=uint16
virtio-blk-pci.logical_block_size=uint16
virtio-blk-pci.drive=str

after output applied this patch:

virtio-blk-pci.physical_block_size=blocksize
virtio-blk-pci.logical_block_size=blocksize
virtio-blk-pci.drive=drive

Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 qmp.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 47 insertions(+), 21 deletions(-)

diff --git a/qmp.c b/qmp.c
index c6767c4..9338bd4 100644
--- a/qmp.c
+++ b/qmp.c
@@ -433,6 +433,40 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements,
     return ret;
 }
 
+static bool find_device_property_info(ObjectClass *klass,
+                                      const char *name,
+                                      DevicePropertyInfo **info)
+{
+    Property *prop;
+    bool found = false;
+
+    for (prop = DEVICE_CLASS(klass)->props; prop && prop->name; prop++) {
+        if (strcmp(name, prop->name) != 0) {
+            continue;
+        }
+
+        /*
+         * TODO Properties without a parser are just for dirty hacks.
+         * qdev_prop_ptr is the only such PropertyInfo.  It's marked
+         * for removal.  This conditional should be removed along with
+         * it.
+         */
+        if (!prop->info->set) {
+            found = true;
+            goto out;           /* no way to set it, don't show */
+        }
+
+        *info = g_malloc0(sizeof(**info));
+        (*info)->name = g_strdup(prop->name);
+        (*info)->type = g_strdup(prop->info->legacy_name ?: prop->info->name);
+
+        found = true;
+        goto out;
+    }
+out:
+    return found;
+}
+
 /* Return a DevicePropertyInfo for a qdev property.
  *
  * If a qdev property with the given name does not exist, use the given default
@@ -442,30 +476,21 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements,
  */
 static DevicePropertyInfo *make_device_property_info(ObjectClass *klass,
                                                      const char *name,
-                                                     const char *default_type)
+                                                     const char *default_type,
+                                                     bool is_alias,
+                                                     Object *target_obj)
 {
     DevicePropertyInfo *info;
-    Property *prop;
-
-    do {
-        for (prop = DEVICE_CLASS(klass)->props; prop && prop->name; prop++) {
-            if (strcmp(name, prop->name) != 0) {
-                continue;
-            }
 
-            /*
-             * TODO Properties without a parser are just for dirty hacks.
-             * qdev_prop_ptr is the only such PropertyInfo.  It's marked
-             * for removal.  This conditional should be removed along with
-             * it.
-             */
-            if (!prop->info->set) {
-                return NULL;           /* no way to set it, don't show */
-            }
+    if (is_alias) {
+        ObjectClass *class = object_get_class(target_obj);
+        if (find_device_property_info(class, name, &info)) {
+            return info;
+        }
+    }
 
-            info = g_malloc0(sizeof(*info));
-            info->name = g_strdup(prop->name);
-            info->type = g_strdup(prop->info->legacy_name ?: prop->info->name);
+    do {
+        if (find_device_property_info(klass, name, &info)) {
             return info;
         }
         klass = object_class_get_parent(klass);
@@ -521,7 +546,8 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename,
             continue;
         }
 
-        info = make_device_property_info(klass, prop->name, prop->type);
+        info = make_device_property_info(klass, prop->name, prop->type,
+                                         prop->is_alias, prop->target_obj);
         if (!info) {
             continue;
         }
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 1/3] qom: add error handler for object alias property

[Paolo Bonzini]

Il 15/09/2014 16:44, arei.gonglei@huawei.com ha scritto:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> object_property_add_alias() is called at some
> places at present. And its parameter errp may not NULL,
> such as
>  object_property_add_alias(obj, "iothread", OBJECT(&dev->vdev),"iothread",
>                               &error_abort);
> This patch add error handler for security.
> 
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  qom/object.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/qom/object.c b/qom/object.c
> index da0919a..e7b16a1 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1634,6 +1634,7 @@ void object_property_add_alias(Object *obj, const char *name,
>      ObjectProperty *op;
>      ObjectProperty *target_prop;
>      gchar *prop_type;
> +    Error *local_err = NULL;
>  
>      target_prop = object_property_find(target_obj, target_name, errp);
>      if (!target_prop) {
> @@ -1655,9 +1656,14 @@ void object_property_add_alias(Object *obj, const char *name,
>                               property_get_alias,
>                               property_set_alias,
>                               property_release_alias,
> -                             prop, errp);
> +                             prop, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        goto out;
> +    }
>      op->resolve = property_resolve_alias;
>  
> +out:
>      g_free(prop_type);
>  }
>  
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Paolo Bonzini]

Il 15/09/2014 16:44, arei.gonglei@huawei.com ha scritto:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> At present, people have no way to know they should
> have a specific format for alias properties.
> 
>  Example:
> 
> before output:
> 
> virtio-blk-pci.physical_block_size=uint16
> virtio-blk-pci.logical_block_size=uint16
> virtio-blk-pci.drive=str
> 
> after output applied this patch series:
> 
> virtio-blk-pci.physical_block_size=blocksize
> virtio-blk-pci.logical_block_size=blocksize
> virtio-blk-pci.drive=drive

Is there actually anyone that is relying on the legacy_names of
properties?  I would be inclined to drop them altogether...

Paolo

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Eric Blake]

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

On 09/15/2014 09:57 AM, Paolo Bonzini wrote:
> Il 15/09/2014 16:44, arei.gonglei@huawei.com ha scritto:
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> At present, people have no way to know they should
>> have a specific format for alias properties.
>>
>>  Example:
>>
>> before output:
>>
>> virtio-blk-pci.physical_block_size=uint16
>> virtio-blk-pci.logical_block_size=uint16
>> virtio-blk-pci.drive=str
>>
>> after output applied this patch series:
>>
>> virtio-blk-pci.physical_block_size=blocksize
>> virtio-blk-pci.logical_block_size=blocksize
>> virtio-blk-pci.drive=drive
> 
> Is there actually anyone that is relying on the legacy_names of
> properties?  I would be inclined to drop them altogether...

Libvirt does not appear to be relying on legacy names, but that's not
conclusive evidence in favor of removing...

-- 
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: 539 bytes --]

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Michael S. Tsirkin]

On Mon, Sep 15, 2014 at 05:57:51PM +0200, Paolo Bonzini wrote:
> Il 15/09/2014 16:44, arei.gonglei@huawei.com ha scritto:
> > From: Gonglei <arei.gonglei@huawei.com>
> > 
> > At present, people have no way to know they should
> > have a specific format for alias properties.
> > 
> >  Example:
> > 
> > before output:
> > 
> > virtio-blk-pci.physical_block_size=uint16
> > virtio-blk-pci.logical_block_size=uint16
> > virtio-blk-pci.drive=str
> > 
> > after output applied this patch series:
> > 
> > virtio-blk-pci.physical_block_size=blocksize
> > virtio-blk-pci.logical_block_size=blocksize
> > virtio-blk-pci.drive=drive
> 
> Is there actually anyone that is relying on the legacy_names of
> properties?  I would be inclined to drop them altogether...
> 
> Paolo

What makes these legacy?
How do you expect users to find out about the properties?
>qemu -device virtio-blk-pci.?
is not the correct way anymore?

How the time flies, it seems only yesterday this was the
new way, -help was the old one ...

-- 
MST

Re: [Qemu-devel] [PATCH 3/3] qmp: print real legacy_name for alias property

[Michael S. Tsirkin]

On Mon, Sep 15, 2014 at 10:44:39PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> In this way, we can get the real legacy_name for
> alias properties, and avoide confusing people because of
> property type. People have no way to know they should
> have a specific format.
> 
>  Example:
> 
> before output:
> 
> virtio-blk-pci.physical_block_size=uint16
> virtio-blk-pci.logical_block_size=uint16
> virtio-blk-pci.drive=str
> 
> after output applied this patch:
> 
> virtio-blk-pci.physical_block_size=blocksize
> virtio-blk-pci.logical_block_size=blocksize
> virtio-blk-pci.drive=drive
> 
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  qmp.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 47 insertions(+), 21 deletions(-)
> 
> diff --git a/qmp.c b/qmp.c
> index c6767c4..9338bd4 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -433,6 +433,40 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements,
>      return ret;
>  }
>  
> +static bool find_device_property_info(ObjectClass *klass,
> +                                      const char *name,
> +                                      DevicePropertyInfo **info)
> +{
> +    Property *prop;
> +    bool found = false;
> +
> +    for (prop = DEVICE_CLASS(klass)->props; prop && prop->name; prop++) {
> +        if (strcmp(name, prop->name) != 0) {
> +            continue;
> +        }
> +
> +        /*
> +         * TODO Properties without a parser are just for dirty hacks.
> +         * qdev_prop_ptr is the only such PropertyInfo.  It's marked
> +         * for removal.  This conditional should be removed along with
> +         * it.
> +         */
> +        if (!prop->info->set) {
> +            found = true;
> +            goto out;           /* no way to set it, don't show */
> +        }
> +
> +        *info = g_malloc0(sizeof(**info));
> +        (*info)->name = g_strdup(prop->name);
> +        (*info)->type = g_strdup(prop->info->legacy_name ?: prop->info->name);
> +
> +        found = true;
> +        goto out;
> +    }
> +out:
> +    return found;
> +}
> +
>  /* Return a DevicePropertyInfo for a qdev property.
>   *
>   * If a qdev property with the given name does not exist, use the given default
> @@ -442,30 +476,21 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements,
>   */
>  static DevicePropertyInfo *make_device_property_info(ObjectClass *klass,
>                                                       const char *name,
> -                                                     const char *default_type)
> +                                                     const char *default_type,
> +                                                     bool is_alias,
> +                                                     Object *target_obj)
>  {
>      DevicePropertyInfo *info;
> -    Property *prop;
> -
> -    do {
> -        for (prop = DEVICE_CLASS(klass)->props; prop && prop->name; prop++) {
> -            if (strcmp(name, prop->name) != 0) {
> -                continue;
> -            }
>  
> -            /*
> -             * TODO Properties without a parser are just for dirty hacks.
> -             * qdev_prop_ptr is the only such PropertyInfo.  It's marked
> -             * for removal.  This conditional should be removed along with
> -             * it.
> -             */
> -            if (!prop->info->set) {
> -                return NULL;           /* no way to set it, don't show */
> -            }
> +    if (is_alias) {
> +        ObjectClass *class = object_get_class(target_obj);
> +        if (find_device_property_info(class, name, &info)) {
> +            return info;
> +        }
> +    }
>  
> -            info = g_malloc0(sizeof(*info));
> -            info->name = g_strdup(prop->name);
> -            info->type = g_strdup(prop->info->legacy_name ?: prop->info->name);
> +    do {
> +        if (find_device_property_info(klass, name, &info)) {
>              return info;
>          }
>          klass = object_class_get_parent(klass);
> @@ -521,7 +546,8 @@ DevicePropertyInfoList *qmp_device_list_properties(const char *typename,
>              continue;
>          }
>  
> -        info = make_device_property_info(klass, prop->name, prop->type);
> +        info = make_device_property_info(klass, prop->name, prop->type,
> +                                         prop->is_alias, prop->target_obj);
>          if (!info) {
>              continue;
>          }
> -- 
> 1.7.12.4
> 

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Michael S. Tsirkin]

On Mon, Sep 15, 2014 at 10:44:36PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> At present, people have no way to know they should
> have a specific format for alias properties.
> 
>  Example:
> 
> before output:
> 
> virtio-blk-pci.physical_block_size=uint16
> virtio-blk-pci.logical_block_size=uint16
> virtio-blk-pci.drive=str
> 
> after output applied this patch series:
> 
> virtio-blk-pci.physical_block_size=blocksize
> virtio-blk-pci.logical_block_size=blocksize
> virtio-blk-pci.drive=drive
> 
> Gonglei (3):
>   qom: add error handler for object alias property
>   qom: add target object poniter for alias property in ObjectProperty
>   qmp: print real legacy_name for alias property
> 
>  include/qom/object.h |  3 +++
>  qmp.c                | 68 ++++++++++++++++++++++++++++++++++++----------------
>  qom/object.c         | 10 +++++++-
>  3 files changed, 59 insertions(+), 22 deletions(-)


OK virtio patches depend on this one, so I guess I'll
have to merge this in my tree ...
Andreas can you ACK that please?

> -- 
> 1.7.12.4
> 

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Michael S. Tsirkin]

On Mon, Sep 15, 2014 at 10:44:36PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> At present, people have no way to know they should
> have a specific format for alias properties.
> 
>  Example:
> 
> before output:
> 
> virtio-blk-pci.physical_block_size=uint16
> virtio-blk-pci.logical_block_size=uint16
> virtio-blk-pci.drive=str
> 
> after output applied this patch series:
> 
> virtio-blk-pci.physical_block_size=blocksize
> virtio-blk-pci.logical_block_size=blocksize
> virtio-blk-pci.drive=drive
> 
> Gonglei (3):
>   qom: add error handler for object alias property
>   qom: add target object poniter for alias property in ObjectProperty
>   qmp: print real legacy_name for alias property
> 
>  include/qom/object.h |  3 +++
>  qmp.c                | 68 ++++++++++++++++++++++++++++++++++++----------------
>  qom/object.c         | 10 +++++++-
>  3 files changed, 59 insertions(+), 22 deletions(-)

fixes regression in 2.1 so 2.1.2 material?

> -- 
> 1.7.12.4
> 

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Gonglei (Arei)]

> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Subject: Re: [PATCH 0/3] Fix confused output for alias properties
> 
> On Mon, Sep 15, 2014 at 10:44:36PM +0800, arei.gonglei@huawei.com wrote:
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > At present, people have no way to know they should
> > have a specific format for alias properties.
> >
> >  Example:
> >
> > before output:
> >
> > virtio-blk-pci.physical_block_size=uint16
> > virtio-blk-pci.logical_block_size=uint16
> > virtio-blk-pci.drive=str
> >
> > after output applied this patch series:
> >
> > virtio-blk-pci.physical_block_size=blocksize
> > virtio-blk-pci.logical_block_size=blocksize
> > virtio-blk-pci.drive=drive
> >
> > Gonglei (3):
> >   qom: add error handler for object alias property
> >   qom: add target object poniter for alias property in ObjectProperty
> >   qmp: print real legacy_name for alias property
> >
> >  include/qom/object.h |  3 +++
> >  qmp.c                | 68
> ++++++++++++++++++++++++++++++++++++----------------
> >  qom/object.c         | 10 +++++++-
> >  3 files changed, 59 insertions(+), 22 deletions(-)
> 
> fixes regression in 2.1 so 2.1.2 material?
> 
Yes. I think it should be. :)

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Mon, Sep 15, 2014 at 05:57:51PM +0200, Paolo Bonzini wrote:
>> Il 15/09/2014 16:44, arei.gonglei@huawei.com ha scritto:
>> > From: Gonglei <arei.gonglei@huawei.com>
>> > 
>> > At present, people have no way to know they should
>> > have a specific format for alias properties.
>> > 
>> >  Example:
>> > 
>> > before output:
>> > 
>> > virtio-blk-pci.physical_block_size=uint16
>> > virtio-blk-pci.logical_block_size=uint16
>> > virtio-blk-pci.drive=str
>> > 
>> > after output applied this patch series:
>> > 
>> > virtio-blk-pci.physical_block_size=blocksize
>> > virtio-blk-pci.logical_block_size=blocksize
>> > virtio-blk-pci.drive=drive
>> 
>> Is there actually anyone that is relying on the legacy_names of
>> properties?  I would be inclined to drop them altogether...
>> 
>> Paolo
>
> What makes these legacy?

Rebasing qdev on top of QOM produced a bit of cruft.

PropertyInfo defines a kind of property.  It used to look like this:

    struct PropertyInfo {
        const char *name;
        size_t size;
        enum PropertyType type;
        int (*parse)(DeviceState *dev, Property *prop, const char *str);
        int (*print)(DeviceState *dev, Property *prop, char *dest, size_t len);
    };

Lets generic code parse, unparse and copy properties.

Commit 922910c added the -device FOO,? feature, using member name to
give a hint on possible property values.

The rebase onto QOM renamed name to legacy_name, to free name for use as
QOM type name (commit cafe5bd).

Current code uses legacy_name as follows:

* QMP command device-list-properties

  Returns a list of DevicePropertyInfo, whose member "type" is
  PropertyInfo's legacy_name if set, else its name.

* -device FOO,help

  Prints FOO.NAME=TYPE, where NAME is the property's name (not to be
  confused with PropertyInfo's name), and TYPE is PropertyInfo's legacy
  name if set, else its name.

  -device FOO,? is an outmoded way to say -device FOO,help.  No plans to
  drop support for it.

The second is actually built on top of the first.

> How do you expect users to find out about the properties?
>>qemu -device virtio-blk-pci.?
> is not the correct way anymore?

As far as I can tell, libvirt uses both device-list-properties and
"-device,?".  It doesn't appear to use the returned "type".

Human users do, however.  I'd object to a degradation of -device
FOO,help.  Changing it is fine, but it should remain at least as helpful
as it is now.

> How the time flies, it seems only yesterday this was the
> new way, -help was the old one ...

Everything must change, so that everything can stay the same.

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Gonglei (Arei)]

> From: Markus Armbruster [mailto:armbru@redhat.com]
> Sent: Tuesday, September 16, 2014 3:21 PM
> To: Michael S. Tsirkin
> Subject: Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
> 
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Mon, Sep 15, 2014 at 05:57:51PM +0200, Paolo Bonzini wrote:
> >> Il 15/09/2014 16:44, arei.gonglei@huawei.com ha scritto:
> >> > From: Gonglei <arei.gonglei@huawei.com>
> >> >
> >> > At present, people have no way to know they should
> >> > have a specific format for alias properties.
> >> >
> >> >  Example:
> >> >
> >> > before output:
> >> >
> >> > virtio-blk-pci.physical_block_size=uint16
> >> > virtio-blk-pci.logical_block_size=uint16
> >> > virtio-blk-pci.drive=str
> >> >
> >> > after output applied this patch series:
> >> >
> >> > virtio-blk-pci.physical_block_size=blocksize
> >> > virtio-blk-pci.logical_block_size=blocksize
> >> > virtio-blk-pci.drive=drive
> >>
> >> Is there actually anyone that is relying on the legacy_names of
> >> properties?  I would be inclined to drop them altogether...
> >>
> >> Paolo
> >
> > What makes these legacy?
> 
> Rebasing qdev on top of QOM produced a bit of cruft.
> 
> PropertyInfo defines a kind of property.  It used to look like this:
> 
>     struct PropertyInfo {
>         const char *name;
>         size_t size;
>         enum PropertyType type;
>         int (*parse)(DeviceState *dev, Property *prop, const char *str);
>         int (*print)(DeviceState *dev, Property *prop, char *dest, size_t len);
>     };
> 
> Lets generic code parse, unparse and copy properties.
> 
> Commit 922910c added the -device FOO,? feature, using member name to
> give a hint on possible property values.
> 
> The rebase onto QOM renamed name to legacy_name, to free name for use as
> QOM type name (commit cafe5bd).
> 
> Current code uses legacy_name as follows:
> 
> * QMP command device-list-properties
> 
>   Returns a list of DevicePropertyInfo, whose member "type" is
>   PropertyInfo's legacy_name if set, else its name.
> 
> * -device FOO,help
> 
>   Prints FOO.NAME=TYPE, where NAME is the property's name (not to be
>   confused with PropertyInfo's name), and TYPE is PropertyInfo's legacy
>   name if set, else its name.
> 
>   -device FOO,? is an outmoded way to say -device FOO,help.  No plans to
>   drop support for it.
> 
> The second is actually built on top of the first.
> 
> > How do you expect users to find out about the properties?
> >>qemu -device virtio-blk-pci.?
> > is not the correct way anymore?
> 
> As far as I can tell, libvirt uses both device-list-properties and
> "-device,?".  It doesn't appear to use the returned "type".
> 
> Human users do, however.  I'd object to a degradation of -device
> FOO,help.  Changing it is fine, but it should remain at least as helpful
> as it is now.
> 
> > How the time flies, it seems only yesterday this was the
> > new way, -help was the old one ...
> 
> Everything must change, so that everything can stay the same.

What detailed explanation it is! Thanks. :)

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Paolo Bonzini]

Il 16/09/2014 09:21, Markus Armbruster ha scritto:
> The rebase onto QOM renamed name to legacy_name, to free name for use as
> QOM type name (commit cafe5bd).

Also, the QOM type name has strict rules:

- either it is a QAPI type (primitive, enum or struct)

- or it is link<qom-type-name>

- or it is child<qom-type-name>

The qdev type names had no rules.  We had uint8, hex8, on/off, drive, etc.

> Human users do, however.  I'd object to a degradation of -device
> FOO,help.  Changing it is fine, but it should remain at least as helpful
> as it is now.

The question is if it is really a degradation.

Example 1:

virtio-blk-pci.physical_block_size=blocksize
virtio-blk-pci.logical_block_size=blocksize

vs.

virtio-blk-pci.physical_block_size=uint16
virtio-blk-pci.logical_block_size=uint16

What is a "blocksize"?  It is a power of two between 512 and 32768, but 
how does the user know?  If the value is too small or invalid, the 
error message is particularly helpful for QEMU standards:

    qemu-system-x86_64: -device virtio-blk-pci,physical_block_size=128,drive=hd:
    Property .physical_block_size doesn't take value 128 (minimum: 512, maximum: 
    32768)

    qemu-system-x86_64: -device virtio-blk-pci,physical_block_size=1023,drive=hd:
    Property .physical_block_size doesn't take value '1023', it's not a power of 2

I think uint16 is actually more informative than "blocksize".



Example 2:

    virtio-blk-pci.drive=drive

vs.

    virtio-blk-pci.drive=str

The fact that it points to a -drive is already guessable (for anyone who
knows the relationship between -drive and -device) from the name of the
property.

    $ qemu-system-x86_64 -drive if=none,file=$HOME/test2.img,id=hd -device virtio-blk-pci
    qemu-system-x86_64: -device virtio-blk-pci: drive property not set
    qemu-system-x86_64: -device virtio-blk-pci: Device initialization failed.
    qemu-system-x86_64: -device virtio-blk-pci: Device 'virtio-blk-pci' could not be 
    initialized

    $ qemu-system-x86_64 -drive if=none,file=$HOME/test2.img,id=hd
    -device virtio-blk-pci,drive=ff
    qemu-system-x86_64: -device virtio-blk-pci,drive=ff: Property
    'virtio-blk-device.drive' can't find value 'ff'

If we QOMified BlockBackend, BTW, it would show up as

    virtio-blk-pci.drive=link<block-backend>

I think both "str" and "link<block-backend>" actually are a small degradation
compared to "drive", and this is why I kept the legacy_name.  But overall I
think it's not really worth the layering violation that patches 2 and 3 are;
and it's definitely not stable material.

I'd rather drop the legacy_name at all.  Here are the legacy_names currently
in use:

    hw/core/qdev-properties-system.c:    .legacy_name  = "drive",
    hw/core/qdev-properties-system.c:    .legacy_name  = "chr",
    hw/core/qdev-properties-system.c:    .legacy_name  = "netdev",
    hw/core/qdev-properties-system.c:    .legacy_name  = "vlan",
    hw/core/qdev-properties.c:    .legacy_name  = "on/off",
    hw/core/qdev-properties.c:    .legacy_name  = "macaddr",
    hw/core/qdev-properties.c:    .legacy_name = "bios-chs-trans",
    hw/core/qdev-properties.c:    .legacy_name  = "pci-devfn",
    hw/core/qdev-properties.c:    .legacy_name  = "blocksize",
    hw/core/qdev-properties.c:    .legacy_name = "pci-host-devaddr",

vlan is just a glorified int, not an id like the others.  chr should be
named chardev.  blocksize, I already covered above.  bios-chs-trans is
an enum (QAPI name BiosAtaTranslation) and is useless.  on/off vs.
bool is just bikeshedding.  macaddr is obviously a string, whose format
is clear from the property name.

pci-host-devaddr and pci-devfn are the only ones that do not have an
obvious property name (respectively "host" and "addr").

Paolo

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Paolo Bonzini]

Il 15/09/2014 19:49, Michael S. Tsirkin ha scritto:
> On Mon, Sep 15, 2014 at 10:44:36PM +0800, arei.gonglei@huawei.com wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> At present, people have no way to know they should
>> have a specific format for alias properties.
>>
>>  Example:
>>
>> before output:
>>
>> virtio-blk-pci.physical_block_size=uint16
>> virtio-blk-pci.logical_block_size=uint16
>> virtio-blk-pci.drive=str
>>
>> after output applied this patch series:
>>
>> virtio-blk-pci.physical_block_size=blocksize
>> virtio-blk-pci.logical_block_size=blocksize
>> virtio-blk-pci.drive=drive
>>
>> Gonglei (3):
>>   qom: add error handler for object alias property
>>   qom: add target object poniter for alias property in ObjectProperty
>>   qmp: print real legacy_name for alias property
>>
>>  include/qom/object.h |  3 +++
>>  qmp.c                | 68 ++++++++++++++++++++++++++++++++++++----------------
>>  qom/object.c         | 10 +++++++-
>>  3 files changed, 59 insertions(+), 22 deletions(-)
> 
> fixes regression in 2.1 so 2.1.2 material?

Given that there's no known breakage, I don't think so.

I'd rather get rid completely of the legacy_name.

Paolo

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 16/09/2014 09:21, Markus Armbruster ha scritto:
>> The rebase onto QOM renamed name to legacy_name, to free name for use as
>> QOM type name (commit cafe5bd).
>
> Also, the QOM type name has strict rules:
>
> - either it is a QAPI type (primitive, enum or struct)
>
> - or it is link<qom-type-name>
>
> - or it is child<qom-type-name>
>
> The qdev type names had no rules.  We had uint8, hex8, on/off, drive, etc.
>
>> Human users do, however.  I'd object to a degradation of -device
>> FOO,help.  Changing it is fine, but it should remain at least as helpful
>> as it is now.
>
> The question is if it is really a degradation.
>
> Example 1:
>
> virtio-blk-pci.physical_block_size=blocksize
> virtio-blk-pci.logical_block_size=blocksize
>
> vs.
>
> virtio-blk-pci.physical_block_size=uint16
> virtio-blk-pci.logical_block_size=uint16
>
> What is a "blocksize"?  It is a power of two between 512 and 32768, but 
> how does the user know?  If the value is too small or invalid, the 
> error message is particularly helpful for QEMU standards:
>
>     qemu-system-x86_64: -device virtio-blk-pci,physical_block_size=128,drive=hd:
>     Property .physical_block_size doesn't take value 128 (minimum:
> 512, maximum:
>     32768)
>
>     qemu-system-x86_64: -device
> virtio-blk-pci,physical_block_size=1023,drive=hd:
>     Property .physical_block_size doesn't take value '1023', it's not
> a power of 2
>
> I think uint16 is actually more informative than "blocksize".

Neither is particularly user-friendly, but I grant you uint16 is less
bad than blocksize in absence of definitions for these terms.

> Example 2:
>
>     virtio-blk-pci.drive=drive
>
> vs.
>
>     virtio-blk-pci.drive=str
>
> The fact that it points to a -drive is already guessable (for anyone who
> knows the relationship between -drive and -device) from the name of the
> property.
>
>     $ qemu-system-x86_64 -drive if=none,file=$HOME/test2.img,id=hd
> -device virtio-blk-pci
>     qemu-system-x86_64: -device virtio-blk-pci: drive property not set
>     qemu-system-x86_64: -device virtio-blk-pci: Device initialization failed.
>     qemu-system-x86_64: -device virtio-blk-pci: Device
> virtio-blk-pci' could not be
>     initialized
>
>     $ qemu-system-x86_64 -drive if=none,file=$HOME/test2.img,id=hd
>     -device virtio-blk-pci,drive=ff
>     qemu-system-x86_64: -device virtio-blk-pci,drive=ff: Property
>     'virtio-blk-device.drive' can't find value 'ff'
>
> If we QOMified BlockBackend, BTW, it would show up as
>
>     virtio-blk-pci.drive=link<block-backend>
>
> I think both "str" and "link<block-backend>" actually are a small degradation
> compared to "drive", and this is why I kept the legacy_name.  But overall I
> think it's not really worth the layering violation that patches 2 and 3 are;
> and it's definitely not stable material.

"str" is clearly a degradation for me.  I breaks usage like

    for i in `qemu -device help 2>&1 | sed -n 's/^name "\([^"]*\)".*/\1/p'`
    do qemu -device $i,help 2>&1
    done | grep =drive

Finds all block device models.  I've done such things many times.

Whether "link<block-backend>" is a degradation or an improvement is
debatable.

> I'd rather drop the legacy_name at all.  Here are the legacy_names currently
> in use:
>
>     hw/core/qdev-properties-system.c:    .legacy_name  = "drive",
>     hw/core/qdev-properties-system.c:    .legacy_name  = "chr",
>     hw/core/qdev-properties-system.c:    .legacy_name  = "netdev",
>     hw/core/qdev-properties-system.c:    .legacy_name  = "vlan",
>     hw/core/qdev-properties.c:    .legacy_name  = "on/off",
>     hw/core/qdev-properties.c:    .legacy_name  = "macaddr",
>     hw/core/qdev-properties.c:    .legacy_name = "bios-chs-trans",
>     hw/core/qdev-properties.c:    .legacy_name  = "pci-devfn",
>     hw/core/qdev-properties.c:    .legacy_name  = "blocksize",
>     hw/core/qdev-properties.c:    .legacy_name = "pci-host-devaddr",

You missed

    target-ppc/translate_init.c:8047:    .legacy_name = "powerpc-server-compat",

which is another enum.  Aside: it should really use the infrastructure
for enums.

>
> vlan is just a glorified int, not an id like the others.  chr should be
> named chardev.  blocksize, I already covered above.  bios-chs-trans is
> an enum (QAPI name BiosAtaTranslation) and is useless.  on/off vs.
> bool is just bikeshedding.  macaddr is obviously a string, whose format
> is clear from the property name.
>
> pci-host-devaddr and pci-devfn are the only ones that do not have an
> obvious property name (respectively "host" and "addr").

Agree on the uselessness of "on/off".

Agree on the uselessness of "blocksize" without a definition of the
term.

"chr" and "netdev" are like "drive", and replacing them by "str" is a
degradation in my book.

Help for enum-valued properties in the form of "prop=ENUM-NAME" is not
really helpful without a definition of ENUM-NAME.  It's still useful for
finding devices with this kind of property.

Same for structured strings like macaddr, pci-devfn, pci-host-devaddr.

legacy_name provides lousy help.  But it's better than nothing, and I'd
hate to see it dropped without a suitable replacement.

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Paolo Bonzini]

Il 16/09/2014 11:16, Markus Armbruster ha scritto:
>> I think both "str" and "link<block-backend>" actually are a small degradation
>> compared to "drive", and this is why I kept the legacy_name.  But overall I
>> think it's not really worth the layering violation that patches 2 and 3 are;
>> and it's definitely not stable material.
> 
> "str" is clearly a degradation for me.  I breaks usage like
> 
>     for i in `qemu -device help 2>&1 | sed -n 's/^name "\([^"]*\)".*/\1/p'`
>     do qemu -device $i,help 2>&1
>     done | grep =drive
> 
> Finds all block device models.  I've done such things many times.

Just replace "grep =drive" with "fgrep .drive".  Similarly:

  =chr     -> .chardev  (bonus: matches the command line option)
  =netdev  -> .netdev
  =vlan    -> .vlan
  =macaddr -> .mac

We probably agree that having "=drive" work sometimes, but not always,
is the worst of both worlds.  Still doesn't make it stable material, IMO.

> Agree on the uselessness of "on/off".
> 
> Agree on the uselessness of "blocksize" without a definition of the
> term.
> 
> "chr" and "netdev" are like "drive", and replacing them by "str" is a
> degradation in my book.

It is, but we're suprisingly consistent in the naming of such
special-typed properties.  So it's actually a good thing that
legacy_name provides redundant information.

> Help for enum-valued properties in the form of "prop=ENUM-NAME" is not
> really helpful without a definition of ENUM-NAME.  It's still useful for
> finding devices with this kind of property.

Yes, but here you wouldn't have 'str', you would have the corresponding
QAPI enum name.  This would be an improvement, though a minor one.

Paolo

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Michael S. Tsirkin]

On Tue, Sep 16, 2014 at 11:34:07AM +0200, Paolo Bonzini wrote:
> Il 16/09/2014 11:16, Markus Armbruster ha scritto:
> >> I think both "str" and "link<block-backend>" actually are a small degradation
> >> compared to "drive", and this is why I kept the legacy_name.  But overall I
> >> think it's not really worth the layering violation that patches 2 and 3 are;
> >> and it's definitely not stable material.
> > 
> > "str" is clearly a degradation for me.  I breaks usage like
> > 
> >     for i in `qemu -device help 2>&1 | sed -n 's/^name "\([^"]*\)".*/\1/p'`
> >     do qemu -device $i,help 2>&1
> >     done | grep =drive
> > 
> > Finds all block device models.  I've done such things many times.
> 
> Just replace "grep =drive" with "fgrep .drive".  Similarly:
> 
>   =chr     -> .chardev  (bonus: matches the command line option)
>   =netdev  -> .netdev
>   =vlan    -> .vlan
>   =macaddr -> .mac
> 
> We probably agree that having "=drive" work sometimes, but not always,
> is the worst of both worlds.  Still doesn't make it stable material, IMO.
> 
> > Agree on the uselessness of "on/off".
> > 
> > Agree on the uselessness of "blocksize" without a definition of the
> > term.
> > 
> > "chr" and "netdev" are like "drive", and replacing them by "str" is a
> > degradation in my book.
> 
> It is, but we're suprisingly consistent in the naming of such
> special-typed properties.  So it's actually a good thing that
> legacy_name provides redundant information.

str really should be for free-form strings.
It makes as much sense to call it as string as it is
to call an integer a string because you type
a string of characters to specify it.


> > Help for enum-valued properties in the form of "prop=ENUM-NAME" is not
> > really helpful without a definition of ENUM-NAME.  It's still useful for
> > finding devices with this kind of property.
> 
> Yes, but here you wouldn't have 'str', you would have the corresponding
> QAPI enum name.  This would be an improvement, though a minor one.
> 
> Paolo

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Paolo Bonzini]

Il 16/09/2014 12:37, Michael S. Tsirkin ha scritto:
> str really should be for free-form strings.
> It makes as much sense to call it as string

Yes, that's why I said drive->str is a degradation.  The question is,
how important is that degradation?

> as it is to call an integer a string because you type
> a string of characters to specify it.

I disagree.  This is true for the command line, and for historical
reasons it is also true for device-add, but the newer QOM commands are
type-safe.  You cannot pass an integer as a JSON string '123' to qom-set
or object-add, for example.

Paolo

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Gonglei (Arei)]

Hi,

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Sent: Tuesday, September 16, 2014 6:46 PM
> Subject: Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
> 
> Il 16/09/2014 12:37, Michael S. Tsirkin ha scritto:
> > str really should be for free-form strings.
> > It makes as much sense to call it as string
> 
> Yes, that's why I said drive->str is a degradation.  The question is,
> how important is that degradation?
> 
After reading your conversation, I'm confused and 
have no idea for PATCH 2 and PATCH 3.  Would you 
give me corresponding comments? Thanks!

Best regards,
-Gonglei

> > as it is to call an integer a string because you type
> > a string of characters to specify it.
> 
> I disagree.  This is true for the command line, and for historical
> reasons it is also true for device-add, but the newer QOM commands are
> type-safe.  You cannot pass an integer as a JSON string '123' to qom-set
> or object-add, for example.
> 

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 16/09/2014 11:16, Markus Armbruster ha scritto:
>>> I think both "str" and "link<block-backend>" actually are a small degradation
>>> compared to "drive", and this is why I kept the legacy_name.  But overall I
>>> think it's not really worth the layering violation that patches 2 and 3 are;
>>> and it's definitely not stable material.
>> 
>> "str" is clearly a degradation for me.  I breaks usage like
>> 
>>     for i in `qemu -device help 2>&1 | sed -n 's/^name "\([^"]*\)".*/\1/p'`
>>     do qemu -device $i,help 2>&1
>>     done | grep =drive
>> 
>> Finds all block device models.  I've done such things many times.
>
> Just replace "grep =drive" with "fgrep .drive".  Similarly:
>
>   =chr     -> .chardev  (bonus: matches the command line option)
>   =netdev  -> .netdev
>   =vlan    -> .vlan
>   =macaddr -> .mac

Unlike =drive, this isn't guaranteed to work.

> We probably agree that having "=drive" work sometimes, but not always,
> is the worst of both worlds.  Still doesn't make it stable material, IMO.

I'd consider it for stable.  It's a regression.  Pretty harmless, but
regression all the same.  A pretty harmless regression may be preferable
to a hairy patch.  Tradeoff, should be evaluated by the stable
maintainer.

>> Agree on the uselessness of "on/off".
>> 
>> Agree on the uselessness of "blocksize" without a definition of the
>> term.
>> 
>> "chr" and "netdev" are like "drive", and replacing them by "str" is a
>> degradation in my book.
>
> It is, but we're suprisingly consistent in the naming of such
> special-typed properties.  So it's actually a good thing that
> legacy_name provides redundant information.

Given our overall consistency track record: yes, it's surprising, and no,
I'm not comfortable relying on us being consistent :)

>> Help for enum-valued properties in the form of "prop=ENUM-NAME" is not
>> really helpful without a definition of ENUM-NAME.  It's still useful for
>> finding devices with this kind of property.
>
> Yes, but here you wouldn't have 'str', you would have the corresponding
> QAPI enum name.  This would be an improvement, though a minor one.

I'd be fine with selectively dropping those legacy_name that are
actually less helpful than name.

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Paolo Bonzini]

Il 16/09/2014 16:32, Markus Armbruster ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Il 16/09/2014 11:16, Markus Armbruster ha scritto:
>>>> I think both "str" and "link<block-backend>" actually are a small degradation
>>>> compared to "drive", and this is why I kept the legacy_name.  But overall I
>>>> think it's not really worth the layering violation that patches 2 and 3 are;
>>>> and it's definitely not stable material.
>>>
>>> "str" is clearly a degradation for me.  I breaks usage like
>>>
>>>     for i in `qemu -device help 2>&1 | sed -n 's/^name "\([^"]*\)".*/\1/p'`
>>>     do qemu -device $i,help 2>&1
>>>     done | grep =drive
>>>
>>> Finds all block device models.  I've done such things many times.
>>
>> Just replace "grep =drive" with "fgrep .drive".  Similarly:
>>
>>   =chr     -> .chardev  (bonus: matches the command line option)
>>   =netdev  -> .netdev
>>   =vlan    -> .vlan
>>   =macaddr -> .mac
> 
> Unlike =drive, this isn't guaranteed to work.

If it doesn't, when we've been consistent so far, it's a bug.

>> It is, but we're suprisingly consistent in the naming of such
>> special-typed properties.  So it's actually a good thing that
>> legacy_name provides redundant information.
> 
> Given our overall consistency track record: yes, it's surprising, and no,
> I'm not comfortable relying on us being consistent :)

The point of stuff like QOM is exactly to force us to be consistent.
No, it doesn't always work.  But patches 2 and 3 really are a layering
violation.  I have no idea how to bring back "drive" without introducing
a layering violation somewhere, but this one is really too much...

Paolo

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Michael S. Tsirkin]

On Tue, Sep 16, 2014 at 12:45:43PM +0200, Paolo Bonzini wrote:
> Il 16/09/2014 12:37, Michael S. Tsirkin ha scritto:
> > str really should be for free-form strings.
> > It makes as much sense to call it as string
> 
> Yes, that's why I said drive->str is a degradation.  The question is,
> how important is that degradation?
> 
> > as it is to call an integer a string because you type
> > a string of characters to specify it.
> 
> I disagree.  This is true for the command line, and for historical
> reasons it is also true for device-add, but the newer QOM commands are
> type-safe.  You cannot pass an integer as a JSON string '123' to qom-set
> or object-add, for example.
> 
> Paolo

Right so types should be explicit.
If an arbitrary string isn't allowed, this should be documented.
It's not great as is: what's the format for macaddr? AA:BB:CC:DD:EE:FF?
aa:bb:cc:dd:ee:ff? aabbccddeeff? 0xaabbccddeeff?
But just saying "string" is going in the wrong direction imho.

-- 
MST

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Paolo Bonzini]

Il 16/09/2014 18:26, Michael S. Tsirkin ha scritto:
> Right so types should be explicit.
> If an arbitrary string isn't allowed, this should be documented.
> It's not great as is: what's the format for macaddr? AA:BB:CC:DD:EE:FF?
> aa:bb:cc:dd:ee:ff? aabbccddeeff? 0xaabbccddeeff?
> But just saying "string" is going in the wrong direction imho.

That's the purpose of documentation (docs/qdev-device-use.txt), and even
then is better done with examples.  I don't think doing it in -device
foo,help (which I'm not even sure is particularly helpful.

"ip link help" doesn't document the format of a MAC address, either.

I'm sympathetic towards fixing the drive->str change, but I have no idea
how to do it.

Paolo

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Michael S. Tsirkin]

On Tue, Sep 16, 2014 at 06:27:51PM +0200, Paolo Bonzini wrote:
> Il 16/09/2014 18:26, Michael S. Tsirkin ha scritto:
> > Right so types should be explicit.
> > If an arbitrary string isn't allowed, this should be documented.
> > It's not great as is: what's the format for macaddr? AA:BB:CC:DD:EE:FF?
> > aa:bb:cc:dd:ee:ff? aabbccddeeff? 0xaabbccddeeff?
> > But just saying "string" is going in the wrong direction imho.
> 
> That's the purpose of documentation (docs/qdev-device-use.txt),

That's not user documentation, that's developer documentation,
isn't it?

> and even
> then is better done with examples.  I don't think doing it in -device
> foo,help (which I'm not even sure is particularly helpful.

-device foo,help isn't helpful at all because it does not
tell people what does each option do.
But it really should be fixed.

> "ip link help" doesn't document the format of a MAC address, either.

Networking on linux is notoriously hard to configure.
Hence the rise of wrapper tools such as network manager
to hide all the details from users.

> I'm sympathetic towards fixing the drive->str change, but I have no idea
> how to do it.
> 
> Paolo

Change legacy_name to point to a detailed human-readable
description of the type?
E.g. "Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF"?

We really really should add description to all properties, too.

-- 
MST

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Paolo Bonzini]

Il 16/09/2014 18:56, Michael S. Tsirkin ha scritto:
> On Tue, Sep 16, 2014 at 06:27:51PM +0200, Paolo Bonzini wrote:
>> Il 16/09/2014 18:26, Michael S. Tsirkin ha scritto:
>>> Right so types should be explicit.
>>> If an arbitrary string isn't allowed, this should be documented.
>>> It's not great as is: what's the format for macaddr? AA:BB:CC:DD:EE:FF?
>>> aa:bb:cc:dd:ee:ff? aabbccddeeff? 0xaabbccddeeff?
>>> But just saying "string" is going in the wrong direction imho.
>>
>> That's the purpose of documentation (docs/qdev-device-use.txt),
> 
> That's not user documentation, that's developer documentation,
> isn't it?

It's user documentation.  It's not distributed because we suck at
documentation.

>> and even
>> then is better done with examples.  I don't think doing it in -device
>> foo,help (which I'm not even sure is particularly helpful.
> 
> -device foo,help isn't helpful at all because it does not
> tell people what does each option do.
> But it really should be fixed.

Exactly.

>> I'm sympathetic towards fixing the drive->str change, but I have no idea
>> how to do it.
> 
> Change legacy_name to point to a detailed human-readable
> description of the type?
> E.g. "Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF"?

If libvirt can cope with

e1000.mac=str (Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF)

that would work for me.

> We really really should add description to all properties, too.

This is a huge job.  We have hundreds of properties.

Paolo

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Michael S. Tsirkin]

On Tue, Sep 16, 2014 at 08:31:08PM +0200, Paolo Bonzini wrote:
> Il 16/09/2014 18:56, Michael S. Tsirkin ha scritto:
> > On Tue, Sep 16, 2014 at 06:27:51PM +0200, Paolo Bonzini wrote:
> >> Il 16/09/2014 18:26, Michael S. Tsirkin ha scritto:
> >>> Right so types should be explicit.
> >>> If an arbitrary string isn't allowed, this should be documented.
> >>> It's not great as is: what's the format for macaddr? AA:BB:CC:DD:EE:FF?
> >>> aa:bb:cc:dd:ee:ff? aabbccddeeff? 0xaabbccddeeff?
> >>> But just saying "string" is going in the wrong direction imho.
> >>
> >> That's the purpose of documentation (docs/qdev-device-use.txt),
> > 
> > That's not user documentation, that's developer documentation,
> > isn't it?
> 
> It's user documentation.  It's not distributed because we suck at
> documentation.
> 
> >> and even
> >> then is better done with examples.  I don't think doing it in -device
> >> foo,help (which I'm not even sure is particularly helpful.
> > 
> > -device foo,help isn't helpful at all because it does not
> > tell people what does each option do.
> > But it really should be fixed.
> 
> Exactly.
> 
> >> I'm sympathetic towards fixing the drive->str change, but I have no idea
> >> how to do it.
> > 
> > Change legacy_name to point to a detailed human-readable
> > description of the type?
> > E.g. "Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF"?
> 
> If libvirt can cope with
> 
> e1000.mac=str (Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF)
> 
> that would work for me.

Shouldn't "str" be "string" in HMP?

Eblake - type is ignored right? Does this mean anything to the right of
= is ignored?

> > We really really should add description to all properties, too.
> 
> This is a huge job.  We have hundreds of properties.
> 
> Paolo

Right. If we don't start we won't get there though, will we?

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Michael S. Tsirkin]

On Tue, Sep 16, 2014 at 10:01:19PM +0300, Michael S. Tsirkin wrote:
> On Tue, Sep 16, 2014 at 08:31:08PM +0200, Paolo Bonzini wrote:
> > Il 16/09/2014 18:56, Michael S. Tsirkin ha scritto:
> > > On Tue, Sep 16, 2014 at 06:27:51PM +0200, Paolo Bonzini wrote:
> > >> Il 16/09/2014 18:26, Michael S. Tsirkin ha scritto:
> > >>> Right so types should be explicit.
> > >>> If an arbitrary string isn't allowed, this should be documented.
> > >>> It's not great as is: what's the format for macaddr? AA:BB:CC:DD:EE:FF?
> > >>> aa:bb:cc:dd:ee:ff? aabbccddeeff? 0xaabbccddeeff?
> > >>> But just saying "string" is going in the wrong direction imho.
> > >>
> > >> That's the purpose of documentation (docs/qdev-device-use.txt),
> > > 
> > > That's not user documentation, that's developer documentation,
> > > isn't it?
> > 
> > It's user documentation.  It's not distributed because we suck at
> > documentation.
> > 
> > >> and even
> > >> then is better done with examples.  I don't think doing it in -device
> > >> foo,help (which I'm not even sure is particularly helpful.
> > > 
> > > -device foo,help isn't helpful at all because it does not
> > > tell people what does each option do.
> > > But it really should be fixed.
> > 
> > Exactly.
> > 
> > >> I'm sympathetic towards fixing the drive->str change, but I have no idea
> > >> how to do it.
> > > 
> > > Change legacy_name to point to a detailed human-readable
> > > description of the type?
> > > E.g. "Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF"?
> > 
> > If libvirt can cope with
> > 
> > e1000.mac=str (Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF)
> > 
> > that would work for me.
> 
> Shouldn't "str" be "string" in HMP?
> 
> Eblake - type is ignored right? Does this mean anything to the right of
> = is ignored?
> 
> > > We really really should add description to all properties, too.
> > 
> > This is a huge job.  We have hundreds of properties.
> > 
> > Paolo
> 
> Right. If we don't start we won't get there though, will we?
> 

And, we'll keep adding undocumented ones.

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Eric Blake]

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

On 09/16/2014 12:31 PM, Paolo Bonzini wrote:

>> Change legacy_name to point to a detailed human-readable
>> description of the type?
>> E.g. "Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF"?
> 
> If libvirt can cope with
> 
> e1000.mac=str (Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF)

Isn't this output only available under -help? Libvirt only cares about
QMP listings of devices and their properties, so improving the human
interface should have no negative impact to the machine interface.

-- 
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: 539 bytes --]

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Gonglei (Arei)]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Sent: Tuesday, September 16, 2014 10:37 PM
> Subject: Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
> 
> Il 16/09/2014 16:32, Markus Armbruster ha scritto:
> > Paolo Bonzini <pbonzini@redhat.com> writes:
> >
> >> Il 16/09/2014 11:16, Markus Armbruster ha scritto:
> >>>> I think both "str" and "link<block-backend>" actually are a small
> degradation
> >>>> compared to "drive", and this is why I kept the legacy_name.  But
> overall I
> >>>> think it's not really worth the layering violation that patches 2 and 3 are;
> >>>> and it's definitely not stable material.
> >>>
> >>> "str" is clearly a degradation for me.  I breaks usage like
> >>>
> >>>     for i in `qemu -device help 2>&1 | sed -n 's/^name "\([^"]*\)".*/\1/p'`
> >>>     do qemu -device $i,help 2>&1
> >>>     done | grep =drive
> >>>
> >>> Finds all block device models.  I've done such things many times.
> >>
> >> Just replace "grep =drive" with "fgrep .drive".  Similarly:
> >>
> >>   =chr     -> .chardev  (bonus: matches the command line option)
> >>   =netdev  -> .netdev
> >>   =vlan    -> .vlan
> >>   =macaddr -> .mac
> >
> > Unlike =drive, this isn't guaranteed to work.
> 
> If it doesn't, when we've been consistent so far, it's a bug.
> 
> >> It is, but we're suprisingly consistent in the naming of such
> >> special-typed properties.  So it's actually a good thing that
> >> legacy_name provides redundant information.
> >
> > Given our overall consistency track record: yes, it's surprising, and no,
> > I'm not comfortable relying on us being consistent :)
> 
> The point of stuff like QOM is exactly to force us to be consistent.
> No, it doesn't always work.  But patches 2 and 3 really are a layering
> violation.  I have no idea how to bring back "drive" without introducing
> a layering violation somewhere, but this one is really too much...
> 
Sorry, I can't understand the layering violation on PATCH2 and PATCH3.
AliasProperty struct is QOM object and ObjectProperty is also QOM object,
I just move AliasProperty struct from Object.c to Object.h meanwhile add
a point reference in ObjectProperty struct for AliasProperty. Does this is a
layering violation? 

Hope for your more detailed information about this, thanks in advance! :)

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 09/16/2014 12:31 PM, Paolo Bonzini wrote:
>
>>> Change legacy_name to point to a detailed human-readable
>>> description of the type?
>>> E.g. "Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF"?
>> 
>> If libvirt can cope with
>> 
>> e1000.mac=str (Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF)
>
> Isn't this output only available under -help? Libvirt only cares about
> QMP listings of devices and their properties, so improving the human
> interface should have no negative impact to the machine interface.

Libvirt still has code parsing this, see virQEMUCapsExtractDeviceStr().
Whether it's actually used with recent QEMU I don't know.

-device FOO,help merely formats qmp_device_list_properties()'s value for
human consumption.  So, to make a description string available for help,
we also have to put it into the function's value.  If we don't want it
in QMP, we need to filter it out in the command handler.  Right now,
qmp_device_list_properties() *is* the command handler.

Aside: why isn't the QMP command named query-device-list-properties?  We
suck at consistency!

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Tue, Sep 16, 2014 at 10:01:19PM +0300, Michael S. Tsirkin wrote:
>> On Tue, Sep 16, 2014 at 08:31:08PM +0200, Paolo Bonzini wrote:
>> > Il 16/09/2014 18:56, Michael S. Tsirkin ha scritto:
>> > > On Tue, Sep 16, 2014 at 06:27:51PM +0200, Paolo Bonzini wrote:
>> > >> Il 16/09/2014 18:26, Michael S. Tsirkin ha scritto:
>> > >>> Right so types should be explicit.
>> > >>> If an arbitrary string isn't allowed, this should be documented.
>> > >>> It's not great as is: what's the format for macaddr? AA:BB:CC:DD:EE:FF?
>> > >>> aa:bb:cc:dd:ee:ff? aabbccddeeff? 0xaabbccddeeff?
>> > >>> But just saying "string" is going in the wrong direction imho.
>> > >>
>> > >> That's the purpose of documentation (docs/qdev-device-use.txt),
>> > > 
>> > > That's not user documentation, that's developer documentation,
>> > > isn't it?
>> > 
>> > It's user documentation.  It's not distributed because we suck at
>> > documentation.
>> > 
>> > >> and even
>> > >> then is better done with examples.  I don't think doing it in -device
>> > >> foo,help (which I'm not even sure is particularly helpful.
>> > > 
>> > > -device foo,help isn't helpful at all because it does not
>> > > tell people what does each option do.
>> > > But it really should be fixed.
>> > 
>> > Exactly.
>> > 
>> > >> I'm sympathetic towards fixing the drive->str change, but I have no idea
>> > >> how to do it.
>> > > 
>> > > Change legacy_name to point to a detailed human-readable
>> > > description of the type?
>> > > E.g. "Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF"?
>> > 
>> > If libvirt can cope with
>> > 
>> > e1000.mac=str (Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF)
>> > 
>> > that would work for me.
>> 
>> Shouldn't "str" be "string" in HMP?

Probably.

What about QMP?  JSON calls the type "string".  Possible justification
for "str": QMP makes up its own type system, losely based on JSON's,
with its own terminology.

I'm not sure "=T" adds value over the description.  Certainly not in the
example above.

>> Eblake - type is ignored right? Does this mean anything to the right of
>> = is ignored?

As far as I can tell from the libvirt sources: yes.

>> > > We really really should add description to all properties, too.
>> > 
>> > This is a huge job.  We have hundreds of properties.
>> > 
>> > Paolo
>> 
>> Right. If we don't start we won't get there though, will we?
>> 
>
> And, we'll keep adding undocumented ones.

Agree with all three points.

We should get our act together on property documentation.

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Eric Blake]

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

On 09/16/2014 11:54 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 09/16/2014 12:31 PM, Paolo Bonzini wrote:
>>
>>>> Change legacy_name to point to a detailed human-readable
>>>> description of the type?
>>>> E.g. "Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF"?
>>>
>>> If libvirt can cope with
>>>
>>> e1000.mac=str (Ethernet 6-byte MAC Address, format: AA:BB:CC:DD:EE:FF)
>>
>> Isn't this output only available under -help? Libvirt only cares about
>> QMP listings of devices and their properties, so improving the human
>> interface should have no negative impact to the machine interface.
> 
> Libvirt still has code parsing this, see virQEMUCapsExtractDeviceStr().
> Whether it's actually used with recent QEMU I don't know.

That code is only used for qemu < 1.5 (when -help parsing is all we can
use) - but those versions of qemu are not going to change -help output,
and new versions of qemu use only QMP, so you are free to change it with
no impact to libvirt.

> 
> -device FOO,help merely formats qmp_device_list_properties()'s value for
> human consumption.  So, to make a description string available for help,
> we also have to put it into the function's value.  If we don't want it
> in QMP, we need to filter it out in the command handler.  Right now,
> qmp_device_list_properties() *is* the command handler.
> 
> Aside: why isn't the QMP command named query-device-list-properties?  We
> suck at consistency!
> 
> 

-- 
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: 539 bytes --]

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Gonglei (Arei)]

Hi, Paolo

> -----Original Message-----
> From: Gonglei (Arei)
> Sent: Wednesday, September 17, 2014 10:32 AM
> To: 'Paolo Bonzini'; Markus Armbruster
> Cc: Huangweidong (C); aliguori@amazon.com; Michael S. Tsirkin;
> agraf@suse.de; qemu-devel@nongnu.org; stefanha@redhat.com; Huangpeng
> (Peter); lcapitulino@redhat.com
> Subject: RE: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
> 
> > From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> > Sent: Tuesday, September 16, 2014 10:37 PM
> > Subject: Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
> >
> > Il 16/09/2014 16:32, Markus Armbruster ha scritto:
> > > Paolo Bonzini <pbonzini@redhat.com> writes:
> > >
> > >> Il 16/09/2014 11:16, Markus Armbruster ha scritto:
> > >>>> I think both "str" and "link<block-backend>" actually are a small
> > degradation
> > >>>> compared to "drive", and this is why I kept the legacy_name.  But
> > overall I
> > >>>> think it's not really worth the layering violation that patches 2 and 3 are;
> > >>>> and it's definitely not stable material.
> > >>>
> > >>> "str" is clearly a degradation for me.  I breaks usage like
> > >>>
> > >>>     for i in `qemu -device help 2>&1 | sed -n 's/^name
> "\([^"]*\)".*/\1/p'`
> > >>>     do qemu -device $i,help 2>&1
> > >>>     done | grep =drive
> > >>>
> > >>> Finds all block device models.  I've done such things many times.
> > >>
> > >> Just replace "grep =drive" with "fgrep .drive".  Similarly:
> > >>
> > >>   =chr     -> .chardev  (bonus: matches the command line option)
> > >>   =netdev  -> .netdev
> > >>   =vlan    -> .vlan
> > >>   =macaddr -> .mac
> > >
> > > Unlike =drive, this isn't guaranteed to work.
> >
> > If it doesn't, when we've been consistent so far, it's a bug.
> >
> > >> It is, but we're suprisingly consistent in the naming of such
> > >> special-typed properties.  So it's actually a good thing that
> > >> legacy_name provides redundant information.
> > >
> > > Given our overall consistency track record: yes, it's surprising, and no,
> > > I'm not comfortable relying on us being consistent :)
> >
> > The point of stuff like QOM is exactly to force us to be consistent.
> > No, it doesn't always work.  But patches 2 and 3 really are a layering
> > violation.  I have no idea how to bring back "drive" without introducing
> > a layering violation somewhere, but this one is really too much...
> >
> Sorry, I can't understand the layering violation on PATCH2 and PATCH3.
> AliasProperty struct is QOM object and ObjectProperty is also QOM object,
> I just move AliasProperty struct from Object.c to Object.h meanwhile add
> a point reference in ObjectProperty struct for AliasProperty. Does this is a
> layering violation?
> 

Ping...?  Thanks !

Best regards,
-Gonglei

> Hope for your more detailed information about this, thanks in advance! :)
> 
> Best regards,
> -Gonglei

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Michael S. Tsirkin]

On Mon, Sep 15, 2014 at 10:44:36PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> At present, people have no way to know they should
> have a specific format for alias properties.
> 
>  Example:
> 
> before output:
> 
> virtio-blk-pci.physical_block_size=uint16
> virtio-blk-pci.logical_block_size=uint16
> virtio-blk-pci.drive=str
> 
> after output applied this patch series:
> 
> virtio-blk-pci.physical_block_size=blocksize
> virtio-blk-pci.logical_block_size=blocksize
> virtio-blk-pci.drive=drive
> 
> Gonglei (3):
>   qom: add error handler for object alias property
>   qom: add target object poniter for alias property in ObjectProperty
>   qmp: print real legacy_name for alias property
> 
>  include/qom/object.h |  3 +++
>  qmp.c                | 68 ++++++++++++++++++++++++++++++++++++----------------
>  qom/object.c         | 10 +++++++-
>  3 files changed, 59 insertions(+), 22 deletions(-)

Basically this patch brings back historical HMP behaviour.
As far as I could tell, it wasn't changed intentionally.
So how about applying this first and then making more
changes on top if required?


> -- 
> 1.7.12.4
> 

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Gonglei (Arei)]

> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Monday, September 22, 2014 4:34 PM
> Subject: Re: [PATCH 0/3] Fix confused output for alias properties
> 
> On Mon, Sep 15, 2014 at 10:44:36PM +0800, arei.gonglei@huawei.com wrote:
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > At present, people have no way to know they should
> > have a specific format for alias properties.
> >
> >  Example:
> >
> > before output:
> >
> > virtio-blk-pci.physical_block_size=uint16
> > virtio-blk-pci.logical_block_size=uint16
> > virtio-blk-pci.drive=str
> >
> > after output applied this patch series:
> >
> > virtio-blk-pci.physical_block_size=blocksize
> > virtio-blk-pci.logical_block_size=blocksize
> > virtio-blk-pci.drive=drive
> >
> > Gonglei (3):
> >   qom: add error handler for object alias property
> >   qom: add target object poniter for alias property in ObjectProperty
> >   qmp: print real legacy_name for alias property
> >
> >  include/qom/object.h |  3 +++
> >  qmp.c                | 68
> ++++++++++++++++++++++++++++++++++++----------------
> >  qom/object.c         | 10 +++++++-
> >  3 files changed, 59 insertions(+), 22 deletions(-)
> 
> Basically this patch brings back historical HMP behaviour.
> As far as I could tell, it wasn't changed intentionally.
> So how about applying this first and then making more
> changes on top if required?
> 
I agree with you because the below virtio patch series is
somewhat of relying on this. (Please apply v2 if possible)

 [PATCH v2 0/9] virtio: fix virtio child recount in transports

What's your opinion, Paolo?  Thanks!

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Paolo Bonzini]

Il 22/09/2014 10:34, Michael S. Tsirkin ha scritto:
> Basically this patch brings back historical HMP behaviour.
> As far as I could tell, it wasn't changed intentionally.

It was changed intentionally.  Or rather, the change was known at the
time Stefan made it.

> So how about applying this first and then making more
> changes on top if required?

No.

There is no reason to add alias-specific fields to ObjectProperty, and
the implementation is not even complete because you can alias property X
of object A to property Y of object B.

Paolo

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Mon, Sep 15, 2014 at 10:44:36PM +0800, arei.gonglei@huawei.com wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>> 
>> At present, people have no way to know they should
>> have a specific format for alias properties.
>> 
>>  Example:
>> 
>> before output:
>> 
>> virtio-blk-pci.physical_block_size=uint16
>> virtio-blk-pci.logical_block_size=uint16
>> virtio-blk-pci.drive=str
>> 
>> after output applied this patch series:
>> 
>> virtio-blk-pci.physical_block_size=blocksize
>> virtio-blk-pci.logical_block_size=blocksize
>> virtio-blk-pci.drive=drive
>> 
>> Gonglei (3):
>>   qom: add error handler for object alias property
>>   qom: add target object poniter for alias property in ObjectProperty
>>   qmp: print real legacy_name for alias property
>> 
>>  include/qom/object.h |  3 +++
>>  qmp.c                | 68 ++++++++++++++++++++++++++++++++++++----------------
>>  qom/object.c         | 10 +++++++-
>>  3 files changed, 59 insertions(+), 22 deletions(-)
>
> Basically this patch brings back historical HMP behaviour.
> As far as I could tell, it wasn't changed intentionally.
> So how about applying this first and then making more
> changes on top if required?

Makes sense to me.

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Gonglei (Arei)]

> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo
> Bonzini
> Sent: Monday, September 22, 2014 5:15 PM
> To: Michael S. Tsirkin; Gonglei (Arei)
> Cc: agraf@suse.de; Huangweidong (C); aliguori@amazon.com; Huangpeng
> (Peter); qemu-devel@nongnu.org; stefanha@redhat.com;
> lcapitulino@redhat.com
> Subject: Re: [PATCH 0/3] Fix confused output for alias properties
> 
> Il 22/09/2014 10:34, Michael S. Tsirkin ha scritto:
> > Basically this patch brings back historical HMP behaviour.
> > As far as I could tell, it wasn't changed intentionally.
> 
> It was changed intentionally.  Or rather, the change was known at the
> time Stefan made it.
> 
> > So how about applying this first and then making more
> > changes on top if required?
> 
> No.
> 
> There is no reason to add alias-specific fields to ObjectProperty, and
> the implementation is not even complete because you can alias property X
> of object A to property Y of object B.
> 

Yes, I knew this issue which object A's property name may
is not the same with object B, so I had posted v2 to fix it.

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Paolo Bonzini]

Il 22/09/2014 11:33, Gonglei (Arei) ha scritto:
> Yes, I knew this issue which object A's property name may
> is not the same with object B, so I had posted v2 to fix it.

This doesn't change the fact that ObjectProperty is a generic struct,
and adding alias-specific fields there is wrong.

Paolo

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Gonglei (Arei)]

> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo
> Bonzini
> Sent: Monday, September 22, 2014 6:04 PM
> To: Gonglei (Arei); Michael S. Tsirkin
> Cc: Huangweidong (C); aliguori@amazon.com; qemu-devel@nongnu.org;
> agraf@suse.de; stefanha@redhat.com; Huangpeng (Peter);
> lcapitulino@redhat.com
> Subject: Re: [PATCH 0/3] Fix confused output for alias properties
> 
> Il 22/09/2014 11:33, Gonglei (Arei) ha scritto:
> > Yes, I knew this issue which object A's property name may
> > is not the same with object B, so I had posted v2 to fix it.
> 
> This doesn't change the fact that ObjectProperty is a generic struct,
> and adding alias-specific fields there is wrong.
> 

OK, Maybe I should find other ways to attach this purpose and
avoid layering violation. Thanks!

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 22/09/2014 10:34, Michael S. Tsirkin ha scritto:
>> Basically this patch brings back historical HMP behaviour.
>> As far as I could tell, it wasn't changed intentionally.
>
> It was changed intentionally.  Or rather, the change was known at the
> time Stefan made it.

Commit hash?

>> So how about applying this first and then making more
>> changes on top if required?
>
> No.
>
> There is no reason to add alias-specific fields to ObjectProperty, and
> the implementation is not even complete because you can alias property X
> of object A to property Y of object B.

I think there are two questions here.  The first one is whether to
accept the regression and move on.  The second one is whether this patch
is a satisfactory fix for the regression.

I'm inclined to answer the first question in the negative.  We discussed
the relative usefulness of the help information before and after the
regression, as well as after ripping out legacy_name entirely.  We agree
that help has always been somewhat cryptic.  I'd certainly support a
patch that replaces it wholesale by something better.  I'm opposed,
however, to degrading it further, if we can help it.

The fact that such a degradation has slipped in already can't change my
opinion.  I view it as regression.

Evidence of a deliberate, informed decision to regress could change it.
That's why I ask for the commit hash.

The "if we can help it" condition ties the first to the second question.

I haven't got this part of the code present in my mind at the moment,
but I'm willing to take your assertion of a layering violation for
granted.  Is this violation necessary for fixing the regression, or is
it just an artifact of this particular fix?

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Paolo Bonzini]

Il 22/09/2014 13:22, Gonglei (Arei) ha scritto:
> > This doesn't change the fact that ObjectProperty is a generic struct,
> > and adding alias-specific fields there is wrong.
> 
> OK, Maybe I should find other ways to attach this purpose and
> avoid layering violation. Thanks!

Unfortunately I cannot think of any.

We could add a description field to ObjectProperty, and replace
legacy_name with a description.  The output then would be

virtio-blk.drive=str (drive)

That's a bit hackish, but perhaps it would be good enough for mst and
Markus?

Paolo

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Michael S. Tsirkin]

On Mon, Sep 22, 2014 at 02:06:38PM +0200, Paolo Bonzini wrote:
> Il 22/09/2014 13:22, Gonglei (Arei) ha scritto:
> > > This doesn't change the fact that ObjectProperty is a generic struct,
> > > and adding alias-specific fields there is wrong.
> > 
> > OK, Maybe I should find other ways to attach this purpose and
> > avoid layering violation. Thanks!
> 
> Unfortunately I cannot think of any.
> 
> We could add a description field to ObjectProperty, and replace
> legacy_name with a description.  The output then would be
> 
> virtio-blk.drive=str (drive)
> That's a bit hackish, but perhaps it would be good enough for mst and
> Markus?
> 
> Paolo

I would just drop "str" and replace it with drive.
I seems to convey zero information.
Description should be something more informative, e.g.
(ID of a drive to use as a backend)
Help for -device could add "Must match ID of a -drive option".
Help for device HMP command could add "Must match ID of a drive command."


-- 
MST

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 22/09/2014 13:22, Gonglei (Arei) ha scritto:
>> > This doesn't change the fact that ObjectProperty is a generic struct,
>> > and adding alias-specific fields there is wrong.
>> 
>> OK, Maybe I should find other ways to attach this purpose and
>> avoid layering violation. Thanks!
>
> Unfortunately I cannot think of any.
>
> We could add a description field to ObjectProperty, and replace
> legacy_name with a description.  The output then would be
>
> virtio-blk.drive=str (drive)
>
> That's a bit hackish, but perhaps it would be good enough for mst and
> Markus?

Works for me.  A step towards better self-documentation of properties.

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Paolo Bonzini]

Il 22/09/2014 14:34, Michael S. Tsirkin ha scritto:
> On Mon, Sep 22, 2014 at 02:06:38PM +0200, Paolo Bonzini wrote:
>> Il 22/09/2014 13:22, Gonglei (Arei) ha scritto:
>>>> This doesn't change the fact that ObjectProperty is a generic struct,
>>>> and adding alias-specific fields there is wrong.
>>>
>>> OK, Maybe I should find other ways to attach this purpose and
>>> avoid layering violation. Thanks!
>>
>> Unfortunately I cannot think of any.
>>
>> We could add a description field to ObjectProperty, and replace
>> legacy_name with a description.  The output then would be
>>
>> virtio-blk.drive=str (drive)
>> That's a bit hackish, but perhaps it would be good enough for mst and
>> Markus?
> 
> I would just drop "str" and replace it with drive.

In the above example, the format would be

   CLASS.PROPERTY=TYPE (DESCRIPTION)

where I just put "drive" as the description to resemble the old output.

You cannot just have

   foo.drive=drive

because the type is *not* "drive", it's "str" (if anything, we could
make it link<BlockBackend> which does not really show the connection
between BlockBackend and -drive).

The problem is that "drive" as the type (the legacy_name) is simply not
available for the virtio-blk-pci.drive property.  It's hidden beneath an
opaque pointer.  Gonglei's patches were adding fields to ObjectProperty
just to avoid visiting that opaque pointer, which I consider a layering
violation.

> I seems to convey zero information.
> Description should be something more informative, e.g.
> (ID of a drive to use as a backend)

That would of course be fine too.  The problem is then that we have 644
properties (DEFINE_PROP_* macro invocations) that someone must add a
description to.

> Help for -device could add "Must match ID of a -drive option".
> Help for device HMP command could add "Must match ID of a drive command."

You are turning a bug that nobody noticed until now, and that really
doesn't break anything, into a month or more worth of work.

Paolo

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Paolo Bonzini]

Il 22/09/2014 13:43, Markus Armbruster ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Il 22/09/2014 10:34, Michael S. Tsirkin ha scritto:
>>> Basically this patch brings back historical HMP behaviour.
>>> As far as I could tell, it wasn't changed intentionally.
>>
>> It was changed intentionally.  Or rather, the change was known at the
>> time Stefan made it.
> 
> Commit hash?

There are three commits involved.

The first is commit f4eb32b (qmp: show QOM properties in
device-list-properties, 2014-05-20).  It was done in preparation for the
change of virtio-blk-pci.drive from qdev property to QOM property, to
avoid breaking device-list-properties.

The actual change took some more time to review, so it went in one month
later.  Commit caffdac (virtio-blk: use aliases instead of duplicate
qdev properties, 2014-06-18) is what changed virtio-blk-pci.drive from
qdev property to QOM property.  This changed the type from 'drive' to
'str' in device-list-properties.  (Note that this patch fixed an actual
bug, it was not just for the sake of cleanup).

I cannot find any reference to the change; perhaps it was discussed only
on IRC.  However, I'm quite certain that I knew about it.

Now one thing did slip through; commit caffdac actually dropped
virtio-blk-pci.drive from -device virtio-blk-pci,help.  Either I
misremembered that the first commit fixed command-line help too, or I
just assumed that -device foo,help was implemented on top of
qmp_device_list_properties.  Which wasn't the case, so the third commit
(9ef52358, qdev-monitor: include QOM properties in -device FOO, help
output, 2014-07-09) did the right thing and brought back
virtio-blk-pci.drive into the help message.

Now, the cover latter for that patch finally has a hint that the change
was known.  http://thread.gmane.org/gmane.comp.emulators.qemu/285819 has
this text:

   We simply need to update -device FOO,help code to use both qdev and
   QOM properties.  Note that types change because a 'drive' qdev type
   is actually a 'str' QOM type.  We're moving more and more to QOM
   properties where the final type for this property would be
   'link<Drive>' or similar.

It is only in the cover letter and thus not part of any commit message.

> I haven't got this part of the code present in my mind at the moment,
> but I'm willing to take your assertion of a layering violation for
> granted.  Is this violation necessary for fixing the regression, or is
> it just an artifact of this particular fix?

I guess we could always add some ad-hoc mechanism for
device-list-properties to get to the "drive" string, and smuggle it as a
QOM feature.  Then, aliases would just forward that mechanism to the
aliased property, which would just work.

Of course, with every new feature we would most likely have yet another
unfinished transition.  In the lack of a clear user complaint (or even
of a clear indication that human users ever used -device foo,help...)
the tempation to pass the buck is strong.

Paolo

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Gonglei (Arei)]

> -----Original Message-----
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo
> Bonzini
> Sent: Monday, September 22, 2014 8:07 PM
> To: Gonglei (Arei); Michael S. Tsirkin
> Cc: agraf@suse.de; Huangweidong (C); aliguori@amazon.com; Huangpeng
> (Peter); qemu-devel@nongnu.org; stefanha@redhat.com;
> lcapitulino@redhat.com
> Subject: Re: [PATCH 0/3] Fix confused output for alias properties
> 
> Il 22/09/2014 13:22, Gonglei (Arei) ha scritto:
> > > This doesn't change the fact that ObjectProperty is a generic struct,
> > > and adding alias-specific fields there is wrong.
> >
> > OK, Maybe I should find other ways to attach this purpose and
> > avoid layering violation. Thanks!
> 
> Unfortunately I cannot think of any.
> 

What about this below way? Thanks!

(I don't add alias-sepecific filed into ObjectProperty
struct, just add a bool property. )

diff --git a/include/qom/object.h b/include/qom/object.h
index 8a05a81..8b8ded5 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -334,6 +334,11 @@ typedef void (ObjectPropertyRelease)(Object *obj,
                                      const char *name,
                                      void *opaque);

+typedef struct {
+    Object *target_obj;
+    const char *target_name;
+} AliasProperty;
+
 typedef struct ObjectProperty
 {
     gchar *name;
@@ -344,6 +349,8 @@ typedef struct ObjectProperty
     ObjectPropertyRelease *release;
     void *opaque;

+    bool is_alias;
+
     QTAILQ_ENTRY(ObjectProperty) node;
 } ObjectProperty;

diff --git a/qom/object.c b/qom/object.c
index a8c3065..0b4e402 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1590,11 +1590,6 @@ void object_property_add_uint64_ptr(Object *obj, const char *name,
                         NULL, NULL, (void *)v, errp);
 }

-typedef struct {
-    Object *target_obj;
-    const char *target_name;
-} AliasProperty;
-
 static void property_get_alias(Object *obj, struct Visitor *v, void *opaque,
                                const char *name, Error **errp)
 {
@@ -1663,6 +1658,7 @@ void object_property_add_alias(Object *obj, const char *name,
         goto out;
     }
     op->resolve = property_resolve_alias;
+    op->is_alias = true;

 out:
     g_free(prop_type);
-- 
1.7.12.4


> We could add a description field to ObjectProperty, and replace
> legacy_name with a description.  The output then would be
> 
> virtio-blk.drive=str (drive)
> 
> That's a bit hackish, but perhaps it would be good enough for mst and
> Markus?
> 
> Paolo

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Gonglei (Arei)]

> > From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo
> > Bonzini
> > Sent: Monday, September 22, 2014 8:07 PM
> > To: Gonglei (Arei); Michael S. Tsirkin
> > Cc: agraf@suse.de; Huangweidong (C); aliguori@amazon.com; Huangpeng
> > (Peter); qemu-devel@nongnu.org; stefanha@redhat.com;
> > lcapitulino@redhat.com
> > Subject: Re: [PATCH 0/3] Fix confused output for alias properties
> >
> > Il 22/09/2014 13:22, Gonglei (Arei) ha scritto:
> > > > This doesn't change the fact that ObjectProperty is a generic struct,
> > > > and adding alias-specific fields there is wrong.
> > >
> > > OK, Maybe I should find other ways to attach this purpose and
> > > avoid layering violation. Thanks!
> >
> > Unfortunately I cannot think of any.
> >
> 
> What about this below way? Thanks!
> 
> (I don't add alias-sepecific filed into ObjectProperty
> struct, just add a bool property. )
> 

Then we can cast "void *opaque" of ObjectPropery
to AliasProperty struct. :)

> diff --git a/include/qom/object.h b/include/qom/object.h
> index 8a05a81..8b8ded5 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -334,6 +334,11 @@ typedef void (ObjectPropertyRelease)(Object *obj,
>                                       const char *name,
>                                       void *opaque);
> 
> +typedef struct {
> +    Object *target_obj;
> +    const char *target_name;
> +} AliasProperty;
> +
>  typedef struct ObjectProperty
>  {
>      gchar *name;
> @@ -344,6 +349,8 @@ typedef struct ObjectProperty
>      ObjectPropertyRelease *release;
>      void *opaque;
> 
> +    bool is_alias;
> +
>      QTAILQ_ENTRY(ObjectProperty) node;
>  } ObjectProperty;
> 
> diff --git a/qom/object.c b/qom/object.c
> index a8c3065..0b4e402 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1590,11 +1590,6 @@ void object_property_add_uint64_ptr(Object *obj,
> const char *name,
>                          NULL, NULL, (void *)v, errp);
>  }
> 
> -typedef struct {
> -    Object *target_obj;
> -    const char *target_name;
> -} AliasProperty;
> -
>  static void property_get_alias(Object *obj, struct Visitor *v, void *opaque,
>                                 const char *name, Error **errp)
>  {
> @@ -1663,6 +1658,7 @@ void object_property_add_alias(Object *obj, const
> char *name,
>          goto out;
>      }
>      op->resolve = property_resolve_alias;
> +    op->is_alias = true;
> 
>  out:
>      g_free(prop_type);
> --
> 1.7.12.4
> 
> 
> > We could add a description field to ObjectProperty, and replace
> > legacy_name with a description.  The output then would be
> >
> > virtio-blk.drive=str (drive)
> >
> > That's a bit hackish, but perhaps it would be good enough for mst and
> > Markus?
> >
> > Paolo
> 
> Best regards,
> -Gonglei

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Paolo Bonzini]

Il 22/09/2014 15:13, Gonglei (Arei) ha scritto:
> (I don't add alias-sepecific filed into ObjectProperty
> struct, just add a bool property. )
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 8a05a81..8b8ded5 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -334,6 +334,11 @@ typedef void (ObjectPropertyRelease)(Object *obj,
>                                       const char *name,
>                                       void *opaque);
> 
> +typedef struct {
> +    Object *target_obj;
> +    const char *target_name;
> +} AliasProperty;
> +
>  typedef struct ObjectProperty
>  {
>      gchar *name;
> @@ -344,6 +349,8 @@ typedef struct ObjectProperty
>      ObjectPropertyRelease *release;
>      void *opaque;
> 
> +    bool is_alias;
> +
>      QTAILQ_ENTRY(ObjectProperty) node;
>  } ObjectProperty;

I don't think this is right.  If the field is called "opaque", it's
opaque.  We already had cases where we were deriving the type of the
opaque, and they caused bugs (though it was using the const char *type
field; admittedly having a separate bool is a bit cleaner).

Paolo

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Gonglei (Arei)]

> Subject: Re: [PATCH 0/3] Fix confused output for alias properties
> 
> Il 22/09/2014 15:13, Gonglei (Arei) ha scritto:
> > (I don't add alias-sepecific filed into ObjectProperty
> > struct, just add a bool property. )
> >
> > diff --git a/include/qom/object.h b/include/qom/object.h
> > index 8a05a81..8b8ded5 100644
> > --- a/include/qom/object.h
> > +++ b/include/qom/object.h
> > @@ -334,6 +334,11 @@ typedef void (ObjectPropertyRelease)(Object *obj,
> >                                       const char *name,
> >                                       void *opaque);
> >
> > +typedef struct {
> > +    Object *target_obj;
> > +    const char *target_name;
> > +} AliasProperty;
> > +
> >  typedef struct ObjectProperty
> >  {
> >      gchar *name;
> > @@ -344,6 +349,8 @@ typedef struct ObjectProperty
> >      ObjectPropertyRelease *release;
> >      void *opaque;
> >
> > +    bool is_alias;
> > +
> >      QTAILQ_ENTRY(ObjectProperty) node;
> >  } ObjectProperty;
> 
> I don't think this is right.  If the field is called "opaque", it's
> opaque.  

Sorry?

> We already had cases where we were deriving the type of the
> opaque, and they caused bugs (though it was using the const char *type
> field; admittedly having a separate bool is a bit cleaner).
> 

Sorry for my poor English. 
I can't understand your mean clearly. :(

Would you explain it for me? Thanks a lot!

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Paolo Bonzini]

Il 22/09/2014 15:36, Gonglei (Arei) ha scritto:
>> If the field is called "opaque", it's opaque.  
> 
> Sorry?

The field is called "opaque" because no one, except the callbacks,
should look into that field.  Casting it to AliasProperty breaks that rule.

Paolo

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Gonglei (Arei)]

> Subject: Re: [PATCH 0/3] Fix confused output for alias properties
> 
> Il 22/09/2014 15:36, Gonglei (Arei) ha scritto:
> >> If the field is called "opaque", it's opaque.
> >
> > Sorry?
> 
> The field is called "opaque" because no one, except the callbacks,
> should look into that field.  Casting it to AliasProperty breaks that rule.
> 
> Paolo

OK. Thanks for your explanation. I have no idea too. :(

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Gonglei (Arei)]

Hi,

> >>>> This doesn't change the fact that ObjectProperty is a generic struct,
> >>>> and adding alias-specific fields there is wrong.
> >>>
> >>> OK, Maybe I should find other ways to attach this purpose and
> >>> avoid layering violation. Thanks!
> >>
> >> Unfortunately I cannot think of any.
> >>
> >> We could add a description field to ObjectProperty, and replace
> >> legacy_name with a description.  The output then would be
> >>
> >> virtio-blk.drive=str (drive)

There is a question that the QOM properties are added dynamically.
When we call qdev_alias_all_properties() adding alias properties to
the source object all qdev properties on the target DeviceState, how do we
judge the property's name and set the value of corresponding description field?
Such as setting virtio-blk-pci.drive.description = "drive".

The only way I think of is using the property' name as the description. In 
object_property_add_alias() add below code:

+   op->description = g_strdup(name);

After those handling we can get results:

virtio-blk-pci.physical_block_size=uint16 (physical_block_size)
virtio-blk-pci.logical_block_size=uint16 (logical_block_size)
virtio-blk-pci.drive=str (drive)

virtio-net-pci.netdev=str (netdev)
virtio-net-pci.vlan=int (vlan)
virtio-net-pci.mac=str (mac)

But if using this way, we just need simply modify make_device_property_info()
like below patch (just assure the new output resemble the old, don't need change
ObjectProperty struct).  Am I right? Thanks :)


diff --git a/qmp.c b/qmp.c
index c6767c4..1da3e25 100644
--- a/qmp.c
+++ b/qmp.c
@@ -446,6 +446,7 @@ static DevicePropertyInfo *make_device_property_info(ObjectClass *klass,
 {
     DevicePropertyInfo *info;
     Property *prop;
+    char *type_desc = NULL;
 
     do {
         for (prop = DEVICE_CLASS(klass)->props; prop && prop->name; prop++) {
@@ -474,7 +475,9 @@ static DevicePropertyInfo *make_device_property_info(ObjectClass *klass,
     /* Not a qdev property, use the default type */
     info = g_malloc0(sizeof(*info));
     info->name = g_strdup(name);
-    info->type = g_strdup(default_type);
+    type_desc = g_strdup_printf("%s (%s)", default_type, name);
+    info->type = g_strdup(type_desc);
+    g_free(type_desc);
     return info;
 }

Best regards,
-Gonglei

> >> That's a bit hackish, but perhaps it would be good enough for mst and
> >> Markus?
> >
> > I would just drop "str" and replace it with drive.
> 
> In the above example, the format would be
> 
>    CLASS.PROPERTY=TYPE (DESCRIPTION)
> 
> where I just put "drive" as the description to resemble the old output.
> 
> You cannot just have
> 
>    foo.drive=drive
> 
> because the type is *not* "drive", it's "str" (if anything, we could
> make it link<BlockBackend> which does not really show the connection
> between BlockBackend and -drive).
> 
> The problem is that "drive" as the type (the legacy_name) is simply not
> available for the virtio-blk-pci.drive property.  It's hidden beneath an
> opaque pointer.  Gonglei's patches were adding fields to ObjectProperty
> just to avoid visiting that opaque pointer, which I consider a layering
> violation.
> 
> > I seems to convey zero information.
> > Description should be something more informative, e.g.
> > (ID of a drive to use as a backend)
> 
> That would of course be fine too.  The problem is then that we have 644
> properties (DEFINE_PROP_* macro invocations) that someone must add a
> description to.
> 
> > Help for -device could add "Must match ID of a -drive option".
> > Help for device HMP command could add "Must match ID of a drive
> command."
> 
> You are turning a bug that nobody noticed until now, and that really
> doesn't break anything, into a month or more worth of work.
> 
> Paolo

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Michael S. Tsirkin]

On Mon, Sep 22, 2014 at 03:08:09PM +0200, Paolo Bonzini wrote:
> Of course, with every new feature we would most likely have yet another
> unfinished transition.  In the lack of a clear user complaint (or even
> of a clear indication that human users ever used -device foo,help...)
> the tempation to pass the buck is strong.
> 
> Paolo

Well I use it. Or try to, each time I need to build QEMU command line
from scratch. In the end, I usually give up, dig out some old script and
copy/paste from it.

-- 
MST

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Paolo Bonzini]

Il 23/09/2014 05:09, Gonglei (Arei) ha scritto:
> Hi,
> 
>>>>>> This doesn't change the fact that ObjectProperty is a generic struct,
>>>>>> and adding alias-specific fields there is wrong.
>>>>>
>>>>> OK, Maybe I should find other ways to attach this purpose and
>>>>> avoid layering violation. Thanks!
>>>>
>>>> Unfortunately I cannot think of any.
>>>>
>>>> We could add a description field to ObjectProperty, and replace
>>>> legacy_name with a description.  The output then would be
>>>>
>>>> virtio-blk.drive=str (drive)
> 
> There is a question that the QOM properties are added dynamically.
> When we call qdev_alias_all_properties() adding alias properties to
> the source object all qdev properties on the target DeviceState, how do we
> judge the property's name and set the value of corresponding description field?
> Such as setting virtio-blk-pci.drive.description = "drive".

You use the legacy_name field of PropertyInfo to set the description of
a qdev property, and then let object_property_add_alias() copy the
description.

Paolo

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Markus Armbruster]

"Gonglei (Arei)" <arei.gonglei@huawei.com> writes:

> Hi,
>
>> >>>> This doesn't change the fact that ObjectProperty is a generic struct,
>> >>>> and adding alias-specific fields there is wrong.
>> >>>
>> >>> OK, Maybe I should find other ways to attach this purpose and
>> >>> avoid layering violation. Thanks!
>> >>
>> >> Unfortunately I cannot think of any.
>> >>
>> >> We could add a description field to ObjectProperty, and replace
>> >> legacy_name with a description.  The output then would be
>> >>
>> >> virtio-blk.drive=str (drive)
>
> There is a question that the QOM properties are added dynamically.
> When we call qdev_alias_all_properties() adding alias properties to
> the source object all qdev properties on the target DeviceState, how do we
> judge the property's name and set the value of corresponding description field?
> Such as setting virtio-blk-pci.drive.description = "drive".
>
> The only way I think of is using the property' name as the description. In 
> object_property_add_alias() add below code:
>
> +   op->description = g_strdup(name);
>
> After those handling we can get results:
>
> virtio-blk-pci.physical_block_size=uint16 (physical_block_size)
> virtio-blk-pci.logical_block_size=uint16 (logical_block_size)
> virtio-blk-pci.drive=str (drive)
>
> virtio-net-pci.netdev=str (netdev)
> virtio-net-pci.vlan=int (vlan)
> virtio-net-pci.mac=str (mac)
>
> But if using this way, we just need simply modify make_device_property_info()
> like below patch (just assure the new output resemble the old, don't need change
> ObjectProperty struct).  Am I right? Thanks :)

Adding descriptions to properties is a big, but useful task.  The
descriptions can serve as documentation in the code, and they can be
used to provide better help.  This applies both to qdev and to object
properties.

Completing this task in one go is unfortunately not practical.  We need
to add descriptions incrementally.  Until we're done, some properties
will lack descriptions (null pointer rather than a descriptive string).

Reusing the property name as description just to have a description adds
no information.

What about this: add a description string (optional for now) both to
ObjectProperty and to PropertyInfo.  Either add a description string
parameter to object property constructors like object_property_add() and
object_property_add_alias, or, for less churn, add a separate function
to set an object property's description.  Update
qdev_alias_all_properties() to set the object property's description to
the qdev property's.

Then you can fix the help regression by giving all the regressed
properties a useful description.

That fix should also permit retiring legacy_name.

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 23/09/2014 05:09, Gonglei (Arei) ha scritto:
>> Hi,
>> 
>>>>>>> This doesn't change the fact that ObjectProperty is a generic struct,
>>>>>>> and adding alias-specific fields there is wrong.
>>>>>>
>>>>>> OK, Maybe I should find other ways to attach this purpose and
>>>>>> avoid layering violation. Thanks!
>>>>>
>>>>> Unfortunately I cannot think of any.
>>>>>
>>>>> We could add a description field to ObjectProperty, and replace
>>>>> legacy_name with a description.  The output then would be
>>>>>
>>>>> virtio-blk.drive=str (drive)
>> 
>> There is a question that the QOM properties are added dynamically.
>> When we call qdev_alias_all_properties() adding alias properties to
>> the source object all qdev properties on the target DeviceState, how do we
>> judge the property's name and set the value of corresponding
>> description field?
>> Such as setting virtio-blk-pci.drive.description = "drive".
>
> You use the legacy_name field of PropertyInfo to set the description of
> a qdev property, and then let object_property_add_alias() copy the
> description.

Gets us part of the way to what I described in my reply.  I'm fine with
that.

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Michael S. Tsirkin]

On Tue, Sep 23, 2014 at 11:06:02AM +0200, Markus Armbruster wrote:
> "Gonglei (Arei)" <arei.gonglei@huawei.com> writes:
> 
> > Hi,
> >
> >> >>>> This doesn't change the fact that ObjectProperty is a generic struct,
> >> >>>> and adding alias-specific fields there is wrong.
> >> >>>
> >> >>> OK, Maybe I should find other ways to attach this purpose and
> >> >>> avoid layering violation. Thanks!
> >> >>
> >> >> Unfortunately I cannot think of any.
> >> >>
> >> >> We could add a description field to ObjectProperty, and replace
> >> >> legacy_name with a description.  The output then would be
> >> >>
> >> >> virtio-blk.drive=str (drive)
> >
> > There is a question that the QOM properties are added dynamically.
> > When we call qdev_alias_all_properties() adding alias properties to
> > the source object all qdev properties on the target DeviceState, how do we
> > judge the property's name and set the value of corresponding description field?
> > Such as setting virtio-blk-pci.drive.description = "drive".
> >
> > The only way I think of is using the property' name as the description. In 
> > object_property_add_alias() add below code:
> >
> > +   op->description = g_strdup(name);
> >
> > After those handling we can get results:
> >
> > virtio-blk-pci.physical_block_size=uint16 (physical_block_size)
> > virtio-blk-pci.logical_block_size=uint16 (logical_block_size)
> > virtio-blk-pci.drive=str (drive)
> >
> > virtio-net-pci.netdev=str (netdev)
> > virtio-net-pci.vlan=int (vlan)
> > virtio-net-pci.mac=str (mac)
> >
> > But if using this way, we just need simply modify make_device_property_info()
> > like below patch (just assure the new output resemble the old, don't need change
> > ObjectProperty struct).  Am I right? Thanks :)
> 
> Adding descriptions to properties is a big, but useful task.  The
> descriptions can serve as documentation in the code, and they can be
> used to provide better help.  This applies both to qdev and to object
> properties.
> 
> Completing this task in one go is unfortunately not practical.  We need
> to add descriptions incrementally.  Until we're done, some properties
> will lack descriptions (null pointer rather than a descriptive string).
> 
> Reusing the property name as description just to have a description adds
> no information.
> 
> What about this: add a description string (optional for now) both to
> ObjectProperty and to PropertyInfo.  Either add a description string
> parameter to object property constructors like object_property_add() and
> object_property_add_alias, or, for less churn, add a separate function
> to set an object property's description.  Update
> qdev_alias_all_properties() to set the object property's description to
> the qdev property's.
> 
> Then you can fix the help regression by giving all the regressed
> properties a useful description.
> 
> That fix should also permit retiring legacy_name.

Ack. Sounds good to me.

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Gonglei (Arei)]

> -----Original Message-----
> From: Markus Armbruster [mailto:armbru@redhat.com]
> Sent: Tuesday, September 23, 2014 5:06 PM
> To: Gonglei (Arei)
> Cc: Paolo Bonzini; Michael S. Tsirkin; agraf@suse.de; Huangweidong (C);
> stefanha@redhat.com; Huangpeng (Peter); qemu-devel@nongnu.org;
> aliguori@amazon.com; lcapitulino@redhat.com
> Subject: Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties
> 
> "Gonglei (Arei)" <arei.gonglei@huawei.com> writes:
> 
> > Hi,
> >
> >> >>>> This doesn't change the fact that ObjectProperty is a generic struct,
> >> >>>> and adding alias-specific fields there is wrong.
> >> >>>
> >> >>> OK, Maybe I should find other ways to attach this purpose and
> >> >>> avoid layering violation. Thanks!
> >> >>
> >> >> Unfortunately I cannot think of any.
> >> >>
> >> >> We could add a description field to ObjectProperty, and replace
> >> >> legacy_name with a description.  The output then would be
> >> >>
> >> >> virtio-blk.drive=str (drive)
> >
> > There is a question that the QOM properties are added dynamically.
> > When we call qdev_alias_all_properties() adding alias properties to
> > the source object all qdev properties on the target DeviceState, how do we
> > judge the property's name and set the value of corresponding description
> field?
> > Such as setting virtio-blk-pci.drive.description = "drive".
> >
> > The only way I think of is using the property' name as the description. In
> > object_property_add_alias() add below code:
> >
> > +   op->description = g_strdup(name);
> >
> > After those handling we can get results:
> >
> > virtio-blk-pci.physical_block_size=uint16 (physical_block_size)
> > virtio-blk-pci.logical_block_size=uint16 (logical_block_size)
> > virtio-blk-pci.drive=str (drive)
> >
> > virtio-net-pci.netdev=str (netdev)
> > virtio-net-pci.vlan=int (vlan)
> > virtio-net-pci.mac=str (mac)
> >
> > But if using this way, we just need simply modify
> make_device_property_info()
> > like below patch (just assure the new output resemble the old, don't need
> change
> > ObjectProperty struct).  Am I right? Thanks :)
> 
> Adding descriptions to properties is a big, but useful task.  The
> descriptions can serve as documentation in the code, and they can be
> used to provide better help.  This applies both to qdev and to object
> properties.
> 
> Completing this task in one go is unfortunately not practical.  We need
> to add descriptions incrementally.  Until we're done, some properties
> will lack descriptions (null pointer rather than a descriptive string).
> 
> Reusing the property name as description just to have a description adds
> no information.
> 
> What about this: add a description string (optional for now) both to
> ObjectProperty and to PropertyInfo.  Either add a description string
> parameter to object property constructors like object_property_add() and
> object_property_add_alias, or, for less churn, add a separate function
> to set an object property's description.  Update
> qdev_alias_all_properties() to set the object property's description to
> the qdev property's.
> 
> Then you can fix the help regression by giving all the regressed
> properties a useful description.
> 
> That fix should also permit retiring legacy_name.

Great! Thanks, I will work for this :)

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Gonglei (Arei)]

> Subject: Re: [PATCH 0/3] Fix confused output for alias properties
> 
> Il 23/09/2014 05:09, Gonglei (Arei) ha scritto:
> > Hi,
> >
> >>>>>> This doesn't change the fact that ObjectProperty is a generic struct,
> >>>>>> and adding alias-specific fields there is wrong.
> >>>>>
> >>>>> OK, Maybe I should find other ways to attach this purpose and
> >>>>> avoid layering violation. Thanks!
> >>>>
> >>>> Unfortunately I cannot think of any.
> >>>>
> >>>> We could add a description field to ObjectProperty, and replace
> >>>> legacy_name with a description.  The output then would be
> >>>>
> >>>> virtio-blk.drive=str (drive)
> >
> > There is a question that the QOM properties are added dynamically.
> > When we call qdev_alias_all_properties() adding alias properties to
> > the source object all qdev properties on the target DeviceState, how do we
> > judge the property's name and set the value of corresponding description
> field?
> > Such as setting virtio-blk-pci.drive.description = "drive".
> 
> You use the legacy_name field of PropertyInfo to set the description of
> a qdev property, and then let object_property_add_alias() copy the
> description.
> 
> Paolo

Got it. Thanks for your point. :)

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH 0/3] Fix confused output for alias properties

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 22/09/2014 13:43, Markus Armbruster ha scritto:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> Il 22/09/2014 10:34, Michael S. Tsirkin ha scritto:
>>>> Basically this patch brings back historical HMP behaviour.
>>>> As far as I could tell, it wasn't changed intentionally.
>>>
>>> It was changed intentionally.  Or rather, the change was known at the
>>> time Stefan made it.
>> 
>> Commit hash?
>
> There are three commits involved.
>
> The first is commit f4eb32b (qmp: show QOM properties in
> device-list-properties, 2014-05-20).  It was done in preparation for the
> change of virtio-blk-pci.drive from qdev property to QOM property, to
> avoid breaking device-list-properties.

Yes.

Unless there were *no* QOM properties then (other than the implicit ones
listed in the commit message), it also made the help complete again,
which I'd regard as regression fix.

> The actual change took some more time to review, so it went in one month
> later.  Commit caffdac (virtio-blk: use aliases instead of duplicate
> qdev properties, 2014-06-18) is what changed virtio-blk-pci.drive from
> qdev property to QOM property.  This changed the type from 'drive' to
> 'str' in device-list-properties.  (Note that this patch fixed an actual
> bug, it was not just for the sake of cleanup).
>
> I cannot find any reference to the change; perhaps it was discussed only
> on IRC.  However, I'm quite certain that I knew about it.

If memory serves, I didn't.

> Now one thing did slip through; commit caffdac actually dropped
> virtio-blk-pci.drive from -device virtio-blk-pci,help.  Either I
> misremembered that the first commit fixed command-line help too, or I
> just assumed that -device foo,help was implemented on top of
> qmp_device_list_properties.  Which wasn't the case,

Happens.

>                                                     so the third commit
> (9ef52358, qdev-monitor: include QOM properties in -device FOO, help
> output, 2014-07-09) did the right thing and brought back

You mean commit ef52358.

> virtio-blk-pci.drive into the help message.
>
> Now, the cover latter for that patch finally has a hint that the change
> was known.  http://thread.gmane.org/gmane.comp.emulators.qemu/285819 has
> this text:
>
>    We simply need to update -device FOO,help code to use both qdev and
>    QOM properties.  Note that types change because a 'drive' qdev type
>    is actually a 'str' QOM type.  We're moving more and more to QOM
>    properties where the final type for this property would be
>    'link<Drive>' or similar.
>
> It is only in the cover letter and thus not part of any commit message.

I missed it.  Of course I might have missed it even if the commit
message had spelled it out.

The cover letter shows the people involved in the patch were aware of
the help change.  The clause "the final type for this property would be
'link<Drive>' or similar" can perhaps be read as "yes, the change is a
degradation, but it's a temporary one".  Sounds sensible, except our
path to this "final type" is still unclear, and we have no ETA.  Makes
the degradation rather less temporary.

>> I haven't got this part of the code present in my mind at the moment,
>> but I'm willing to take your assertion of a layering violation for
>> granted.  Is this violation necessary for fixing the regression, or is
>> it just an artifact of this particular fix?
>
> I guess we could always add some ad-hoc mechanism for
> device-list-properties to get to the "drive" string, and smuggle it as a
> QOM feature.  Then, aliases would just forward that mechanism to the
> aliased property, which would just work.
>
> Of course, with every new feature we would most likely have yet another
> unfinished transition.  In the lack of a clear user complaint (or even
> of a clear indication that human users ever used -device foo,help...)
> the tempation to pass the buck is strong.

I use it all the time, both interactively to get help, and
programmatically to answer questions about the code or a configuration.

I think elsewhere in this thread, we found a way to improve help that
doesn't involve ad hoc hackery to object.  It does start yet another
transition, though.

Thanks for your explanation, and your patience.