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

[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 1/8] qapi: fix NULL pointer dereference

[Paolo Bonzini]

QAPI currently cannot deal with no object pushed to the stack,
and dereferences a NULL pointer.  This is visible with

    qom-get path=/i440fx/piix3 property=romfile

after static non-string properties are introduced.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qapi/qmp-output-visitor.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index f76d015..29575da 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -65,13 +65,13 @@ static QObject *qmp_output_pop(QmpOutputVisitor *qov)
 static QObject *qmp_output_first(QmpOutputVisitor *qov)
 {
     QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack);
-    return e->value;
+    return e ? e->value : NULL;
 }
 
 static QObject *qmp_output_last(QmpOutputVisitor *qov)
 {
     QStackEntry *e = QTAILQ_FIRST(&qov->stack);
-    return e->value;
+    return e ? e->value : NULL;
 }
 
 static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name,
-- 
1.7.7.1

[Qemu-devel] [PATCH 2/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!

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 3/8] qom: fix swapped parameters

[Paolo Bonzini]

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 4/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.

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

[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

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

[Paolo Bonzini]

For non-string properties, there is no reason to distinguish type names
such as "uint32" or "hex32".  Restrict those to legacy properties.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-properties.c |   12 ++++++++----
 hw/qdev.c            |    9 ++++++---
 hw/qdev.h            |    1 +
 3 files changed, 15 insertions(+), 7 deletions(-)

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

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 d76861e..e09f540 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);
@@ -1175,7 +1180,7 @@ static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
  * @qdev_add_legacy_property - adds a legacy property
  *
  * Do not use this is new code!  Properties added through this interface will
- * be given types in the "legacy<>" type namespace.
+ * be given names and types in the "legacy<>" type namespace.
  *
  * Legacy properties are always processed as strings.  The format of the string
  * depends on the property type.
@@ -1183,18 +1188,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 c7d9535..0ba4e3a 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -485,11 +485,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 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 0/8] qom: introduce non-legacy static properties

[Anthony Liguori]

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

Excellent!  See the patches for individual comments but this is definitely a 
good thing to have.

Regards,

Anthony Liguori

>
> 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(-)
>

Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference

[Anthony Liguori]

On 12/16/2011 06:01 AM, Paolo Bonzini wrote:
> QAPI currently cannot deal with no object pushed to the stack,
> and dereferences a NULL pointer.  This is visible with
>
>      qom-get path=/i440fx/piix3 property=romfile
>
> after static non-string properties are introduced.

I'm a bit confused about what's happening here.  What's the significance of 
non-string properties?

Regards,

Anthony Liguori

>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   qapi/qmp-output-visitor.c |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
> index f76d015..29575da 100644
> --- a/qapi/qmp-output-visitor.c
> +++ b/qapi/qmp-output-visitor.c
> @@ -65,13 +65,13 @@ static QObject *qmp_output_pop(QmpOutputVisitor *qov)
>   static QObject *qmp_output_first(QmpOutputVisitor *qov)
>   {
>       QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack);
> -    return e->value;
> +    return e ? e->value : NULL;
>   }
>
>   static QObject *qmp_output_last(QmpOutputVisitor *qov)
>   {
>       QStackEntry *e = QTAILQ_FIRST(&qov->stack);
> -    return e->value;
> +    return e ? e->value : NULL;
>   }
>
>   static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name,

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

[Anthony Liguori]

On 12/16/2011 06:01 AM, Paolo Bonzini wrote:
> 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 <aliguori@us.ibm.com>

Regards,

Anthony Liguori

>
> 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;

Re: [Qemu-devel] [PATCH 3/8] qom: fix swapped parameters

[Anthony Liguori]

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

Yeah, this code path is currently dead as there is no way to test it until we 
defer s/init/realize/ and defer it to guest launch.

In my next series when I introduce Object and move properties into it, we can 
write some unit tests to cover this sort of thing.

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

Regards,

Anthony Liguori

> ---
>   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);
>       }
>   }
>

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

[Anthony Liguori]

On 12/16/2011 06:01 AM, Paolo Bonzini wrote:
> 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.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>

Nice cleanup.

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

Regards,

