[Qemu-devel] [PATCH v2 0/8] qom: introduce non-legacy static properties

[Qemu-devel] [PATCH v2 0/8] qom: introduce non-legacy static properties

[Paolo Bonzini]

QOM right now does not have a way to communicate values for qdev
properties except as strings.  This is bad.

This patch improves the Property implementation so that properties
export a visitor-based interface in addition to the string-based
interface.  The new interface can then be registered as a "static"
property.  It's called static because it uses a struct field for
storage and, as such, should be present in all objects of a given
class.

Patches 1-2 are bugfixes and patches 3-4 are cleanups.

Example using qmp-shell:

x86_64-softmmu/qemu-system-x86_64 \
   -hda ~/test.img -snapshot -S \
   -qmp unix:/tmp/server.sock,server,nowait \
   -netdev type=user,id=net -device virtio-net-pci,netdev=net,id=net \
   -net user,vlan=1 -device virtio-net-pci,id=net2,vlan=1 \
   -chardev id=stdio,backend=stdio -device isa-serial,chardev=stdio,id=serial

Boolean properties:

(QEMU) qom-get path=/i440fx/piix3 property=command_serr_enable
{u'return': True}
(QEMU) qom-get path=/i440fx/piix3 property=legacy<command_serr_enable>
{u'return': u'on'}

PCI address properties (perhaps will disappear later, but not yet):

(QEMU) qom-get path=/i440fx/piix3 property=addr
{u'return': u'01.0'}
(QEMU) qom-get path=/i440fx/piix3 property=legacy<addr>
{u'return': u'01.0'}

String properties (QObject does not have NULL):

(QEMU) qom-get path=/vga property=romfile
{u'return': u'vgabios-cirrus.bin'}
(QEMU) qom-get path=/vga property=legacy<romfile>
{u'return': u'"vgabios-cirrus.bin"'}
(QEMU) qom-get path=/i440fx/piix3 property=romfile
{u'return': u''}
(QEMU) qom-get path=/i440fx/piix3 property=legacy<romfile>
{u'return': u'<null>'}

MAC properties:

(QEMU) qom-get path=/peripheral/net property=mac
{u'return': u'52:54:00:12:34:56'}
(QEMU) qom-get path=/peripheral/net property=legacy<mac>
{u'return': u'52:54:00:12:34:56'}
(QEMU) qom-set path=/peripheral/net property=mac value=52-54-00-12-34-57
{u'error': {u'data': {}, u'class': u'PermissionDenied', u'desc': u'Insufficient permission to perform this operation'}}

Network properties:

(QEMU) qom-get path=/peripheral/net property=netdev
{u'return': u'net'}
(QEMU) qom-get path=/peripheral/net property=legacy<netdev>
{u'return': u'net'}

VLAN properties:

(QEMU) qom-get path=/peripheral/net property=vlan
{u'return': -1}
(QEMU) qom-get path=/peripheral/net property=legacy<vlan>
{u'return': u'<null>'}
(QEMU) qom-get path=/peripheral/net2 property=vlan
{u'return': 1}
(QEMU) qom-get path=/peripheral/net2 property=legacy<vlan>
{u'return': u'1'}

Chardev properties:

(QEMU) qom-get path=/peripheral/serial property=chardev
{u'return': u'stdio'}
(QEMU) qom-get path=/peripheral/serial property=legacy<chardev>
{u'return': u'stdio'}

Legacy hex32 properties:

(QEMU) qom-get path=/peripheral/serial property=iobase
{u'return': 1016}
(QEMU) qom-get path=/peripheral/serial property=legacy<iobase>
{u'return': u'0x3f8'}

Examples of setting properties (after disabling the DEV_STATE_CREATED
check for testing only):

(QEMU) qom-set path=/peripheral/net2 property=vlan value=-1
{u'return': {}}
(QEMU) qom-get path=/peripheral/net2 property=vlan
{u'return': -1}
(QEMU) qom-get path=/peripheral/net2 property=legacy<vlan>
{u'return': u'<null>'}

(QEMU) qom-set path=/peripheral/serial property=iobase value=760
{u'return': {}}
(QEMU) qom-get path=/peripheral/serial property=iobase
{u'return': 760}
(QEMU) qom-get path=/peripheral/serial property=legacy<iobase>
{u'return': u'0x2f8'}

v1->v2:
        New "qom: interpret the return value when setting legacy
        properties".  Always pass a value to the visitor when there
        is no error.  Handle empty strings as NULL.  Did not change
        QERR_PROPERTY_VALUE_OUT_OF_RANGE because it is consistent
        with other QERR_PROPERTY_* errors, now used by QOM too.

Paolo Bonzini (8):
  qapi: protect against NULL QObject in qmp_input_get_object
  qom: fix swapped parameters
  qom: push permission checks up into qdev_property_add_legacy
  qom: interpret the return value when setting legacy properties
  qom: introduce QERR_PROPERTY_VALUE_OUT_OF_RANGE
  qom: introduce get/set methods for Property
  qom: distinguish "legacy" property type name from QOM type name
  qom: register qdev properties also as non-legacy properties

 hw/qdev-addr.c           |   41 +++++
 hw/qdev-properties.c     |  406 +++++++++++++++++++++++++++++++++++++++++++---
 hw/qdev.c                |   84 ++++++----
 hw/qdev.h                |   14 ++-
 qapi/qmp-input-visitor.c |   10 +-
 qerror.c                 |    5 +
 qerror.h                 |    3 +
 7 files changed, 502 insertions(+), 61 deletions(-)

-- 
1.7.7.1

[Qemu-devel] [PATCH v2 1/8] qapi: protect against NULL QObject in qmp_input_get_object

[Paolo Bonzini]

A NULL qobj can occur when a parameter is fetched via qdict_get, but
the parameter is not in the command.  By returning NULL, the caller can
choose whether to raise a missing parameter error, an invalid parameter
type error, or use a default value.  For example, qom-set could can
use this to reset a property to its default value, though at this time
it will fail with "Invalid parameter type".  In any case, anything is
better than crashing!

Reviewed-by: Anthony Liguori <anthony@codemonkey.ws>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qapi/qmp-input-visitor.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 8cbc0ab..c78022b 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -49,10 +49,12 @@ static const QObject *qmp_input_get_object(QmpInputVisitor *qiv,
         qobj = qiv->stack[qiv->nb_stack - 1].obj;
     }
 
