Patchwork [7/8] qom: distinguish "legacy" property type name from QOM type name

login
register
mail settings
Submitter Paolo Bonzini
Date Dec. 16, 2011, 12:01 p.m.
Message ID <1324036918-2405-8-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/131813/
State New
Headers show

Comments

Paolo Bonzini - Dec. 16, 2011, 12:01 p.m.
For non-string properties, there is no reason to distinguish type names
such as "uint32" or "hex32".  Restrict those to legacy properties.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-properties.c |   12 ++++++++----
 hw/qdev.c            |    9 ++++++---
 hw/qdev.h            |    1 +
 3 files changed, 15 insertions(+), 7 deletions(-)
Anthony Liguori - Dec. 16, 2011, 2:06 p.m.
On 12/16/2011 06:01 AM, Paolo Bonzini wrote:
> For non-string properties, there is no reason to distinguish type names
> such as "uint32" or "hex32".  Restrict those to legacy properties.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   hw/qdev-properties.c |   12 ++++++++----
>   hw/qdev.c            |    9 ++++++---
>   hw/qdev.h            |    1 +
>   3 files changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index 5e8dd9a..6b6732e 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -86,7 +86,8 @@ static void set_bit(DeviceState *dev, Visitor *v, void *opaque,
>   }
>
>   PropertyInfo qdev_prop_bit = {
> -    .name  = "on/off",
> +    .name  = "boolean",
> +    .legacy_name  = "on/off",
>       .type  = PROP_TYPE_BIT,
>       .size  = sizeof(uint32_t),
>       .parse = parse_bit,
> @@ -189,7 +190,8 @@ static int print_hex8(DeviceState *dev, Property *prop, char *dest, size_t len)
>   }
>
>   PropertyInfo qdev_prop_hex8 = {
> -    .name  = "hex8",
> +    .name  = "uint8",
> +    .legacy_name  = "hex8",
>       .type  = PROP_TYPE_UINT8,
>       .size  = sizeof(uint8_t),
>       .parse = parse_hex8,
> @@ -397,7 +399,8 @@ static int print_hex32(DeviceState *dev, Property *prop, char *dest, size_t len)
>   }
>
>   PropertyInfo qdev_prop_hex32 = {
> -    .name  = "hex32",
> +    .name  = "uint32",
> +    .legacy_name  = "hex32",
>       .type  = PROP_TYPE_UINT32,
>       .size  = sizeof(uint32_t),
>       .parse = parse_hex32,
> @@ -485,7 +488,8 @@ static int print_hex64(DeviceState *dev, Property *prop, char *dest, size_t len)
>   }
>
>   PropertyInfo qdev_prop_hex64 = {
> -    .name  = "hex64",
> +    .name  = "uint64",
> +    .legacy_name  = "hex64",
>       .type  = PROP_TYPE_UINT64,
>       .size  = sizeof(uint64_t),
>       .parse = parse_hex64,
> diff --git a/hw/qdev.c b/hw/qdev.c
> index c020a6f..d76861e 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -218,13 +218,15 @@ int qdev_device_help(QemuOpts *opts)
>           if (!prop->info->parse) {
>               continue;           /* no way to set it, don't show */
>           }
> -        error_printf("%s.%s=%s\n", info->name, prop->name, prop->info->name);
> +        error_printf("%s.%s=%s\n", info->name, prop->name,
> +                     prop->info->legacy_name ?: prop->info->name);
>       }
>       for (prop = info->bus_info->props; prop&&  prop->name; prop++) {
>           if (!prop->info->parse) {
>               continue;           /* no way to set it, don't show */
>           }
> -        error_printf("%s.%s=%s\n", info->name, prop->name, prop->info->name);
> +        error_printf("%s.%s=%s\n", info->name, prop->name,
> +                     prop->info->legacy_name ?: prop->info->name);
>       }
>       return 1;
>   }
> @@ -1183,7 +1185,8 @@ void qdev_property_add_legacy(DeviceState *dev, Property *prop,
>   {
>       gchar *type;
>
> -    type = g_strdup_printf("legacy<%s>", prop->info->name);
> +    type = g_strdup_printf("legacy<%s>",
> +                           prop->info->legacy_name ?: prop->info->name);

I think this confuses the legacy type with the legacy property names.

I think it would be better to do 'legacy-%s' as then it's at least clear when 
something is a type name vs. a property name.

Regards,

Anthony Liguori

>
>       qdev_property_add(dev, prop->name, type,
>                         prop->info->print ? qdev_get_legacy_property : NULL,
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 9778123..c7d9535 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -156,6 +156,7 @@ enum PropertyType {
>
>   struct PropertyInfo {
>       const char *name;
> +    const char *legacy_name;
>       size_t size;
>       enum PropertyType type;
>       int64_t min;
Paolo Bonzini - Dec. 16, 2011, 2:18 p.m.
On 12/16/2011 03:06 PM, Anthony Liguori wrote:
>>
>> -    type = g_strdup_printf("legacy<%s>", prop->info->name);
>> +    type = g_strdup_printf("legacy<%s>",
>> +                           prop->info->legacy_name ?: prop->info->name);
>
> I think this confuses the legacy type with the legacy property names.
>
> I think it would be better to do 'legacy-%s' as then it's at least clear
> when something is a type name vs. a property name.

That's only in 8/8.  Here I'm not changing property names yet, note that 
everything is in prop->info.

This patch prepares for when you'll have a legacy<hex32> type for 
property legacy<iobase>, and uint32 for iobase.  But I can surely rename 
legacy<iobase> to legacy-iobase, if that's what you meant.

paolo
Anthony Liguori - Dec. 16, 2011, 2:43 p.m.
On 12/16/2011 08:18 AM, Paolo Bonzini wrote:
> On 12/16/2011 03:06 PM, Anthony Liguori wrote:
>>>
>>> - type = g_strdup_printf("legacy<%s>", prop->info->name);
>>> + type = g_strdup_printf("legacy<%s>",
>>> + prop->info->legacy_name ?: prop->info->name);
>>
>> I think this confuses the legacy type with the legacy property names.
>>
>> I think it would be better to do 'legacy-%s' as then it's at least clear
>> when something is a type name vs. a property name.
>
> That's only in 8/8. Here I'm not changing property names yet, note that
> everything is in prop->info.
>
> This patch prepares for when you'll have a legacy<hex32> type for property
> legacy<iobase>, and uint32 for iobase. But I can surely rename legacy<iobase> to
> legacy-iobase, if that's what you meant.

No, I got confused.  Ignore my comments.

Regards,

Anthony Liguori

>
> paolo
>

Patch

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 5e8dd9a..6b6732e 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -86,7 +86,8 @@  static void set_bit(DeviceState *dev, Visitor *v, void *opaque,
 }
 
 PropertyInfo qdev_prop_bit = {
-    .name  = "on/off",
+    .name  = "boolean",
+    .legacy_name  = "on/off",
     .type  = PROP_TYPE_BIT,
     .size  = sizeof(uint32_t),
     .parse = parse_bit,
@@ -189,7 +190,8 @@  static int print_hex8(DeviceState *dev, Property *prop, char *dest, size_t len)
 }
 
 PropertyInfo qdev_prop_hex8 = {
-    .name  = "hex8",
+    .name  = "uint8",
+    .legacy_name  = "hex8",
     .type  = PROP_TYPE_UINT8,
     .size  = sizeof(uint8_t),
     .parse = parse_hex8,
@@ -397,7 +399,8 @@  static int print_hex32(DeviceState *dev, Property *prop, char *dest, size_t len)
 }
 
 PropertyInfo qdev_prop_hex32 = {
-    .name  = "hex32",
+    .name  = "uint32",
+    .legacy_name  = "hex32",
     .type  = PROP_TYPE_UINT32,
     .size  = sizeof(uint32_t),
     .parse = parse_hex32,
@@ -485,7 +488,8 @@  static int print_hex64(DeviceState *dev, Property *prop, char *dest, size_t len)
 }
 
 PropertyInfo qdev_prop_hex64 = {
-    .name  = "hex64",
+    .name  = "uint64",
+    .legacy_name  = "hex64",
     .type  = PROP_TYPE_UINT64,
     .size  = sizeof(uint64_t),
     .parse = parse_hex64,
diff --git a/hw/qdev.c b/hw/qdev.c
index c020a6f..d76861e 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -218,13 +218,15 @@  int qdev_device_help(QemuOpts *opts)
         if (!prop->info->parse) {
             continue;           /* no way to set it, don't show */
         }
-        error_printf("%s.%s=%s\n", info->name, prop->name, prop->info->name);
+        error_printf("%s.%s=%s\n", info->name, prop->name,
+                     prop->info->legacy_name ?: prop->info->name);
     }
     for (prop = info->bus_info->props; prop && prop->name; prop++) {
         if (!prop->info->parse) {
             continue;           /* no way to set it, don't show */
         }
-        error_printf("%s.%s=%s\n", info->name, prop->name, prop->info->name);
+        error_printf("%s.%s=%s\n", info->name, prop->name,
+                     prop->info->legacy_name ?: prop->info->name);
     }
     return 1;
 }
@@ -1183,7 +1185,8 @@  void qdev_property_add_legacy(DeviceState *dev, Property *prop,
 {
     gchar *type;
 
-    type = g_strdup_printf("legacy<%s>", prop->info->name);
+    type = g_strdup_printf("legacy<%s>",
+                           prop->info->legacy_name ?: prop->info->name);
 
     qdev_property_add(dev, prop->name, type,
                       prop->info->print ? qdev_get_legacy_property : NULL,
diff --git a/hw/qdev.h b/hw/qdev.h
index 9778123..c7d9535 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -156,6 +156,7 @@  enum PropertyType {
 
 struct PropertyInfo {
     const char *name;
+    const char *legacy_name;
     size_t size;
     enum PropertyType type;
     int64_t min;