Anthony Liguori

> ---
>   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);
>

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 1/8] qapi: fix NULL pointer dereference

[Paolo Bonzini]

On 12/16/2011 02:55 PM, Anthony Liguori wrote:
>> This is visible with
>>
>>      qom-get path=/i440fx/piix3 property=romfile
>>
>> after static non-string properties are introduced.
>
> I'm a bit confused about what's happening here.  What's the significance
> of non-string properties?

Should have been "static non-legacy properties".

When you don't have a value for a property, legacy properties are 
visited as "<null>", while the new static properties do not pass 
anything to the visitor.

I stole this from qdev_property_get_str:

     value = prop->get(dev, errp);
     if (value) {
         visit_type_str(v, &value, name, errp);
         g_free(value);
     }

Paolo

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 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 7/8] qom: distinguish "legacy" property type name from QOM type name

[Anthony Liguori]

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

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

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

Regards,

Anthony Liguori

>
>       qdev_property_add(dev, prop->name, type,
>                         prop->info->print ? qdev_get_legacy_property : NULL,
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 9778123..c7d9535 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -156,6 +156,7 @@ enum PropertyType {
>
>   struct PropertyInfo {
>       const char *name;
> +    const char *legacy_name;
>       size_t size;
>       enum PropertyType type;
>       int64_t min;

Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference

[Anthony Liguori]

On 12/16/2011 08:00 AM, Paolo Bonzini wrote:
> On 12/16/2011 02:55 PM, Anthony Liguori wrote:
>>> This is visible with
>>>
>>> qom-get path=/i440fx/piix3 property=romfile
>>>
>>> after static non-string properties are introduced.
>>
>> I'm a bit confused about what's happening here. What's the significance
>> of non-string properties?
>
> Should have been "static non-legacy properties".
>
> When you don't have a value for a property, legacy properties are visited as
> "<null>", while the new static properties do not pass anything to the visitor.

>
> I stole this from qdev_property_get_str:
>
> value = prop->get(dev, errp);
> if (value) {
> visit_type_str(v, &value, name, errp);
> g_free(value);
> }

I should more clearly document it.  NULL would be only return if errp was set. 
I just didn't want to introduce a local_err in order to be able to check whether 
the function failed.

If a property function does not set the Error pointer, it must visit something.

Regards,

Anthony Liguori

>
> Paolo
>

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

[Paolo Bonzini]

On 12/16/2011 03:06 PM, Anthony Liguori wrote:
>>
>> -    type = g_strdup_printf("legacy<%s>", prop->info->name);
>> +    type = g_strdup_printf("legacy<%s>",
>> +                           prop->info->legacy_name ?: prop->info->name);
>
> I think this confuses the legacy type with the legacy property names.
>
> I think it would be better to do 'legacy-%s' as then it's at least clear
> when something is a type name vs. a property name.

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

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

paolo

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 1/8] qapi: fix NULL pointer dereference

[Paolo Bonzini]

On 12/16/2011 03:10 PM, Anthony Liguori wrote:
> If a property function does not set the Error pointer, it must visit
> something.

Hmm, then we have to introduce NULL into QJson and visitors.

Paolo

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

[Anthony Liguori]

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

No, I got confused.  Ignore my comments.

Regards,

Anthony Liguori

>
> paolo
>

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
>

Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference

[Anthony Liguori]

On 12/16/2011 08:22 AM, Paolo Bonzini wrote:
> On 12/16/2011 03:10 PM, Anthony Liguori wrote:
>> If a property function does not set the Error pointer, it must visit
>> something.
>
> Hmm, then we have to introduce NULL into QJson and visitors.

Visitors assume that strings aren't nullable (which is actually true in JSON and 
in QString).

I also think that string properties shouldn't be nullable.

Regards,

Anthony Liguori

>
> Paolo
>

Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference

[Paolo Bonzini]

On 12/16/2011 03:46 PM, Anthony Liguori wrote:
>>
>> Hmm, then we have to introduce NULL into QJson and visitors.
>
> Visitors assume that strings aren't nullable (which is actually true in
> JSON and in QString).
>
> I also think that string properties shouldn't be nullable.