-    if (name && qobject_type(qobj) == QTYPE_QDICT) {
-        return qdict_get(qobject_to_qdict(qobj), name);
-    } else if (qiv->nb_stack > 0 && qobject_type(qobj) == QTYPE_QLIST) {
-        return qlist_entry_obj(qiv->stack[qiv->nb_stack - 1].entry);
+    if (qobj) {
+        if (name && qobject_type(qobj) == QTYPE_QDICT) {
+            return qdict_get(qobject_to_qdict(qobj), name);
+        } else if (qiv->nb_stack > 0 && qobject_type(qobj) == QTYPE_QLIST) {
+            return qlist_entry_obj(qiv->stack[qiv->nb_stack - 1].entry);
+        }
     }
 
     return qobj;
-- 
1.7.7.1

[Qemu-devel] [PATCH v2 2/8] qom: fix swapped parameters

[Paolo Bonzini]

Reviewed-by: Anthony Liguori <anthony@codemonkey.ws>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 83913c7..bda8d6c 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1110,7 +1110,7 @@ void qdev_property_set(DeviceState *dev, Visitor *v, const char *name,
     if (!prop->set) {
         error_set(errp, QERR_PERMISSION_DENIED);
     } else {
-        prop->set(dev, prop->opaque, v, name, errp);
+        prop->set(dev, v, prop->opaque, name, errp);
     }
 }
 
-- 
1.7.7.1

[Qemu-devel] [PATCH v2 3/8] qom: push permission checks up into qdev_property_add_legacy

[Paolo Bonzini]

qdev_property_get and qdev_property_set can generate permission
denied errors themselves.  Do not duplicate this functionality in
qdev_get/set_legacy_property, and clean up excessive indentation.

Reviewed-by: Anthony Liguori <anthony@codemonkey.ws>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev.c |   46 +++++++++++++++++++---------------------------
 1 files changed, 19 insertions(+), 27 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index bda8d6c..c020a6f 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1135,46 +1135,38 @@ static void qdev_get_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
 {
     Property *prop = opaque;
 
-    if (prop->info->print) {
-        char buffer[1024];
-        char *ptr = buffer;
+    char buffer[1024];
+    char *ptr = buffer;
 
-        prop->info->print(dev, prop, buffer, sizeof(buffer));
-        visit_type_str(v, &ptr, name, errp);
-    } else {
-        error_set(errp, QERR_PERMISSION_DENIED);
-    }
+    prop->info->print(dev, prop, buffer, sizeof(buffer));
+    visit_type_str(v, &ptr, name, errp);
 }
 
 static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
                                      const char *name, Error **errp)
 {
     Property *prop = opaque;
+    Error *local_err = NULL;
+    char *ptr = NULL;
+    int ret;
 
     if (dev->state != DEV_STATE_CREATED) {
         error_set(errp, QERR_PERMISSION_DENIED);
         return;
     }
 
-    if (prop->info->parse) {
-        Error *local_err = NULL;
-        char *ptr = NULL;
+    visit_type_str(v, &ptr, name, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
 
-        visit_type_str(v, &ptr, name, &local_err);
-        if (!local_err) {
-            int ret;
-            ret = prop->info->parse(dev, prop, ptr);
-            if (ret != 0) {
-                error_set(errp, QERR_INVALID_PARAMETER_VALUE,
-                          name, prop->info->name);
-            }
-            g_free(ptr);
-        } else {
-            error_propagate(errp, local_err);
-        }
-    } else {
-        error_set(errp, QERR_PERMISSION_DENIED);
+    ret = prop->info->parse(dev, prop, ptr);
+    if (ret != 0) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  name, prop->info->name);
     }
+    g_free(ptr);
 }
 
 /**
@@ -1194,8 +1186,8 @@ void qdev_property_add_legacy(DeviceState *dev, Property *prop,
     type = g_strdup_printf("legacy<%s>", prop->info->name);
 
     qdev_property_add(dev, prop->name, type,
-                      qdev_get_legacy_property,
-                      qdev_set_legacy_property,
+                      prop->info->print ? qdev_get_legacy_property : NULL,
+                      prop->info->parse ? qdev_set_legacy_property : NULL,
                       NULL,
                       prop, errp);
 
-- 
1.7.7.1

[Qemu-devel] [PATCH v2 4/8] qom: interpret the return value when setting legacy properties

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-properties.c |   39 ++++++++++++++++++++++++---------------
 hw/qdev.c            |    3 +--
 hw/qdev.h            |    2 ++
 3 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index f0b811c..76ecb38 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -614,6 +614,26 @@ int qdev_prop_exists(DeviceState *dev, const char *name)
     return qdev_prop_find(dev, name) ? true : false;
 }
 
+void qdev_prop_error(Error **errp, int ret,
+                     DeviceState *dev, Property *prop, const char *value)
+{
+    switch (ret) {
+    case -EEXIST:
+        error_set(errp, QERR_PROPERTY_VALUE_IN_USE,
+                  dev->info->name, prop->name, value);
+        break;
+    default:
+    case -EINVAL:
+        error_set(errp, QERR_PROPERTY_VALUE_BAD,
+                  dev->info->name, prop->name, value);
+        break;
+    case -ENOENT:
+        error_set(errp, QERR_PROPERTY_VALUE_NOT_FOUND,
+                  dev->info->name, prop->name, value);
+        break;
+    }
+}
+
 int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
 {
     Property *prop;
@@ -632,21 +652,10 @@ int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
     }
     ret = prop->info->parse(dev, prop, value);
     if (ret < 0) {
-        switch (ret) {
-        case -EEXIST:
-            qerror_report(QERR_PROPERTY_VALUE_IN_USE,
-                          dev->info->name, name, value);
-            break;
-        default:
-        case -EINVAL:
-            qerror_report(QERR_PROPERTY_VALUE_BAD,
-                          dev->info->name, name, value);
-            break;
-        case -ENOENT:
-            qerror_report(QERR_PROPERTY_VALUE_NOT_FOUND,
-                          dev->info->name, name, value);
-            break;
-        }
+        Error *err;
+        qdev_prop_error(&err, ret, dev, prop, value);
+        qerror_report_err(err);
+        error_free(err);
         return -1;
     }
     return 0;
diff --git a/hw/qdev.c b/hw/qdev.c
index c020a6f..c8ab7b7 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1163,8 +1163,7 @@ static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
 
     ret = prop->info->parse(dev, prop, ptr);
     if (ret != 0) {
-        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
-                  name, prop->info->name);
+        qdev_prop_error(errp, ret, dev, prop, ptr);
     }
     g_free(ptr);
 }
diff --git a/hw/qdev.h b/hw/qdev.h
index 6e18427..828d811 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -370,6 +370,8 @@ void qdev_prop_set_defaults(DeviceState *dev, Property *props);
 
 void qdev_prop_register_global_list(GlobalProperty *props);
 void qdev_prop_set_globals(DeviceState *dev);
+void qdev_prop_error(Error **errp, int ret, DeviceState *name,
+                     Property *prop, const char *value);
 
 static inline const char *qdev_fw_name(DeviceState *dev)
 {
-- 
1.7.7.1

Re: [Qemu-devel] [PATCH v2 4/8] qom: interpret the return value when setting legacy properties

[Anthony Liguori]

On 12/16/2011 10:47 AM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   hw/qdev-properties.c |   39 ++++++++++++++++++++++++---------------
>   hw/qdev.c            |    3 +--
>   hw/qdev.h            |    2 ++
>   3 files changed, 27 insertions(+), 17 deletions(-)
>
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index f0b811c..76ecb38 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -614,6 +614,26 @@ int qdev_prop_exists(DeviceState *dev, const char *name)
>       return qdev_prop_find(dev, name) ? true : false;
>   }
>
> +void qdev_prop_error(Error **errp, int ret,
> +                     DeviceState *dev, Property *prop, const char *value)
> +{
> +    switch (ret) {
> +    case -EEXIST:
> +        error_set(errp, QERR_PROPERTY_VALUE_IN_USE,
> +                  dev->info->name, prop->name, value);
> +        break;
> +    default:
> +    case -EINVAL:
> +        error_set(errp, QERR_PROPERTY_VALUE_BAD,
> +                  dev->info->name, prop->name, value);
> +        break;
> +    case -ENOENT:
> +        error_set(errp, QERR_PROPERTY_VALUE_NOT_FOUND,
> +                  dev->info->name, prop->name, value);
> +        break;
> +    }
> +}
> +
>   int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
>   {
>       Property *prop;
> @@ -632,21 +652,10 @@ int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
>       }
>       ret = prop->info->parse(dev, prop, value);
>       if (ret<  0) {
> -        switch (ret) {
> -        case -EEXIST:
> -            qerror_report(QERR_PROPERTY_VALUE_IN_USE,
> -                          dev->info->name, name, value);
> -            break;
> -        default:
> -        case -EINVAL:
> -            qerror_report(QERR_PROPERTY_VALUE_BAD,
> -                          dev->info->name, name, value);
> -            break;
> -        case -ENOENT:
> -            qerror_report(QERR_PROPERTY_VALUE_NOT_FOUND,
> -                          dev->info->name, name, value);
> -            break;
> -        }
> +        Error *err;
> +        qdev_prop_error(&err, ret, dev, prop, value);
> +        qerror_report_err(err);
> +        error_free(err);
>           return -1;
>       }
>       return 0;
> diff --git a/hw/qdev.c b/hw/qdev.c
> index c020a6f..c8ab7b7 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -1163,8 +1163,7 @@ static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
>
>       ret = prop->info->parse(dev, prop, ptr);
>       if (ret != 0) {
> -        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> -                  name, prop->info->name);
> +        qdev_prop_error(errp, ret, dev, prop, ptr);
>       }
>       g_free(ptr);
>   }
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 6e18427..828d811 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -370,6 +370,8 @@ void qdev_prop_set_defaults(DeviceState *dev, Property *props);
>
>   void qdev_prop_register_global_list(GlobalProperty *props);
>   void qdev_prop_set_globals(DeviceState *dev);
> +void qdev_prop_error(Error **errp, int ret, DeviceState *name,
> +                     Property *prop, const char *value);

s/name/dev

And perhaps it would make more sense to return Error * and make the function 
name be a constructor:

Error *error_from_qdev_prop_err(int ret, DeviceState *dev,
                                 Property *prop, const char *value);

I was fairly confused about what was going on here at first.

Reards,

Anthony Liguori

>
>   static inline const char *qdev_fw_name(DeviceState *dev)
>   {

Re: [Qemu-devel] [PATCH v2 4/8] qom: interpret the return value when setting legacy properties

[Paolo Bonzini]

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

On 12/16/2011 06:00 PM, Anthony Liguori wrote:
>
> And perhaps it would make more sense to return Error * and make the
> function name be a constructor:
>
> Error *error_from_qdev_prop_err(int ret, DeviceState *dev,
>                                  Property *prop, const char *value);

That doesn't work too well when calling it from setters.  However, 
error_set_from_qdev_prop_error is definitely an improvement (no matter 
what a mouthful it is).

I need to rush now.  I placed this at git://github.com/qemu/bonzini.git 
qom-props (commit 263e8c5), so you can play on top of it or pull it.  I 
attach the interdiff from v2.

Paolo

[-- Attachment #2: interdiff.patch --]
[-- Type: text/x-patch, Size: 3619 bytes --]

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index dd41e5a..80115b3 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -644,13 +644,11 @@ static void set_generic(DeviceState *dev, Visitor *v, void *opaque,
     }
     if (!*str) {
         g_free(str);
-        qdev_prop_error(errp, EINVAL, dev, prop, str);
+        error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
         return;
     }
     ret = prop->info->parse(dev, prop, str);
-    if (ret != 0) {
-        qdev_prop_error(errp, ret, dev, prop, str);
-    }
+    error_set_from_qdev_prop_error(errp, ret, dev, prop, str);
     g_free(str);
 }
 
@@ -973,8 +972,8 @@ int qdev_prop_exists(DeviceState *dev, const char *name)
     return qdev_prop_find(dev, name) ? true : false;
 }
 
-void qdev_prop_error(Error **errp, int ret,
-                     DeviceState *dev, Property *prop, const char *value)
+void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
+                                    Property *prop, const char *value)
 {
     switch (ret) {
     case -EEXIST:
@@ -990,6 +989,10 @@ void qdev_prop_error(Error **errp, int ret,
         error_set(errp, QERR_PROPERTY_VALUE_NOT_FOUND,
                   dev->info->name, prop->name, value);
         break;
+    case 0:
+        break;
+    default:
+        abort();
     }
 }
 
@@ -1012,7 +1015,7 @@ int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
     ret = prop->info->parse(dev, prop, value);
     if (ret < 0) {
         Error *err;
-        qdev_prop_error(&err, ret, dev, prop, value);
+        error_set_from_qdev_prop_error(&err, ret, dev, prop, value);
         qerror_report_err(err);
         error_free(err);
         return -1;
diff --git a/hw/qdev.c b/hw/qdev.c
index 2f7defc..0465632 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1169,9 +1169,7 @@ static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
     }
 
     ret = prop->info->parse(dev, prop, ptr);
-    if (ret != 0) {
-        qdev_prop_error(errp, ret, dev, prop, ptr);
-    }
+    error_set_from_qdev_prop_error(errp, ret, dev, prop, ptr);
     g_free(ptr);
 }
 
@@ -1179,7 +1177,7 @@ static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
  * @qdev_add_legacy_property - adds a legacy property
  *
  * Do not use this is new code!  Properties added through this interface will
- * be given names and types in the "legacy<>" type namespace.
+ * be given names and types in the "legacy" namespace.
  *
  * Legacy properties are always processed as strings.  The format of the string
  * depends on the property type.
@@ -1189,7 +1187,7 @@ void qdev_property_add_legacy(DeviceState *dev, Property *prop,
 {
     gchar *name, *type;
 
-    name = g_strdup_printf("legacy<%s>", prop->name);
+    name = g_strdup_printf("legacy-%s", prop->name);
     type = g_strdup_printf("legacy<%s>",
                            prop->info->legacy_name ?: prop->info->name);
 
diff --git a/hw/qdev.h b/hw/qdev.h
index 3410e77..d5896be 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -375,8 +375,8 @@ void qdev_prop_set_defaults(DeviceState *dev, Property *props);
 
 void qdev_prop_register_global_list(GlobalProperty *props);
 void qdev_prop_set_globals(DeviceState *dev);
-void qdev_prop_error(Error **errp, int ret, DeviceState *name,
-                     Property *prop, const char *value);
+void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
+                                    Property *prop, const char *value);
 
 static inline const char *qdev_fw_name(DeviceState *dev)
 {

[Qemu-devel] [PATCH v2 5/8] qom: introduce QERR_PROPERTY_VALUE_OUT_OF_RANGE

[Paolo Bonzini]

This will be used when reject invalid values for integer fields that
are less than 64-bits wide.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qerror.c |    5 +++++
 qerror.h |    3 +++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/qerror.c b/qerror.c
index adde8a5..9a75d06 100644
--- a/qerror.c
+++ b/qerror.c
@@ -206,6 +206,11 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Property '%(device).%(property)' can't find value '%(value)'",
     },
     {
+        .error_fmt = QERR_PROPERTY_VALUE_OUT_OF_RANGE,
+        .desc      = "Property '%(device).%(property)' doesn't take "
+                     "value %(value) (minimum: %(min), maximum: %(max)'",
+    },
+    {
         .error_fmt = QERR_QMP_BAD_INPUT_OBJECT,
         .desc      = "Expected '%(expected)' in QMP input",
     },
diff --git a/qerror.h b/qerror.h
index 9190b02..efda232 100644
--- a/qerror.h
+++ b/qerror.h
@@ -171,6 +171,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_PROPERTY_VALUE_NOT_FOUND \
     "{ 'class': 'PropertyValueNotFound', 'data': { 'device': %s, 'property': %s, 'value': %s } }"
 
+#define QERR_PROPERTY_VALUE_OUT_OF_RANGE \
+    "{ 'class': 'PropertyValueOutOfRange', 'data': { 'device': %s, 'property': %s, 'value': %"PRId64", 'min': %"PRId64", 'max': %"PRId64" } }"
+
 #define QERR_QMP_BAD_INPUT_OBJECT \
     "{ 'class': 'QMPBadInputObject', 'data': { 'expected': %s } }"
 
-- 
1.7.7.1

Re: [Qemu-devel] [PATCH v2 5/8] qom: introduce QERR_PROPERTY_VALUE_OUT_OF_RANGE

[Anthony Liguori]

On 12/16/2011 10:47 AM, Paolo Bonzini wrote:
> This will be used when reject invalid values for integer fields that
> are less than 64-bits wide.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>

VALUE_OUT_OF_RANGE?

Regards,

Anthony Liguori

> ---
>   qerror.c |    5 +++++
>   qerror.h |    3 +++
>   2 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/qerror.c b/qerror.c
> index adde8a5..9a75d06 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -206,6 +206,11 @@ static const QErrorStringTable qerror_table[] = {
>           .desc      = "Property '%(device).%(property)' can't find value '%(value)'",
>       },
>       {
> +        .error_fmt = QERR_PROPERTY_VALUE_OUT_OF_RANGE,
> +        .desc      = "Property '%(device).%(property)' doesn't take "
> +                     "value %(value) (minimum: %(min), maximum: %(max)'",
> +    },
> +    {
>           .error_fmt = QERR_QMP_BAD_INPUT_OBJECT,
>           .desc      = "Expected '%(expected)' in QMP input",
>       },
> diff --git a/qerror.h b/qerror.h
> index 9190b02..efda232 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -171,6 +171,9 @@ QError *qobject_to_qerror(const QObject *obj);
>   #define QERR_PROPERTY_VALUE_NOT_FOUND \
>       "{ 'class': 'PropertyValueNotFound', 'data': { 'device': %s, 'property': %s, 'value': %s } }"
>
> +#define QERR_PROPERTY_VALUE_OUT_OF_RANGE \
> +    "{ 'class': 'PropertyValueOutOfRange', 'data': { 'device': %s, 'property': %s, 'value': %"PRId64", 'min': %"PRId64", 'max': %"PRId64" } }"
> +
>   #define QERR_QMP_BAD_INPUT_OBJECT \
>       "{ 'class': 'QMPBadInputObject', 'data': { 'expected': %s } }"
>

[Qemu-devel] [PATCH v2 6/8] qom: introduce get/set methods for Property

[Paolo Bonzini]

This patch adds a visitor interface to Property.  This way, QOM will be
able to expose Properties that access a fixed field in a struct without
exposing also the everything-is-a-string "feature" of qdev properties.

Whenever the printed representation in both QOM and qdev (which is
typically the case for device backends), parse/print code can be reused
via get_generic/set_generic.  Dually, whenever multiple PropertyInfos
have the same representation in both the struct and the visitors the
code can be reused (for example among all of int32/uint32/hex32).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-addr.c       |   41 ++++++
 hw/qdev-properties.c |  355 ++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h            |    4 +
 3 files changed, 400 insertions(+), 0 deletions(-)

diff --git a/hw/qdev-addr.c b/hw/qdev-addr.c
index 305c2d3..5ddda2d 100644
--- a/hw/qdev-addr.c
+++ b/hw/qdev-addr.c
@@ -18,12 +18,53 @@ static int print_taddr(DeviceState *dev, Property *prop, char *dest, size_t len)
     return snprintf(dest, len, "0x" TARGET_FMT_plx, *ptr);
 }
 
+static void get_taddr(DeviceState *dev, Visitor *v, void *opaque,
+                      const char *name, Error **errp)
+{
+    Property *prop = opaque;
+    target_phys_addr_t *ptr = qdev_get_prop_ptr(dev, prop);
+    int64_t value;
+
+    value = *ptr;
+    visit_type_int(v, &value, name, errp);
+}
+
+static void set_taddr(DeviceState *dev, Visitor *v, void *opaque,
+                      const char *name, Error **errp)
+{
+    Property *prop = opaque;
+    target_phys_addr_t *ptr = qdev_get_prop_ptr(dev, prop);
+    Error *local_err = NULL;
+    int64_t value;
+
+    if (dev->state != DEV_STATE_CREATED) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+        return;
+    }
+
+    visit_type_int(v, &value, name, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    if ((uint64_t)value <= (uint64_t) ~(target_phys_addr_t)0) {
+        *ptr = value;
+    } else {
+        error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
+                  dev->id?:"", name, value, (uint64_t) 0,
+                  (uint64_t) ~(target_phys_addr_t)0);
+    }
+}
+
+
 PropertyInfo qdev_prop_taddr = {
     .name  = "taddr",
     .type  = PROP_TYPE_TADDR,
     .size  = sizeof(target_phys_addr_t),
     .parse = parse_taddr,
     .print = print_taddr,
+    .get   = get_taddr,
+    .set   = set_taddr,
 };
 
 void qdev_prop_set_taddr(DeviceState *dev, const char *name, target_phys_addr_t value)
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 76ecb38..f7aa3cb 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -55,12 +55,44 @@ static int print_bit(DeviceState *dev, Property *prop, char *dest, size_t len)
     return snprintf(dest, len, (*p & qdev_get_prop_mask(prop)) ? "on" : "off");
 }
 
+static void get_bit(DeviceState *dev, Visitor *v, void *opaque,
+                    const char *name, Error **errp)
+{
+    Property *prop = opaque;
+    uint32_t *p = qdev_get_prop_ptr(dev, prop);
+    bool value = (*p & qdev_get_prop_mask(prop)) != 0;
+
+    visit_type_bool(v, &value, name, errp);
+}
+
+static void set_bit(DeviceState *dev, Visitor *v, void *opaque,
+                    const char *name, Error **errp)
+{
+    Property *prop = opaque;
+    Error *local_err = NULL;
+    bool value;
+
+    if (dev->state != DEV_STATE_CREATED) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+        return;
+    }
+
+    visit_type_bool(v, &value, name, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    bit_prop_set(dev, prop, value);
+}
+
 PropertyInfo qdev_prop_bit = {
     .name  = "on/off",
     .type  = PROP_TYPE_BIT,
     .size  = sizeof(uint32_t),
     .parse = parse_bit,
     .print = print_bit,
+    .get   = get_bit,
+    .set   = set_bit,
 };
 
 /* --- 8bit integer --- */
