[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 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 5/8] qom: introduce QERR_PROPERTY_VALUE_OUT_OF_RANGE

[Anthony Liguori]

On 12/16/2011 06:01 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>

I'd rather use generic errors when possible.  How about VALUE_OUT_OF_RANGE and 
we can make the message "'%(item)' doesn't take value..." and pass "%s.%s" % 
(device, property) for item.

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 } }"
>

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

[Paolo Bonzini]

On 12/16/2011 03:00 PM, Anthony Liguori 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>
>
> I'd rather use generic errors when possible.  How about
> VALUE_OUT_OF_RANGE and we can make the message "'%(item)' doesn't take
> value..." and pass "%s.%s" % (device, property) for item.

Ok.

Paolo

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

[Paolo Bonzini]

On 12/16/2011 03:01 PM, Paolo Bonzini wrote:
>>
>> I'd rather use generic errors when possible.  How about
>> VALUE_OUT_OF_RANGE and we can make the message "'%(item)' doesn't take
>> value..." and pass "%s.%s" % (device, property) for item.
>
> Ok.

I didn't do this in the end for two reasons.

First, that it is inconsistent with other errors from qdev properties. 
Current master does not raise them when properties are accessed via QOM, 
but my revised series does.

Second, that it is actually provides less structured information. 
There's no reason why a client should be expected to "know" that %(item) 
is in that form.

Paolo

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

[Anthony Liguori]

On 12/16/2011 11:00 AM, Paolo Bonzini wrote:
> On 12/16/2011 03:01 PM, Paolo Bonzini wrote:
>>>
>>> I'd rather use generic errors when possible. How about
>>> VALUE_OUT_OF_RANGE and we can make the message "'%(item)' doesn't take
>>> value..." and pass "%s.%s" % (device, property) for item.
>>
>> Ok.
>
> I didn't do this in the end for two reasons.
>
> First, that it is inconsistent with other errors from qdev properties. Current
> master does not raise them when properties are accessed via QOM, but my revised
> series does.
>
> Second, that it is actually provides less structured information. There's no
> reason why a client should be expected to "know" that %(item) is in that form.

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

Regards,

Anthony Liguori

> Paolo