Unfortunately, qdev string properties are nullable and there might well 
be examples in which empty and NULL are different.  I'd rather not risk.

But JSON actually has NULL, so not all is lost.  I can introduce a 
nullable_str type in visitors (restricting structs and arrays should be 
fine, though).

Paolo

Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference

[Anthony Liguori]

On 12/16/2011 08:49 AM, Paolo Bonzini wrote:
> On 12/16/2011 03:46 PM, Anthony Liguori wrote:
>>>
>>> Hmm, then we have to introduce NULL into QJson and visitors.
>>
>> Visitors assume that strings aren't nullable (which is actually true in
>> JSON and in QString).
>>
>> I also think that string properties shouldn't be nullable.
>
> Unfortunately, qdev string properties are nullable and there might well be
> examples in which empty and NULL are different. I'd rather not risk.
>
> But JSON actually has NULL, so not all is lost. I can introduce a nullable_str
> type in visitors (restricting structs and arrays should be fine, though).

I'd really prefer to stick to non-nullable strings as there is no obvious way to 
specify NULL in command line options.

What are the uses of null in qdev string properties?  I know you can't set a 
string to null since parse() doesn't have a null syntax.  So we're really just 
talking about an uninitialized state, right?

Regards,

Anthony Liguori

>
> Paolo
>

Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference

[Paolo Bonzini]

On 12/16/2011 03:56 PM, Anthony Liguori wrote:
> I'd really prefer to stick to non-nullable strings as there is no
> obvious way to specify NULL in command line options.

We can leave it as the default.  A property with a non-null default is 
implicitly not nullable, which actually makes some sense.  We can model 
this in get_string/set_string too.

> What are the uses of null in qdev string properties?  I know you can't
> set a string to null since parse() doesn't have a null syntax.  So we're
> really just talking about an uninitialized state, right?

Yes.  No ROM BAR is an example of a NULL string property.

Paolo

Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference

[Anthony Liguori]

On 12/16/2011 09:03 AM, Paolo Bonzini wrote:
> On 12/16/2011 03:56 PM, Anthony Liguori wrote:
>> I'd really prefer to stick to non-nullable strings as there is no
>> obvious way to specify NULL in command line options.
>
> We can leave it as the default. A property with a non-null default is implicitly
> not nullable, which actually makes some sense. We can model this in
> get_string/set_string too.
>
>> What are the uses of null in qdev string properties? I know you can't
>> set a string to null since parse() doesn't have a null syntax. So we're
>> really just talking about an uninitialized state, right?
>
> Yes. No ROM BAR is an example of a NULL string property.

So it's really filenames and backend names, right?  Can we just treat empty 
strings like NULL?  I think all of the various bits handles both the same way.

Regards,

Anthony Liguori

>
> Paolo
>

Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference

[Paolo Bonzini]

On 12/16/2011 04:05 PM, Anthony Liguori wrote:
>>
>>
>>> What are the uses of null in qdev string properties? I know you can't
>>> set a string to null since parse() doesn't have a null syntax. So we're
>>> really just talking about an uninitialized state, right?
>>
>> Yes. No ROM BAR is an example of a NULL string property.
>
> So it's really filenames and backend names, right?  Can we just treat
> empty strings like NULL?  I think all of the various bits handles both
> the same way.

The only case I can think of when it differs is serial numbers.  An 
empty serial number is different from no serial number (which means do 
not expose one at all).

However, DEFINE_PROP_STRING does not allow setting a default, and some 
device models rely on a NULL value as a default.  I see two ways out: 1) 
add nullable strings; 2) merge without this patch, knowing qom-get is 
kind-of broken, and proceed adding a default to all DEFINE_PROP_STRING; 
this would be a merge nightmare for you.  3) merge without this patch, 
knowing it's kind-of broken, and later decide what to do.

Let's do (3), and add nullable strings if the discussion stalls.

Paolo

Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference

[Anthony Liguori]