@@ -85,12 +117,54 @@ static int print_uint8(DeviceState *dev, Property *prop, char *dest, size_t len)
     return snprintf(dest, len, "%" PRIu8, *ptr);
 }
 
+static void get_int8(DeviceState *dev, Visitor *v, void *opaque,
+                     const char *name, Error **errp)
+{
+    Property *prop = opaque;
+    int8_t *ptr = qdev_get_prop_ptr(dev, prop);
+    int64_t value;
+
+    value = *ptr;
+    visit_type_int(v, &value, name, errp);
+}
+
+static void set_int8(DeviceState *dev, Visitor *v, void *opaque,
+                      const char *name, Error **errp)
+{
+    Property *prop = opaque;
+    int8_t *ptr = qdev_get_prop_ptr(dev, prop);
+    Error *local_err = NULL;
+    int64_t value;
+
+    if (dev->state != DEV_STATE_CREATED) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+        return;
+    }
+
+    visit_type_int(v, &value, name, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    if (value > prop->info->min && value <= prop->info->max) {
+        *ptr = value;
+    } else {
+        error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
+                  dev->id?:"", name, value, prop->info->min,
+                  prop->info->max);
+    }
+}
+
 PropertyInfo qdev_prop_uint8 = {
     .name  = "uint8",
     .type  = PROP_TYPE_UINT8,
     .size  = sizeof(uint8_t),
     .parse = parse_uint8,
     .print = print_uint8,
+    .get   = get_int8,
+    .set   = set_int8,
+    .min   = 0,
+    .max   = 255,
 };
 
 /* --- 8bit hex value --- */
