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

[Qemu-devel] [PATCH v3 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 (NULL mapped to empty string):

(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'}

v2->v3:
	Tweaks to patch 4.

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     |  407 +++++++++++++++++++++++++++++++++++++++++++--
 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(+), 62 deletions(-)

-- 
1.7.7.1

[Qemu-devel] [PATCH 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 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 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 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 |   41 ++++++++++++++++++++++++++---------------
 hw/qdev.c            |    5 +----
 hw/qdev.h            |    2 ++
 3 files changed, 29 insertions(+), 19 deletions(-)

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index f0b811c..de618f2 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -614,6 +614,28 @@ int qdev_prop_exists(DeviceState *dev, const char *name)
     return qdev_prop_find(dev, name) ? true : false;
 }
 
+void error_set_from_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;
+    case 0:
+        break;
+    }
+}
+
 int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
 {
     Property *prop;
@@ -632,21 +654,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;
+        error_set_from_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..2a146f7 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1162,10 +1162,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);
-    }
+    error_set_from_qdev_prop_error(errp, ret, dev, prop, ptr);
     g_free(ptr);
 }
 
diff --git a/hw/qdev.h b/hw/qdev.h
index 6e18427..94f5ce9 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 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)
 {
-- 
1.7.7.1

[Qemu-devel] [PATCH 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.

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
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

[Qemu-devel] [PATCH 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).

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-addr.c       |   41 ++++++
 hw/qdev-properties.c |  354 ++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h            |    4 +
 3 files changed, 399 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 de618f2..41d2165 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,57 @@ 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);
+        error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
+        return;
+    }
+    ret = prop->info->parse(dev, prop, str);
+    error_set_from_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 +693,8 @@ PropertyInfo qdev_prop_chr = {
     .size  = sizeof(CharDriverState*),
     .parse = parse_chr,
     .print = print_chr,
+    .get   = get_generic,
+    .set   = set_generic,
 };
 
 /* --- netdev device --- */
@@ -441,6 +729,8 @@ PropertyInfo qdev_prop_netdev = {
     .size  = sizeof(VLANClientState*),
     .parse = parse_netdev,
     .print = print_netdev,
+    .get   = get_generic,
+    .set   = set_generic,
 };
 
 /* --- vlan --- */
@@ -469,12 +759,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 +866,8 @@ PropertyInfo qdev_prop_macaddr = {
     .size  = sizeof(MACAddr),
     .parse = parse_mac,
     .print = print_mac,
+    .get   = get_generic,
+    .set   = set_generic,
 };
 
 /* --- pci address --- */
@@ -570,12 +907,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 94f5ce9..42495ae 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

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

[Paolo Bonzini]

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
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 41d2165..663c2a0 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 2a146f7..c6fcf4c 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;
 }
@@ -1180,7 +1182,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 42495ae..5f23cac 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

[Qemu-devel] [PATCH 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.

For now, all properties come in both variants.  This need not be the
case for string properties.  We will revisit this after -device is
changed to actually use the legacy properties.

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
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 c6fcf4c..0465632 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);
@@ -1172,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 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.
@@ -1180,18 +1185,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 5f23cac..d5896be 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 v3 0/8] qom: introduce non-legacy static properties

[Anthony Liguori]

On 12/18/2011 10:05 AM, Paolo Bonzini wrote:
> 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.

Applied all.  Thanks.

Regards,

Anthony Liguori