On 12/16/2011 09:13 AM, Paolo Bonzini wrote:
> On 12/16/2011 04:05 PM, Anthony Liguori wrote:
>>>
>>>
>>>> What are the uses of null in qdev string properties? I know you can't
>>>> set a string to null since parse() doesn't have a null syntax. So we're
>>>> really just talking about an uninitialized state, right?
>>>
>>> Yes. No ROM BAR is an example of a NULL string property.
>>
>> So it's really filenames and backend names, right? Can we just treat
>> empty strings like NULL? I think all of the various bits handles both
>> the same way.
>
> The only case I can think of when it differs is serial numbers. An empty serial
> number is different from no serial number (which means do not expose one at all).
>
> However, DEFINE_PROP_STRING does not allow setting a default, and some device
> models rely on a NULL value as a default. I see two ways out: 1) add nullable
> strings; 2) merge without this patch, knowing qom-get is kind-of broken, and
> proceed adding a default to all DEFINE_PROP_STRING; this would be a merge
> nightmare for you.

I thrive on pain it seems :-)

 > 3) merge without this patch, knowing it's kind-of broken, and
> later decide what to do.

Ok.  I think nullable strings are not a good idea simply because it means that a 
property can have a state that cannot be set.

Long term, I want to be able to do something like dump the current device graph 
to a config file, and then use that config file to recreate the same machine 
again.  A nullable property without a null representation would not allow this.

Regards,

Anthony Liguori

> Let's do (3), and add nullable strings if the discussion stalls.
>
> Paolo
>

Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference

[Paolo Bonzini]

On 12/16/2011 04:23 PM, Anthony Liguori wrote:
> Ok.  I think nullable strings are not a good idea simply because it
> means that a property can have a state that cannot be set.

How is this different from NULL links?  (Honest, not trick question :)).

> Long term, I want to be able to do something like dump the current
> device graph to a config file, and then use that config file to recreate
> the same machine again.  A nullable property without a null
> representation would not allow this.

JSON null is such a representation.

Paolo

Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference

[Anthony Liguori]

On 12/16/2011 09:42 AM, Paolo Bonzini wrote:
> On 12/16/2011 04:23 PM, Anthony Liguori wrote:
>> Ok. I think nullable strings are not a good idea simply because it
>> means that a property can have a state that cannot be set.
>
> How is this different from NULL links? (Honest, not trick question :)).

An empty string == NULL for links.

If a pointer is NULL, an empty string is returned.  So get/set is full symmetric.

>> Long term, I want to be able to do something like dump the current
>> device graph to a config file, and then use that config file to recreate
>> the same machine again. A nullable property without a null
>> representation would not allow this.
>
> JSON null is such a representation.

For JSON, but it doesn't map to a config file easily nor does it map to command 
line syntax well.

Regards,

Anthony Liguori

>
> Paolo
>

Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference

[Paolo Bonzini]

On 12/16/2011 04:54 PM, Anthony Liguori wrote:
> On 12/16/2011 09:42 AM, Paolo Bonzini wrote:
>> On 12/16/2011 04:23 PM, Anthony Liguori wrote:
>>> Ok. I think nullable strings are not a good idea simply because it
>>> means that a property can have a state that cannot be set.
>>
>> How is this different from NULL links? (Honest, not trick question :)).
>
> An empty string == NULL for links.
>
> If a pointer is NULL, an empty string is returned. So get/set is full
> symmetric.

Ok, so we can do the same for "pointer" properties.  I'm just a bit 
worried about serial numbers, where "-device virtio-blk-pci,...,serial=" 
is different from no serial at all.  One shows "" in info qtree, the 
other shows <null>.

> For JSON, but it doesn't map to a config file easily nor does it map to
> command line syntax well.

You can force nullable properties to have a null default (which is now 
the case in qdev), but I agree that's not too nice.

Paolo

Re: [Qemu-devel] [PATCH 1/8] qapi: fix NULL pointer dereference

[Gerd Hoffmann]

  Hi,

>> What are the uses of null in qdev string properties?  I know you can't
>> set a string to null since parse() doesn't have a null syntax.  So we're
>> really just talking about an uninitialized state, right?
> 
> Yes.  No ROM BAR is an example of a NULL string property.

Both NULL and zero-length string are valid for "empty" though, so you
can use -device e1000,romfile=,mac=whatever to boot without pxe rom.

Which underlines anthonys point ;)

cheers,
  Gerd

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