@@ -120,6 +194,10 @@ PropertyInfo qdev_prop_hex8 = {
     .size  = sizeof(uint8_t),
     .parse = parse_hex8,
     .print = print_hex8,
+    .get   = get_int8,
+    .set   = set_int8,
+    .min   = 0,
+    .max   = 255,
 };
 
 /* --- 16bit integer --- */
@@ -144,12 +222,54 @@ static int print_uint16(DeviceState *dev, Property *prop, char *dest, size_t len
     return snprintf(dest, len, "%" PRIu16, *ptr);
 }
 
+static void get_int16(DeviceState *dev, Visitor *v, void *opaque,
+                      const char *name, Error **errp)
+{
+    Property *prop = opaque;
+    int16_t *ptr = qdev_get_prop_ptr(dev, prop);
+    int64_t value;
+
+    value = *ptr;
+    visit_type_int(v, &value, name, errp);
+}
+
+static void set_int16(DeviceState *dev, Visitor *v, void *opaque,
+                      const char *name, Error **errp)
+{
+    Property *prop = opaque;
+    int16_t *ptr = qdev_get_prop_ptr(dev, prop);
+    Error *local_err = NULL;
+    int64_t value;
+
+    if (dev->state != DEV_STATE_CREATED) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+        return;
+    }
+
+    visit_type_int(v, &value, name, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    if (value > prop->info->min && value <= prop->info->max) {
+        *ptr = value;
+    } else {
+        error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
+                  dev->id?:"", name, value, prop->info->min,
+                  prop->info->max);
+    }
+}
+
 PropertyInfo qdev_prop_uint16 = {
     .name  = "uint16",
     .type  = PROP_TYPE_UINT16,
     .size  = sizeof(uint16_t),
     .parse = parse_uint16,
     .print = print_uint16,
+    .get   = get_int16,
+    .set   = set_int16,
+    .min   = 0,
+    .max   = 65535,
 };
 
 /* --- 32bit integer --- */
@@ -174,12 +294,54 @@ static int print_uint32(DeviceState *dev, Property *prop, char *dest, size_t len
     return snprintf(dest, len, "%" PRIu32, *ptr);
 }
 
+static void get_int32(DeviceState *dev, Visitor *v, void *opaque,
+                      const char *name, Error **errp)
+{
+    Property *prop = opaque;
+    int32_t *ptr = qdev_get_prop_ptr(dev, prop);
+    int64_t value;
+
+    value = *ptr;
+    visit_type_int(v, &value, name, errp);
+}
+
+static void set_int32(DeviceState *dev, Visitor *v, void *opaque,
+                      const char *name, Error **errp)
+{
+    Property *prop = opaque;
+    int32_t *ptr = qdev_get_prop_ptr(dev, prop);
+    Error *local_err = NULL;
+    int64_t value;
+
+    if (dev->state != DEV_STATE_CREATED) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+        return;
+    }
+
+    visit_type_int(v, &value, name, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    if (value > prop->info->min && value <= prop->info->max) {
+        *ptr = value;
+    } else {
+        error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
+                  dev->id?:"", name, value, prop->info->min,
+                  prop->info->max);
+    }
+}
+
 PropertyInfo qdev_prop_uint32 = {
     .name  = "uint32",
     .type  = PROP_TYPE_UINT32,
     .size  = sizeof(uint32_t),
     .parse = parse_uint32,
     .print = print_uint32,
+    .get   = get_int32,
+    .set   = set_int32,
+    .min   = 0,
+    .max   = 0xFFFFFFFFULL,
 };
 
 static int parse_int32(DeviceState *dev, Property *prop, const char *str)
@@ -207,6 +369,10 @@ PropertyInfo qdev_prop_int32 = {
     .size  = sizeof(int32_t),
     .parse = parse_int32,
     .print = print_int32,
+    .get   = get_int32,
+    .set   = set_int32,
+    .min   = -0x80000000LL,
+    .max   = 0x7FFFFFFFLL,
 };
 
 /* --- 32bit hex value --- */
@@ -236,6 +402,10 @@ PropertyInfo qdev_prop_hex32 = {
     .size  = sizeof(uint32_t),
     .parse = parse_hex32,
     .print = print_hex32,
+    .get   = get_int32,
+    .set   = set_int32,
+    .min   = 0,
+    .max   = 0xFFFFFFFFULL,
 };
 
 /* --- 64bit integer --- */
@@ -260,12 +430,37 @@ static int print_uint64(DeviceState *dev, Property *prop, char *dest, size_t len
     return snprintf(dest, len, "%" PRIu64, *ptr);
 }
 
+static void get_int64(DeviceState *dev, Visitor *v, void *opaque,
+                      const char *name, Error **errp)
+{
+    Property *prop = opaque;
+    int64_t *ptr = qdev_get_prop_ptr(dev, prop);
+
+    visit_type_int(v, ptr, name, errp);
+}
+
+static void set_int64(DeviceState *dev, Visitor *v, void *opaque,
+                      const char *name, Error **errp)
+{
+    Property *prop = opaque;
+    int64_t *ptr = qdev_get_prop_ptr(dev, prop);
+
+    if (dev->state != DEV_STATE_CREATED) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+        return;
+    }
+
+    visit_type_int(v, ptr, name, errp);
+}
+
 PropertyInfo qdev_prop_uint64 = {
     .name  = "uint64",
     .type  = PROP_TYPE_UINT64,
     .size  = sizeof(uint64_t),
     .parse = parse_uint64,
     .print = print_uint64,
+    .get   = get_int64,
+    .set   = set_int64,
 };
 
 /* --- 64bit hex value --- */
@@ -295,6 +490,8 @@ PropertyInfo qdev_prop_hex64 = {
     .size  = sizeof(uint64_t),
     .parse = parse_hex64,
     .print = print_hex64,
+    .get   = get_int64,
+    .set   = set_int64,
 };
 
 /* --- string --- */
@@ -322,6 +519,48 @@ static int print_string(DeviceState *dev, Property *prop, char *dest, size_t len
     return snprintf(dest, len, "\"%s\"", *ptr);
 }
 