>
> 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 (NULL mapped to empty string):
>
> (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'}
>
> v2->v3:
> 	Tweaks to patch 4.
>
> 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     |  407 +++++++++++++++++++++++++++++++++++++++++++--
>   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(+), 62 deletions(-)
>

[Qemu-devel] [PATCH 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-3 are bugfixes and patch 4 is a cleanup, so please apply
those at least.

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': {}}
(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': {}}
(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': {}}
(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'}

Paolo Bonzini (8):
  qapi: fix NULL pointer dereference
  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: 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      |  360 ++++++++++++++++++++++++++++++++++++++++++++-
 hw/qdev.c                 |   85 +++++++-----
 hw/qdev.h                 |   12 +-
 qapi/qmp-input-visitor.c  |   10 +-
 qapi/qmp-output-visitor.c |    4 +-
 qerror.c                  |    5 +
 qerror.h                  |    3 +
 8 files changed, 472 insertions(+), 48 deletions(-)

-- 
1.7.7.1

[Qemu-devel] [PATCH 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 |  348 ++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h            |    4 +
 3 files changed, 393 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 f0b811c..5e8dd9a 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,41 @@ 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) {
+        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 (*ptr) {
+        g_free(*ptr);
+    }
+    *ptr = str;
+}
+
 PropertyInfo qdev_prop_string = {
     .name  = "string",
     .type  = PROP_TYPE_STRING,
@@ -329,6 +561,8 @@ PropertyInfo qdev_prop_string = {
     .parse = parse_string,
     .print = print_string,
     .free  = free_string,
+    .get   = get_string,
+    .set   = set_string,
 };
 
 /* --- drive --- */
@@ -364,12 +598,58 @@ 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);
+
+    /* Same as qdev_get_legacy_property, but do not pass anything if
+     * the property is empty (NULL).
+     */
+    if (*ptr) {
+        char buffer[1024];
+        char *p = buffer;
+
+        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;
+    }
+    ret = prop->info->parse(dev, prop, str);
+    if (ret != 0) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  name, prop->info->name);
+    }
+    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 +687,8 @@ PropertyInfo qdev_prop_chr = {
     .size  = sizeof(CharDriverState*),
     .parse = parse_chr,
     .print = print_chr,
+    .get   = get_generic,
+    .set   = set_generic,
 };
 
 /* --- netdev device --- */
@@ -441,6 +723,8 @@ PropertyInfo qdev_prop_netdev = {
     .size  = sizeof(VLANClientState*),
     .parse = parse_netdev,
     .print = print_netdev,
+    .get   = get_generic,
+    .set   = set_generic,
 };
 
 /* --- vlan --- */
@@ -469,12 +753,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);
+
+    if (*ptr) {
+        int64_t id = (*ptr)->id;
+        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;
+    }
+}
+
 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 +860,8 @@ PropertyInfo qdev_prop_macaddr = {
     .size  = sizeof(MACAddr),
     .parse = parse_mac,
     .print = print_mac,
+    .get   = get_generic,
+    .set   = set_generic,
 };
 
 /* --- pci address --- */
@@ -570,12 +901,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);
+
+    if (*ptr != -1) {
+        char buffer[32];
+        char *p = buffer;
+
+        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 6e18427..9778123 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 6/8] qom: introduce get/set methods for Property

[Gerd Hoffmann]

  Hi,

>  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 +687,8 @@ PropertyInfo qdev_prop_chr = {
>      .size  = sizeof(CharDriverState*),
>      .parse = parse_chr,
>      .print = print_chr,
> +    .get   = get_generic,
> +    .set   = set_generic,
>  };
>  
>  /* --- netdev device --- */
> @@ -441,6 +723,8 @@ PropertyInfo qdev_prop_netdev = {
>      .size  = sizeof(VLANClientState*),
>      .parse = parse_netdev,
>      .print = print_netdev,
> +    .get   = get_generic,
> +    .set   = set_generic,
>  };

>  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 +860,8 @@ PropertyInfo qdev_prop_macaddr = {
>      .size  = sizeof(MACAddr),
>      .parse = parse_mac,
>      .print = print_mac,
> +    .get   = get_generic,
> +    .set   = set_generic,
>  };
>  
>  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,
>  };

I think we should not touch these.  First it doesn't buy us much as we
are using the old parse/print functions for the visitor-based access,
which doesn't look like a good idea to me.  Also they will not stay but
will be converted to child<> and link<>, so I think it is better to keep
the old stuff in the legacy<> namespace.

Agree on the bit/bool/int types.  Although we maybe should apply some
care to integer bus properties, some of them are used for addressing and
will most likely replaced by child<> and link<> too.

cheers,
  Gerd

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

[Paolo Bonzini]

On 12/16/2011 02:11 PM, Gerd Hoffmann wrote:
> I think we should not touch these.  First it doesn't buy us much as we
> are using the old parse/print functions for the visitor-based access,
> which doesn't look like a good idea to me.  Also they will not stay but
> will be converted to child<>  and link<>, so I think it is better to keep
> the old stuff in the legacy<>  namespace.