+static void get_string(DeviceState *dev, Visitor *v, void *opaque,
+                       const char *name, Error **errp)
+{
+    Property *prop = opaque;
+    char **ptr = qdev_get_prop_ptr(dev, prop);
+
+    if (!*ptr) {
+        char *str = (char *)"";
+        visit_type_str(v, &str, name, errp);
+    } else {
+        visit_type_str(v, ptr, name, errp);
+    }
+}
+
+static void set_string(DeviceState *dev, Visitor *v, void *opaque,
+                       const char *name, Error **errp)
+{
+    Property *prop = opaque;
+    char **ptr = qdev_get_prop_ptr(dev, prop);
+    Error *local_err = NULL;
+    char *str;
+
+    if (dev->state != DEV_STATE_CREATED) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+        return;
+    }
+
+    visit_type_str(v, &str, name, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    if (!*str) {
+        g_free(str);
+        str = NULL;
+    }
+    if (*ptr) {
+        g_free(*ptr);
+    }
+    *ptr = str;
+}
+
 PropertyInfo qdev_prop_string = {
     .name  = "string",
     .type  = PROP_TYPE_STRING,
@@ -329,6 +568,8 @@ PropertyInfo qdev_prop_string = {
     .parse = parse_string,
     .print = print_string,
     .free  = free_string,
+    .get   = get_string,
+    .set   = set_string,
 };
 
 /* --- drive --- */
@@ -364,12 +605,59 @@ static int print_drive(DeviceState *dev, Property *prop, char *dest, size_t len)
                     *ptr ? bdrv_get_device_name(*ptr) : "<null>");
 }
 
+static void get_generic(DeviceState *dev, Visitor *v, void *opaque,
+                       const char *name, Error **errp)
+{
+    Property *prop = opaque;
+    void **ptr = qdev_get_prop_ptr(dev, prop);
+    char buffer[1024];
+    char *p = buffer;
+
+    buffer[0] = 0;
+    if (*ptr) {
+        prop->info->print(dev, prop, buffer, sizeof(buffer));
+    }
+    visit_type_str(v, &p, name, errp);
+}
+
+static void set_generic(DeviceState *dev, Visitor *v, void *opaque,
+                        const char *name, Error **errp)
+{
+    Property *prop = opaque;
+    Error *local_err = NULL;
+    char *str;
+    int ret;
+
+    if (dev->state != DEV_STATE_CREATED) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+        return;
+    }
+
+    visit_type_str(v, &str, name, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    if (!*str) {
+        g_free(str);
+        qdev_prop_error(errp, EINVAL, dev, prop, str);
+        return;
+    }
+    ret = prop->info->parse(dev, prop, str);
+    if (ret != 0) {
+        qdev_prop_error(errp, ret, dev, prop, str);
+    }
+    g_free(str);
+}
+
 PropertyInfo qdev_prop_drive = {
     .name  = "drive",
     .type  = PROP_TYPE_DRIVE,
     .size  = sizeof(BlockDriverState *),
     .parse = parse_drive,
     .print = print_drive,
+    .get   = get_generic,
+    .set   = set_generic,
     .free  = free_drive,
 };
 
@@ -407,6 +695,8 @@ PropertyInfo qdev_prop_chr = {
     .size  = sizeof(CharDriverState*),
     .parse = parse_chr,
     .print = print_chr,
+    .get   = get_generic,
+    .set   = set_generic,
 };
 
 /* --- netdev device --- */
@@ -441,6 +731,8 @@ PropertyInfo qdev_prop_netdev = {
     .size  = sizeof(VLANClientState*),
     .parse = parse_netdev,
     .print = print_netdev,
+    .get   = get_generic,
+    .set   = set_generic,
 };
 
 /* --- vlan --- */
@@ -469,12 +761,57 @@ static int print_vlan(DeviceState *dev, Property *prop, char *dest, size_t len)
     }
 }
 
+static void get_vlan(DeviceState *dev, Visitor *v, void *opaque,
+                     const char *name, Error **errp)
+{
+    Property *prop = opaque;
+    VLANState **ptr = qdev_get_prop_ptr(dev, prop);
+    int64_t id;
+
+    id = *ptr ? (*ptr)->id : -1;
+    visit_type_int(v, &id, name, errp);
+}
+
+static void set_vlan(DeviceState *dev, Visitor *v, void *opaque,
+                     const char *name, Error **errp)
+{
+    Property *prop = opaque;
+    VLANState **ptr = qdev_get_prop_ptr(dev, prop);
+    Error *local_err = NULL;
+    int64_t id;
+    VLANState *vlan;
+
+    if (dev->state != DEV_STATE_CREATED) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+        return;
+    }
+
+    visit_type_int(v, &id, name, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    if (id == -1) {
+        *ptr = NULL;
+        return;
+    }
+    vlan = qemu_find_vlan(id, 1);
+    if (!vlan) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  name, prop->info->name);
+        return;
+    }
+    *ptr = vlan;
+}
+
 PropertyInfo qdev_prop_vlan = {
     .name  = "vlan",
     .type  = PROP_TYPE_VLAN,
     .size  = sizeof(VLANClientState*),
     .parse = parse_vlan,
     .print = print_vlan,
+    .get   = get_vlan,
+    .set   = set_vlan,
 };
 
 /* --- pointer --- */
@@ -531,6 +867,8 @@ PropertyInfo qdev_prop_macaddr = {
     .size  = sizeof(MACAddr),
     .parse = parse_mac,
     .print = print_mac,
+    .get   = get_generic,
+    .set   = set_generic,
 };
 
 /* --- pci address --- */
@@ -570,12 +908,29 @@ static int print_pci_devfn(DeviceState *dev, Property *prop, char *dest, size_t
     }
 }
 
+static void get_pci_devfn(DeviceState *dev, Visitor *v, void *opaque,
+                          const char *name, Error **errp)
+{
+    Property *prop = opaque;
+    uint32_t *ptr = qdev_get_prop_ptr(dev, prop);
+    char buffer[32];
+    char *p = buffer;
+
+    buffer[0] = 0;
+    if (*ptr != -1) {
+        snprintf(buffer, sizeof(buffer), "%02x.%x", *ptr >> 3, *ptr & 7);
+    }
+    visit_type_str(v, &p, name, errp);
+}
+
 PropertyInfo qdev_prop_pci_devfn = {
     .name  = "pci-devfn",
     .type  = PROP_TYPE_UINT32,
     .size  = sizeof(uint32_t),
     .parse = parse_pci_devfn,
     .print = print_pci_devfn,
+    .get   = get_pci_devfn,
+    .set   = set_generic,
 };
 
 /* --- public helpers --- */
diff --git a/hw/qdev.h b/hw/qdev.h
index 828d811..a1cce37 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -158,9 +158,13 @@ struct PropertyInfo {
     const char *name;
     size_t size;
     enum PropertyType type;
+    int64_t min;
+    int64_t max;
     int (*parse)(DeviceState *dev, Property *prop, const char *str);
     int (*print)(DeviceState *dev, Property *prop, char *dest, size_t len);
     void (*free)(DeviceState *dev, Property *prop);
+    DevicePropertyAccessor *get;
+    DevicePropertyAccessor *set;
 };
 
 typedef struct GlobalProperty {
-- 
1.7.7.1

Re: [Qemu-devel] [PATCH v2 6/8] qom: introduce get/set methods for Property

[Anthony Liguori]

On 12/16/2011 10:47 AM, Paolo Bonzini wrote:
> This patch adds a visitor interface to Property.  This way, QOM will be
> able to expose Properties that access a fixed field in a struct without
> exposing also the everything-is-a-string "feature" of qdev properties.
>
> Whenever the printed representation in both QOM and qdev (which is
> typically the case for device backends), parse/print code can be reused
> via get_generic/set_generic.  Dually, whenever multiple PropertyInfos
> have the same representation in both the struct and the visitors the
> code can be reused (for example among all of int32/uint32/hex32).
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>


Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>   hw/qdev-addr.c       |   41 ++++++
>   hw/qdev-properties.c |  355 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   hw/qdev.h            |    4 +
>   3 files changed, 400 insertions(+), 0 deletions(-)
>
> diff --git a/hw/qdev-addr.c b/hw/qdev-addr.c
> index 305c2d3..5ddda2d 100644
> --- a/hw/qdev-addr.c
> +++ b/hw/qdev-addr.c
> @@ -18,12 +18,53 @@ static int print_taddr(DeviceState *dev, Property *prop, char *dest, size_t len)
>       return snprintf(dest, len, "0x" TARGET_FMT_plx, *ptr);
>   }
>
> +static void get_taddr(DeviceState *dev, Visitor *v, void *opaque,
> +                      const char *name, Error **errp)
> +{
> +    Property *prop = opaque;
> +    target_phys_addr_t *ptr = qdev_get_prop_ptr(dev, prop);
> +    int64_t value;
> +
> +    value = *ptr;
> +    visit_type_int(v,&value, name, errp);
> +}
> +
> +static void set_taddr(DeviceState *dev, Visitor *v, void *opaque,
> +                      const char *name, Error **errp)
> +{
> +    Property *prop = opaque;
> +    target_phys_addr_t *ptr = qdev_get_prop_ptr(dev, prop);
> +    Error *local_err = NULL;
> +    int64_t value;
> +
> +    if (dev->state != DEV_STATE_CREATED) {
> +        error_set(errp, QERR_PERMISSION_DENIED);
> +        return;
> +    }
> +
> +    visit_type_int(v,&value, name,&local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    if ((uint64_t)value<= (uint64_t) ~(target_phys_addr_t)0) {
> +        *ptr = value;
> +    } else {
> +        error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
> +                  dev->id?:"", name, value, (uint64_t) 0,
> +                  (uint64_t) ~(target_phys_addr_t)0);
> +    }
> +}
> +
> +
>   PropertyInfo qdev_prop_taddr = {
>       .name  = "taddr",
>       .type  = PROP_TYPE_TADDR,
>       .size  = sizeof(target_phys_addr_t),
>       .parse = parse_taddr,
>       .print = print_taddr,
> +    .get   = get_taddr,
> +    .set   = set_taddr,
>   };
>
>   void qdev_prop_set_taddr(DeviceState *dev, const char *name, target_phys_addr_t value)
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index 76ecb38..f7aa3cb 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -55,12 +55,44 @@ static int print_bit(DeviceState *dev, Property *prop, char *dest, size_t len)
>       return snprintf(dest, len, (*p&  qdev_get_prop_mask(prop)) ? "on" : "off");
>   }
>
> +static void get_bit(DeviceState *dev, Visitor *v, void *opaque,
> +                    const char *name, Error **errp)
> +{
> +    Property *prop = opaque;
> +    uint32_t *p = qdev_get_prop_ptr(dev, prop);
> +    bool value = (*p&  qdev_get_prop_mask(prop)) != 0;
> +
> +    visit_type_bool(v,&value, name, errp);
> +}
> +
> +static void set_bit(DeviceState *dev, Visitor *v, void *opaque,
> +                    const char *name, Error **errp)
> +{
> +    Property *prop = opaque;
> +    Error *local_err = NULL;
> +    bool value;
> +
> +    if (dev->state != DEV_STATE_CREATED) {
> +        error_set(errp, QERR_PERMISSION_DENIED);
> +        return;
> +    }
> +
> +    visit_type_bool(v,&value, name,&local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    bit_prop_set(dev, prop, value);
> +}
> +
>   PropertyInfo qdev_prop_bit = {
>       .name  = "on/off",
>       .type  = PROP_TYPE_BIT,
>       .size  = sizeof(uint32_t),
>       .parse = parse_bit,
>       .print = print_bit,
> +    .get   = get_bit,
> +    .set   = set_bit,
>   };
>
>   /* --- 8bit integer --- */
> @@ -85,12 +117,54 @@ static int print_uint8(DeviceState *dev, Property *prop, char *dest, size_t len)
>       return snprintf(dest, len, "%" PRIu8, *ptr);
>   }
>
> +static void get_int8(DeviceState *dev, Visitor *v, void *opaque,
> +                     const char *name, Error **errp)
> +{
> +    Property *prop = opaque;
> +    int8_t *ptr = qdev_get_prop_ptr(dev, prop);
> +    int64_t value;
> +
> +    value = *ptr;
> +    visit_type_int(v,&value, name, errp);
> +}
> +
> +static void set_int8(DeviceState *dev, Visitor *v, void *opaque,
> +                      const char *name, Error **errp)
> +{
> +    Property *prop = opaque;
> +    int8_t *ptr = qdev_get_prop_ptr(dev, prop);
> +    Error *local_err = NULL;
> +    int64_t value;
> +
> +    if (dev->state != DEV_STATE_CREATED) {
> +        error_set(errp, QERR_PERMISSION_DENIED);
> +        return;
> +    }
> +
> +    visit_type_int(v,&value, name,&local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    if (value>  prop->info->min&&  value<= prop->info->max) {
> +        *ptr = value;
> +    } else {
> +        error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
> +                  dev->id?:"", name, value, prop->info->min,
> +                  prop->info->max);
> +    }
> +}
> +
>   PropertyInfo qdev_prop_uint8 = {
>       .name  = "uint8",
>       .type  = PROP_TYPE_UINT8,
>       .size  = sizeof(uint8_t),
>       .parse = parse_uint8,
>       .print = print_uint8,
> +    .get   = get_int8,
> +    .set   = set_int8,
> +    .min   = 0,
> +    .max   = 255,
>   };
>
>   /* --- 8bit hex value --- */
> @@ -120,6 +194,10 @@ PropertyInfo qdev_prop_hex8 = {
>       .size  = sizeof(uint8_t),
>       .parse = parse_hex8,
>       .print = print_hex8,
> +    .get   = get_int8,
> +    .set   = set_int8,
> +    .min   = 0,
> +    .max   = 255,
>   };
>
>   /* --- 16bit integer --- */
> @@ -144,12 +222,54 @@ static int print_uint16(DeviceState *dev, Property *prop, char *dest, size_t len
>       return snprintf(dest, len, "%" PRIu16, *ptr);
>   }
>
> +static void get_int16(DeviceState *dev, Visitor *v, void *opaque,
> +                      const char *name, Error **errp)
> +{
> +    Property *prop = opaque;
> +    int16_t *ptr = qdev_get_prop_ptr(dev, prop);
> +    int64_t value;
> +
> +    value = *ptr;
> +    visit_type_int(v,&value, name, errp);
> +}
> +
> +static void set_int16(DeviceState *dev, Visitor *v, void *opaque,
> +                      const char *name, Error **errp)
> +{
> +    Property *prop = opaque;
> +    int16_t *ptr = qdev_get_prop_ptr(dev, prop);
> +    Error *local_err = NULL;
> +    int64_t value;
> +
> +    if (dev->state != DEV_STATE_CREATED) {
> +        error_set(errp, QERR_PERMISSION_DENIED);
> +        return;
> +    }
> +
> +    visit_type_int(v,&value, name,&local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    if (value>  prop->info->min&&  value<= prop->info->max) {
> +        *ptr = value;
> +    } else {
> +        error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
> +                  dev->id?:"", name, value, prop->info->min,
> +                  prop->info->max);
> +    }
> +}
> +
>   PropertyInfo qdev_prop_uint16 = {
>       .name  = "uint16",
>       .type  = PROP_TYPE_UINT16,
>       .size  = sizeof(uint16_t),
>       .parse = parse_uint16,
>       .print = print_uint16,
> +    .get   = get_int16,
> +    .set   = set_int16,
> +    .min   = 0,
> +    .max   = 65535,
>   };
>
>   /* --- 32bit integer --- */
> @@ -174,12 +294,54 @@ static int print_uint32(DeviceState *dev, Property *prop, char *dest, size_t len
>       return snprintf(dest, len, "%" PRIu32, *ptr);
>   }
>
> +static void get_int32(DeviceState *dev, Visitor *v, void *opaque,
> +                      const char *name, Error **errp)
> +{
> +    Property *prop = opaque;
> +    int32_t *ptr = qdev_get_prop_ptr(dev, prop);
> +    int64_t value;
> +
> +    value = *ptr;
> +    visit_type_int(v,&value, name, errp);
> +}
> +
> +static void set_int32(DeviceState *dev, Visitor *v, void *opaque,
> +                      const char *name, Error **errp)
> +{
> +    Property *prop = opaque;
> +    int32_t *ptr = qdev_get_prop_ptr(dev, prop);
> +    Error *local_err = NULL;
> +    int64_t value;
> +
> +    if (dev->state != DEV_STATE_CREATED) {
> +        error_set(errp, QERR_PERMISSION_DENIED);
> +        return;
> +    }
> +
> +    visit_type_int(v,&value, name,&local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    if (value>  prop->info->min&&  value<= prop->info->max) {
> +        *ptr = value;
> +    } else {
> +        error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
> +                  dev->id?:"", name, value, prop->info->min,
> +                  prop->info->max);
> +    }
> +}
> +
>   PropertyInfo qdev_prop_uint32 = {
>       .name  = "uint32",
>       .type  = PROP_TYPE_UINT32,
>       .size  = sizeof(uint32_t),
>       .parse = parse_uint32,
>       .print = print_uint32,
> +    .get   = get_int32,
> +    .set   = set_int32,
> +    .min   = 0,
> +    .max   = 0xFFFFFFFFULL,
>   };
>
>   static int parse_int32(DeviceState *dev, Property *prop, const char *str)
> @@ -207,6 +369,10 @@ PropertyInfo qdev_prop_int32 = {
>       .size  = sizeof(int32_t),
>       .parse = parse_int32,
>       .print = print_int32,
> +    .get   = get_int32,
> +    .set   = set_int32,
> +    .min   = -0x80000000LL,
> +    .max   = 0x7FFFFFFFLL,
>   };
>
>   /* --- 32bit hex value --- */
> @@ -236,6 +402,10 @@ PropertyInfo qdev_prop_hex32 = {
>       .size  = sizeof(uint32_t),
>       .parse = parse_hex32,
>       .print = print_hex32,
> +    .get   = get_int32,
> +    .set   = set_int32,
> +    .min   = 0,
> +    .max   = 0xFFFFFFFFULL,
>   };
>
>   /* --- 64bit integer --- */
> @@ -260,12 +430,37 @@ static int print_uint64(DeviceState *dev, Property *prop, char *dest, size_t len
>       return snprintf(dest, len, "%" PRIu64, *ptr);
>   }
>
> +static void get_int64(DeviceState *dev, Visitor *v, void *opaque,
> +                      const char *name, Error **errp)
> +{
> +    Property *prop = opaque;
> +    int64_t *ptr = qdev_get_prop_ptr(dev, prop);
> +
> +    visit_type_int(v, ptr, name, errp);
> +}
> +
> +static void set_int64(DeviceState *dev, Visitor *v, void *opaque,
> +                      const char *name, Error **errp)
> +{
> +    Property *prop = opaque;
> +    int64_t *ptr = qdev_get_prop_ptr(dev, prop);
> +
> +    if (dev->state != DEV_STATE_CREATED) {
> +        error_set(errp, QERR_PERMISSION_DENIED);
> +        return;
> +    }
> +
> +    visit_type_int(v, ptr, name, errp);
> +}
> +
>   PropertyInfo qdev_prop_uint64 = {
>       .name  = "uint64",
>       .type  = PROP_TYPE_UINT64,
>       .size  = sizeof(uint64_t),
>       .parse = parse_uint64,
>       .print = print_uint64,
> +    .get   = get_int64,
> +    .set   = set_int64,
>   };
>
>   /* --- 64bit hex value --- */
> @@ -295,6 +490,8 @@ PropertyInfo qdev_prop_hex64 = {
>       .size  = sizeof(uint64_t),
>       .parse = parse_hex64,
>       .print = print_hex64,
> +    .get   = get_int64,
> +    .set   = set_int64,
>   };
>
>   /* --- string --- */
> @@ -322,6 +519,48 @@ static int print_string(DeviceState *dev, Property *prop, char *dest, size_t len
>       return snprintf(dest, len, "\"%s\"", *ptr);
>   }
>
> +static void get_string(DeviceState *dev, Visitor *v, void *opaque,
> +                       const char *name, Error **errp)
> +{
> +    Property *prop = opaque;
> +    char **ptr = qdev_get_prop_ptr(dev, prop);
> +
> +    if (!*ptr) {
> +        char *str = (char *)"";
> +        visit_type_str(v,&str, name, errp);
> +    } else {
> +        visit_type_str(v, ptr, name, errp);
> +    }
> +}
> +
> +static void set_string(DeviceState *dev, Visitor *v, void *opaque,
> +                       const char *name, Error **errp)
> +{
> +    Property *prop = opaque;
> +    char **ptr = qdev_get_prop_ptr(dev, prop);
> +    Error *local_err = NULL;
> +    char *str;
> +
> +    if (dev->state != DEV_STATE_CREATED) {
> +        error_set(errp, QERR_PERMISSION_DENIED);
> +        return;
> +    }
> +
> +    visit_type_str(v,&str, name,&local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    if (!*str) {
> +        g_free(str);
> +        str = NULL;
> +    }
> +    if (*ptr) {
> +        g_free(*ptr);
> +    }
> +    *ptr = str;
> +}
> +
>   PropertyInfo qdev_prop_string = {
>       .name  = "string",
>       .type  = PROP_TYPE_STRING,
> @@ -329,6 +568,8 @@ PropertyInfo qdev_prop_string = {
>       .parse = parse_string,
>       .print = print_string,
>       .free  = free_string,
> +    .get   = get_string,
> +    .set   = set_string,
>   };
>
>   /* --- drive --- */
> @@ -364,12 +605,59 @@ static int print_drive(DeviceState *dev, Property *prop, char *dest, size_t len)
>                       *ptr ? bdrv_get_device_name(*ptr) : "<null>");
>   }
>
> +static void get_generic(DeviceState *dev, Visitor *v, void *opaque,
> +                       const char *name, Error **errp)
> +{
> +    Property *prop = opaque;
> +    void **ptr = qdev_get_prop_ptr(dev, prop);
> +    char buffer[1024];
> +    char *p = buffer;
> +
> +    buffer[0] = 0;
> +    if (*ptr) {
> +        prop->info->print(dev, prop, buffer, sizeof(buffer));
> +    }
> +    visit_type_str(v,&p, name, errp);
> +}
> +
> +static void set_generic(DeviceState *dev, Visitor *v, void *opaque,
> +                        const char *name, Error **errp)
> +{
> +    Property *prop = opaque;
> +    Error *local_err = NULL;
> +    char *str;
> +    int ret;
> +
> +    if (dev->state != DEV_STATE_CREATED) {
> +        error_set(errp, QERR_PERMISSION_DENIED);
> +        return;
> +    }
> +
> +    visit_type_str(v,&str, name,&local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    if (!*str) {
> +        g_free(str);
> +        qdev_prop_error(errp, EINVAL, dev, prop, str);
> +        return;
> +    }
> +    ret = prop->info->parse(dev, prop, str);
> +    if (ret != 0) {
> +        qdev_prop_error(errp, ret, dev, prop, str);
> +    }
> +    g_free(str);
> +}
> +
>   PropertyInfo qdev_prop_drive = {
>       .name  = "drive",
>       .type  = PROP_TYPE_DRIVE,
>       .size  = sizeof(BlockDriverState *),
>       .parse = parse_drive,
>       .print = print_drive,
> +    .get   = get_generic,
> +    .set   = set_generic,
>       .free  = free_drive,
>   };
>
> @@ -407,6 +695,8 @@ PropertyInfo qdev_prop_chr = {
>       .size  = sizeof(CharDriverState*),
>       .parse = parse_chr,
>       .print = print_chr,
> +    .get   = get_generic,
> +    .set   = set_generic,
>   };
>
>   /* --- netdev device --- */
> @@ -441,6 +731,8 @@ PropertyInfo qdev_prop_netdev = {
>       .size  = sizeof(VLANClientState*),
>       .parse = parse_netdev,
>       .print = print_netdev,
> +    .get   = get_generic,
> +    .set   = set_generic,
>   };
>
>   /* --- vlan --- */
> @@ -469,12 +761,57 @@ static int print_vlan(DeviceState *dev, Property *prop, char *dest, size_t len)
>       }
>   }
>
> +static void get_vlan(DeviceState *dev, Visitor *v, void *opaque,
> +                     const char *name, Error **errp)
> +{
> +    Property *prop = opaque;
> +    VLANState **ptr = qdev_get_prop_ptr(dev, prop);
> +    int64_t id;
> +
> +    id = *ptr ? (*ptr)->id : -1;
> +    visit_type_int(v,&id, name, errp);
> +}
> +
> +static void set_vlan(DeviceState *dev, Visitor *v, void *opaque,
> +                     const char *name, Error **errp)
> +{
> +    Property *prop = opaque;
> +    VLANState **ptr = qdev_get_prop_ptr(dev, prop);
> +    Error *local_err = NULL;
> +    int64_t id;
> +    VLANState *vlan;
> +
> +    if (dev->state != DEV_STATE_CREATED) {
> +        error_set(errp, QERR_PERMISSION_DENIED);
> +        return;
> +    }
> +
> +    visit_type_int(v,&id, name,&local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    if (id == -1) {
> +        *ptr = NULL;
> +        return;
> +    }
> +    vlan = qemu_find_vlan(id, 1);
> +    if (!vlan) {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> +                  name, prop->info->name);
> +        return;
> +    }
> +    *ptr = vlan;
> +}
> +
>   PropertyInfo qdev_prop_vlan = {
>       .name  = "vlan",
>       .type  = PROP_TYPE_VLAN,
>       .size  = sizeof(VLANClientState*),
>       .parse = parse_vlan,
>       .print = print_vlan,
> +    .get   = get_vlan,
> +    .set   = set_vlan,
>   };
>
>   /* --- pointer --- */
> @@ -531,6 +867,8 @@ PropertyInfo qdev_prop_macaddr = {
>       .size  = sizeof(MACAddr),
>       .parse = parse_mac,
>       .print = print_mac,
> +    .get   = get_generic,
> +    .set   = set_generic,
>   };
>
>   /* --- pci address --- */
> @@ -570,12 +908,29 @@ static int print_pci_devfn(DeviceState *dev, Property *prop, char *dest, size_t
>       }
>   }
>
> +static void get_pci_devfn(DeviceState *dev, Visitor *v, void *opaque,
> +                          const char *name, Error **errp)
> +{
> +    Property *prop = opaque;
> +    uint32_t *ptr = qdev_get_prop_ptr(dev, prop);
> +    char buffer[32];
> +    char *p = buffer;
> +
> +    buffer[0] = 0;
> +    if (*ptr != -1) {
> +        snprintf(buffer, sizeof(buffer), "%02x.%x", *ptr>>  3, *ptr&  7);
> +    }
> +    visit_type_str(v,&p, name, errp);
> +}
> +
>   PropertyInfo qdev_prop_pci_devfn = {
>       .name  = "pci-devfn",
>       .type  = PROP_TYPE_UINT32,
>       .size  = sizeof(uint32_t),
>       .parse = parse_pci_devfn,
>       .print = print_pci_devfn,
> +    .get   = get_pci_devfn,
> +    .set   = set_generic,
>   };
>
>   /* --- public helpers --- */
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 828d811..a1cce37 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -158,9 +158,13 @@ struct PropertyInfo {
>       const char *name;
>       size_t size;
>       enum PropertyType type;
> +    int64_t min;
> +    int64_t max;
>       int (*parse)(DeviceState *dev, Property *prop, const char *str);
>       int (*print)(DeviceState *dev, Property *prop, char *dest, size_t len);
>       void (*free)(DeviceState *dev, Property *prop);
> +    DevicePropertyAccessor *get;
> +    DevicePropertyAccessor *set;
>   };
>
>   typedef struct GlobalProperty {

[Qemu-devel] [PATCH v2 7/8] qom: distinguish "legacy" property type name from QOM type name

[Paolo Bonzini]

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 f7aa3cb..dd41e5a 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 c8ab7b7..426ea71 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;
 }