I thought the same initially.  However, I noticed that the visitor 
interfaces for links is also a string.  So, even if a block/char/netdev 
property later becomes a link<>, the interface would not change.

Using the old parse/print functions and get_set/generic is only to avoid 
code duplication, I can surely inline everything but it would be uglier. 
  And again, I found an example in the code of using a similar adapter 
pattern (the string properties).

There is one case where I had doubts, namely the PCI address properties. 
  They will be replaced by links that you set in the parent.  However, 
in the end I decided to start this way because:

1) QOM properties can still come and go at this stage;

2) The PCI address property can still stay forever as a synthetic 
property declared by PCIDevice, so the "qom-get" ABI won't change.  The 
"qom-set" ABI will, so it might be better to do:

  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   = NULL,
  };

Advantages: it shows that setting the PCI address is (going to be) a 
legacy feature;

Disadvantages: looks a little ad-hoc.  See below for an alternative.

> Agree on the bit/bool/int types.  Although we maybe should apply some
> care to integer bus properties, some of them are used for addressing and
> will most likely replaced by child<>  and link<>  too.

Yes, these will also become synthetic and read-only.  So an alternative 
could be:

    for (prop = dev->info->props; prop && prop->name; prop++) {
        qdev_property_add_legacy(dev, prop, NULL);

        /* Let the generic initializer register alternative definitions
         * for qdev properties.
         */
        if (!qdev_property_find(dev, prop->name) {
            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);
        if (!qdev_property_find(dev, prop->name) {
            qdev_property_add_static(dev, prop, NULL);
        }
    }

For now the pci_devfn property remains read-write, but as soon as the 
PCIDevice will be able to define it as synthetic, it will become read-only.

Paolo

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

[Anthony Liguori]

On 12/16/2011 07:51 AM, Paolo Bonzini wrote:
> On 12/16/2011 02:11 PM, Gerd Hoffmann wrote:
>> I think we should not touch these. First it doesn't buy us much as we
>> are using the old parse/print functions for the visitor-based access,
>> which doesn't look like a good idea to me. Also they will not stay but
>> will be converted to child<> and link<>, so I think it is better to keep
>> the old stuff in the legacy<> namespace.
>
> I thought the same initially. However, I noticed that the visitor interfaces for
> links is also a string. So, even if a block/char/netdev property later becomes a
> link<>, the interface would not change.

The semantics change though.  A "drive" link takes a flat block device name. 
When it's converted to a link, it will take a QOM path.  Since block devices 
will exist in their own directory, it will certainly still be possible to use 
the flat block device name but since a paths will also be supported, I think 
it's best to clearly distinguish the link based property from the flat block 
device name property.

Regards,

Anthony Liguori

>
> Paolo
>

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

[Paolo Bonzini]

On 12/16/2011 03:05 PM, Anthony Liguori wrote:
>>
>> I thought the same initially. However, I noticed that the visitor
>> interfaces for
>> links is also a string. So, even if a block/char/netdev property later
>> becomes a
>> link<>, the interface would not change.
>
> The semantics change though.  A "drive" link takes a flat block device
> name. When it's converted to a link, it will take a QOM path.  Since
> block devices will exist in their own directory, it will certainly still
> be possible to use the flat block device name but since a paths will
> also be supported, I think it's best to clearly distinguish the link
> based property from the flat block device name property.

But it's a superset, no?

Paolo

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

[Anthony Liguori]

On 12/16/2011 08:18 AM, Paolo Bonzini wrote:
> On 12/16/2011 03:05 PM, Anthony Liguori wrote:
>>>
>>> I thought the same initially. However, I noticed that the visitor
>>> interfaces for
>>> links is also a string. So, even if a block/char/netdev property later
>>> becomes a
>>> link<>, the interface would not change.
>>
>> The semantics change though. A "drive" link takes a flat block device
>> name. When it's converted to a link, it will take a QOM path. Since
>> block devices will exist in their own directory, it will certainly still
>> be possible to use the flat block device name but since a paths will
>> also be supported, I think it's best to clearly distinguish the link
>> based property from the flat block device name property.
>
> But it's a superset, no?

My concern is whether you'll get a graceful failure going new->old if you start 
making use of absolute paths.

The type name would change, so I guess that's good enough.

Regards,

Anthony Liguori

> Paolo
>