@@ -1182,7 +1184,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 a1cce37..8002644 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;
-- 
1.7.7.1

Re: [Qemu-devel] [PATCH v2 7/8] qom: distinguish "legacy" property type name from QOM type name

[Anthony Liguori]

On 12/16/2011 10:47 AM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>   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 f7aa3cb..dd41e5a 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 c8ab7b7..426ea71 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;
>   }
> @@ -1182,7 +1184,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 a1cce37..8002644 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;

[Qemu-devel] [PATCH v2 8/8] qom: register qdev properties also as non-legacy properties

[Paolo Bonzini]

Push legacy properties into a legacy<...> namespace, and make them
available with correct types too.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev.c |   28 +++++++++++++++++++++++++---
 hw/qdev.h |    7 +++----
 2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 426ea71..2f7defc 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -80,6 +80,9 @@ static DeviceInfo *qdev_find_info(BusInfo *bus_info, const char *name)
     return NULL;
 }
 
+static void qdev_property_add_legacy(DeviceState *dev, Property *prop,
+                                     Error **errp);
+
 static DeviceState *qdev_create_from_info(BusState *bus, DeviceInfo *info)
 {
     DeviceState *dev;
@@ -104,10 +107,12 @@ static DeviceState *qdev_create_from_info(BusState *bus, DeviceInfo *info)
 
     for (prop = dev->info->props; prop && prop->name; prop++) {
         qdev_property_add_legacy(dev, prop, NULL);
+        qdev_property_add_static(dev, prop, NULL);
     }
 
     for (prop = dev->info->bus_info->props; prop && prop->name; prop++) {
         qdev_property_add_legacy(dev, prop, NULL);
+        qdev_property_add_static(dev, prop, NULL);
     }
 
     qdev_property_add_str(dev, "type", qdev_get_type, NULL, NULL);
@@ -1174,7 +1179,7 @@ static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
  * @qdev_add_legacy_property - adds a legacy property
  *
  * Do not use this is new code!  Properties added through this interface will
- * be given types in the "legacy<>" type namespace.
+ * be given names and types in the "legacy<>" type namespace.
  *
  * Legacy properties are always processed as strings.  The format of the string
  * depends on the property type.
@@ -1182,18 +1187,35 @@ static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
 void qdev_property_add_legacy(DeviceState *dev, Property *prop,
                               Error **errp)
 {
-    gchar *type;
+    gchar *name, *type;
 
+    name = g_strdup_printf("legacy<%s>", prop->name);
     type = g_strdup_printf("legacy<%s>",
                            prop->info->legacy_name ?: prop->info->name);
 
-    qdev_property_add(dev, prop->name, type,
+    qdev_property_add(dev, name, type,
                       prop->info->print ? qdev_get_legacy_property : NULL,
                       prop->info->parse ? qdev_set_legacy_property : NULL,
                       NULL,
                       prop, errp);
 
     g_free(type);
+    g_free(name);
+}
+
+/**
+ * @qdev_property_add_static - add a @Property to a device.
+ *
+ * Static properties access data in a struct.  The actual type of the
+ * property and the field depends on the property type.
+ */
+void qdev_property_add_static(DeviceState *dev, Property *prop,
+                              Error **errp)
+{
+    qdev_property_add(dev, prop->name, prop->info->name,
+                      prop->info->get, prop->info->set,
+                      NULL,
+                      prop, errp);
 }
 
 DeviceState *qdev_get_root(void)
diff --git a/hw/qdev.h b/hw/qdev.h
index 8002644..3410e77 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -487,11 +487,10 @@ const char *qdev_property_get_type(DeviceState *dev, const char *name,
                                    Error **errp);
 
 /**
- * @qdev_property_add_legacy - add a legacy @Property to a device
- *
- * DO NOT USE THIS IN NEW CODE!
+ * @qdev_property_add_static - add a @Property to a device referencing a
+ * field in a struct.
  */
-void qdev_property_add_legacy(DeviceState *dev, Property *prop, Error **errp);
+void qdev_property_add_static(DeviceState *dev, Property *prop, Error **errp);
 
 /**
  * @qdev_get_root - returns the root device of the composition tree
-- 
1.7.7.1

Re: [Qemu-devel] [PATCH v2 8/8] qom: register qdev properties also as non-legacy properties

[Anthony Liguori]

On 12/16/2011 10:47 AM, Paolo Bonzini wrote:
> Push legacy properties into a legacy<...>  namespace, and make them
> available with correct types too.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   hw/qdev.c |   28 +++++++++++++++++++++++++---
>   hw/qdev.h |    7 +++----
>   2 files changed, 28 insertions(+), 7 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 426ea71..2f7defc 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -80,6 +80,9 @@ static DeviceInfo *qdev_find_info(BusInfo *bus_info, const char *name)
>       return NULL;
>   }
>
> +static void qdev_property_add_legacy(DeviceState *dev, Property *prop,
> +                                     Error **errp);
> +
>   static DeviceState *qdev_create_from_info(BusState *bus, DeviceInfo *info)
>   {
>       DeviceState *dev;
> @@ -104,10 +107,12 @@ static DeviceState *qdev_create_from_info(BusState *bus, DeviceInfo *info)
>
>       for (prop = dev->info->props; prop&&  prop->name; prop++) {
>           qdev_property_add_legacy(dev, prop, NULL);
> +        qdev_property_add_static(dev, prop, NULL);
>       }
>
>       for (prop = dev->info->bus_info->props; prop&&  prop->name; prop++) {
>           qdev_property_add_legacy(dev, prop, NULL);
> +        qdev_property_add_static(dev, prop, NULL);
>       }
>
>       qdev_property_add_str(dev, "type", qdev_get_type, NULL, NULL);
> @@ -1174,7 +1179,7 @@ static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
>    * @qdev_add_legacy_property - adds a legacy property
>    *
>    * Do not use this is new code!  Properties added through this interface will
> - * be given types in the "legacy<>" type namespace.
> + * be given names and types in the "legacy<>" type namespace.
>    *
>    * Legacy properties are always processed as strings.  The format of the string
>    * depends on the property type.
> @@ -1182,18 +1187,35 @@ static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
>   void qdev_property_add_legacy(DeviceState *dev, Property *prop,
>                                 Error **errp)
>   {
> -    gchar *type;
> +    gchar *name, *type;
>
> +    name = g_strdup_printf("legacy<%s>", prop->name);

Okay, let's make this name legacy-%s to make it clear that it's not type. 
Otherwise, the patch looks good.

Regards,

Anthony Liguori


>       type = g_strdup_printf("legacy<%s>",
>                              prop->info->legacy_name ?: prop->info->name);
>
> -    qdev_property_add(dev, prop->name, type,
> +    qdev_property_add(dev, name, type,
>                         prop->info->print ? qdev_get_legacy_property : NULL,
>                         prop->info->parse ? qdev_set_legacy_property : NULL,
>                         NULL,
>                         prop, errp);
>
>       g_free(type);
> +    g_free(name);
> +}
> +
> +/**
> + * @qdev_property_add_static - add a @Property to a device.
> + *
> + * Static properties access data in a struct.  The actual type of the
> + * property and the field depends on the property type.
> + */
> +void qdev_property_add_static(DeviceState *dev, Property *prop,
> +                              Error **errp)
> +{
> +    qdev_property_add(dev, prop->name, prop->info->name,
> +                      prop->info->get, prop->info->set,
> +                      NULL,
> +                      prop, errp);
>   }
>
>   DeviceState *qdev_get_root(void)
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 8002644..3410e77 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -487,11 +487,10 @@ const char *qdev_property_get_type(DeviceState *dev, const char *name,
>                                      Error **errp);
>
>   /**
> - * @qdev_property_add_legacy - add a legacy @Property to a device
> - *
> - * DO NOT USE THIS IN NEW CODE!
> + * @qdev_property_add_static - add a @Property to a device referencing a
> + * field in a struct.
>    */
> -void qdev_property_add_legacy(DeviceState *dev, Property *prop, Error **errp);
> +void qdev_property_add_static(DeviceState *dev, Property *prop, Error **errp);
>
>   /**
>    * @qdev_get_root - returns the root device of the composition tree