[Qemu-devel] [PATCH 00/16] access qdev properties via QOM

[Qemu-devel] [PATCH 00/16] access qdev properties via QOM

[Paolo Bonzini]

This series, on top of Anthony's qom-upstream.13, fixes several
qdev bugs, and adds enough functionality to QOM, to the point
that all property accesses go through QOM rather than poking
into the structs.

In particular, device initialization goes through the regular,
non-legacy properties; command-line option goes through the
legacy interface.  However, one important change is that if
the two are equivalent, the legacy interface will not be
registered anymore.

Patches 1 and 2 are bugfixes.

Patches 3 to 5 add wrappers to access properties easily as
QObjects, Objects or C types.

Patches 6 switches command-line operation and "info qtree"
to the QOM legacy properties.

Patches 7 to 11 progressively remove functionality from the
legacy properties when the normal ones are just as good.

Patch 12 switches property free to the normal QOM way.

Patches 13 to 15 switches device initialization to use the QOM
non-legacy properties (except for PROP_PTR).  Patch 16 finally switches
default values to use the QOM non-legacy properties.

Paolo Bonzini (16):
  qdev: fix hot-unplug
  qom: store object with correct type in interface links
  qom: do not include qdev header file
  qom: add QObject-based property get/set wrappers
  qom: add property get/set wrappers for C types
  qdev: remove direct calls to print/parse
  qdev: allow reusing get/set for legacy property
  qdev: remove parse method for string properties
  qdev: remove parse/print methods for mac properties
  qdev: make the non-legacy pci address property accept an integer
  qdev: remove parse/print methods for pointer properties
  qdev: let QOM free properties
  qdev: fix off-by-one
  qdev: access properties via QOM
  qdev: inline qdev_prop_set into qdev_prop_set_ptr
  qdev: initialize properties via QOM

 hw/qdev-addr.c        |    5 +-
 hw/qdev-monitor.c     |   30 ++--
 hw/qdev-properties.c  |  387 ++++++++++++++++++++++++++-----------------------
 hw/qdev.c             |   32 +++--
 hw/qdev.h             |   14 +-
 include/qemu/object.h |   94 ++++++++++++
 qmp.c                 |   17 +--
 qom/object.c          |  149 +++++++++++++++++--
 vl.c                  |    1 +
 9 files changed, 487 insertions(+), 242 deletions(-)

-- 
1.7.7.6

[Qemu-devel] [PATCH 01/16] qdev: fix hot-unplug

[Paolo Bonzini]

The reference that is returned by qdev_device_add is never given
back, so that device_del does not cause the refcount to go to zero
(and thus does nothing).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 vl.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/vl.c b/vl.c
index d88a18c..c63af69 100644
--- a/vl.c
+++ b/vl.c
@@ -1746,6 +1746,7 @@ static int device_init_func(QemuOpts *opts, void *opaque)
     dev = qdev_device_add(opts);
     if (!dev)
         return -1;
+    object_unref(OBJECT(dev));
     return 0;
 }
 
-- 
1.7.7.6

[Qemu-devel] [PATCH 02/16] qom: store object with correct type in interface links

[Paolo Bonzini]

When a link property's type is an interface, the code expects the
implementation object (not the parent object) to be stored in the
variable.  The parent object does not contain the right vtable.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qom/object.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index cd517f6..de6484d 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -749,7 +749,8 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
             target_type = g_strdup(&type[5]);
             target_type[strlen(target_type) - 2] = 0;
 
-            if (object_dynamic_cast(target, target_type)) {
+            target = object_dynamic_cast(target, target_type);
+            if (target) {
                 object_ref(target);
                 *child = target;
             } else {
-- 
1.7.7.6

[Qemu-devel] [PATCH 03/16] qom: do not include qdev header file

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qom/object.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index de6484d..299e146 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -13,8 +13,6 @@
 #include "qemu/object.h"
 #include "qemu-common.h"
 #include "qapi/qapi-visit-core.h"
-#include "hw/qdev.h"
-// FIXME remove above
 
 #define MAX_INTERFACES 32
 
-- 
1.7.7.6

[Qemu-devel] [PATCH 04/16] qom: add QObject-based property get/set wrappers

[Paolo Bonzini]

Move the creation of QmpInputVisitor and QmpOutputVisitor from
qmp.c to qom/object.c, since it's the only practical way to access
object properties.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/object.h |   24 ++++++++++++++++++++++++
 qmp.c                 |   17 ++---------------
 qom/object.c          |   29 +++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 15 deletions(-)

diff --git a/include/qemu/object.h b/include/qemu/object.h
index 947cf29..71090f2 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -542,6 +542,18 @@ void object_property_get(Object *obj, struct Visitor *v, const char *name,
                          struct Error **errp);
 
 /**
+ * object_property_get_qobject:
+ * @obj: the object
+ * @name: the name of the property
+ * @errp: returns an error if this function fails
+ *
+ * Returns: the value of the property, converted to QObject, or NULL if
+ * an error occurs.
+ */
+struct QObject *object_property_get_qobject(Object *obj, const char *name,
+                                            struct Error **errp);
+
+/**
  * object_property_set:
  * @obj: the object
  * @v: the visitor that will be used to write the property value.  This should
@@ -556,6 +568,18 @@ void object_property_set(Object *obj, struct Visitor *v, const char *name,
                          struct Error **errp);
 
 /**
+ * object_property_set_qobject:
+ * @obj: the object
+ * @ret: The value that will be written to the property.
+ * @name: the name of the property
+ * @errp: returns an error if this function fails
+ *
+ * Writes a property to a object.
+ */
+void object_property_set_qobject(Object *obj, struct QObject *qobj,
+                                 const char *name, struct Error **errp);
+
+/**
  * @object_property_get_type:
  * @obj: the object
  * @name: the name of the property
diff --git a/qmp.c b/qmp.c
index 45052cc..c7a81cc 100644
--- a/qmp.c
+++ b/qmp.c
@@ -21,8 +21,6 @@
 #include "kvm.h"
 #include "arch_init.h"
 #include "hw/qdev.h"
-#include "qapi/qmp-input-visitor.h"
-#include "qapi/qmp-output-visitor.h"
 #include "blockdev.h"
 
 NameInfo *qmp_query_name(Error **errp)
@@ -198,7 +196,6 @@ int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret)
     const char *property = qdict_get_str(qdict, "property");
     QObject *value = qdict_get(qdict, "value");
     Error *local_err = NULL;
-    QmpInputVisitor *mi;
     Object *obj;
 
     obj = object_resolve_path(path, NULL);
@@ -207,10 +204,7 @@ int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret)
         goto out;
     }
 
-    mi = qmp_input_visitor_new(value);
-    object_property_set(obj, qmp_input_get_visitor(mi), property, &local_err);
-
-    qmp_input_visitor_cleanup(mi);
+    object_property_set_qobject(obj, value, property, &local_err);
 
 out:
     if (local_err) {
@@ -227,7 +221,6 @@ int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret)
     const char *path = qdict_get_str(qdict, "path");
     const char *property = qdict_get_str(qdict, "property");
     Error *local_err = NULL;
-    QmpOutputVisitor *mo;
     Object *obj;
 
     obj = object_resolve_path(path, NULL);
@@ -236,13 +229,7 @@ int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret)
         goto out;
     }
 
-    mo = qmp_output_visitor_new();
-    object_property_get(obj, qmp_output_get_visitor(mo), property, &local_err);
-    if (!local_err) {
-        *ret = qmp_output_get_qobject(mo);
-    }
-
-    qmp_output_visitor_cleanup(mo);
+    *ret = object_property_get_qobject(obj, property, &local_err);
 
 out:
     if (local_err) {
diff --git a/qom/object.c b/qom/object.c
index 299e146..13c8bec 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -13,6 +13,8 @@
 #include "qemu/object.h"
 #include "qemu-common.h"
 #include "qapi/qapi-visit-core.h"
+#include "qapi/qmp-input-visitor.h"
+#include "qapi/qmp-output-visitor.h"
 
 #define MAX_INTERFACES 32
 
@@ -646,6 +648,33 @@ void object_property_set(Object *obj, Visitor *v, const char *name,
     }
 }
 
+void object_property_set_qobject(Object *obj, QObject *value,
+                                 const char *name, Error **errp)
+{
+    QmpInputVisitor *mi;
+    mi = qmp_input_visitor_new(value);
+    object_property_set(obj, qmp_input_get_visitor(mi), name, errp);
+
+    qmp_input_visitor_cleanup(mi);
+}
+
+QObject *object_property_get_qobject(Object *obj, const char *name,
+                                     Error **errp)
+{
+    QObject *ret = NULL;
+    Error *local_err = NULL;
+    QmpOutputVisitor *mo;
+
+    mo = qmp_output_visitor_new();
+    object_property_get(obj, qmp_output_get_visitor(mo), name, &local_err);
+    if (!local_err) {
+        ret = qmp_output_get_qobject(mo);
+    }
+    error_propagate(errp, local_err);
+    qmp_output_visitor_cleanup(mo);
+    return ret;
+}
+
 const char *object_property_get_type(Object *obj, const char *name, Error **errp)
 {
     ObjectProperty *prop = object_property_find(obj, name);
-- 
1.7.7.6

[Qemu-devel] [PATCH 05/16] qom: add property get/set wrappers for C types

[Paolo Bonzini]

Add more wrappers that create a QObject and free it around a
property set, and that convert a QObject to a C type for a property
get.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/object.h |   70 ++++++++++++++++++++++++++++++
 qom/object.c          |  115 +++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 176 insertions(+), 9 deletions(-)

diff --git a/include/qemu/object.h b/include/qemu/object.h
index 71090f2..1dcaea2 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -554,6 +554,76 @@ struct QObject *object_property_get_qobject(Object *obj, const char *name,
                                             struct Error **errp);
 
 /**
+ * object_property_set_str:
+ * @value: the value to be written to the property
+ * @name: the name of the property
+ * @errp: returns an error if this function fails
+ *
+ * Writes a string value to a property.
+ */
+void object_property_set_str(Object *obj, const char *value,
+                             const char *name, struct Error **errp);
+
+/**
+ * object_property_get_str:
+ * @obj: the object
+ * @name: the name of the property
+ * @errp: returns an error if this function fails
+ *
+ * Returns: the value of the property, converted to a C string, or NULL if
+ * an error occurs (including when the property value is not a string).
+ * The caller should free the string.
+ */
+char *object_property_get_str(Object *obj, const char *name,
+                              struct Error **errp);
+
+/**
+ * object_property_set_bool:
+ * @value: the value to be written to the property
+ * @name: the name of the property
+ * @errp: returns an error if this function fails
+ *
+ * Writes a bool value to a property.
+ */
+void object_property_set_bool(Object *obj, bool value,
+                              const char *name, struct Error **errp);
+
+/**
+ * object_property_get_bool:
+ * @obj: the object
+ * @name: the name of the property
+ * @errp: returns an error if this function fails
+ *
+ * Returns: the value of the property, converted to a boolean, or NULL if
+ * an error occurs (including when the property value is not a bool).
+ */
+bool object_property_get_bool(Object *obj, const char *name,
+                              struct Error **errp);
+
+/**
+ * object_property_set_int:
+ * @value: the value to be written to the property
+ * @name: the name of the property
+ * @errp: returns an error if this function fails
+ *
+ * Writes an integer value to a property.
+ */
+void object_property_set_int(Object *obj, int64_t value,
+                             const char *name, struct Error **errp);
+
+/**
+ * object_property_get_int:
+ * @obj: the object
+ * @name: the name of the property
+ * @errp: returns an error if this function fails
+ *
+ * Returns: the value of the property, converted to an integer, or NULL if
+ * an error occurs (including when the property value is not an integer).
+ */
+int64_t object_property_get_int(Object *obj, const char *name,
+                                struct Error **errp);
+
+/**
  * object_property_set:
  * @obj: the object
  * @v: the visitor that will be used to write the property value.  This should
diff --git a/qom/object.c b/qom/object.c
index 13c8bec..f1a1261 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -15,6 +15,10 @@
 #include "qapi/qapi-visit-core.h"
 #include "qapi/qmp-input-visitor.h"
 #include "qapi/qmp-output-visitor.h"
+#include "qobject.h"
+#include "qbool.h"
+#include "qint.h"
+#include "qstring.h"
 
 #define MAX_INTERFACES 32
 
@@ -675,6 +679,99 @@ QObject *object_property_get_qobject(Object *obj, const char *name,
     return ret;
 }
 
+void object_property_set_str(Object *obj, const char *value,
+                             const char *name, Error **errp)
+{
+    QString *qstr = qstring_from_str(value);
+    object_property_set_qobject(obj, QOBJECT(qstr), name, errp);
+
+    QDECREF(qstr);
+}
+
+char *object_property_get_str(Object *obj, const char *name,
+                              Error **errp)
+{
+    QObject *ret = object_property_get_qobject(obj, name, errp);
+    QString *qstring;
+    char *retval;
+
+    if (!ret) {
+        return NULL;
+    }
+    qstring = qobject_to_qstring(ret);
+    if (!qstring) {
+        error_set(errp, QERR_INVALID_PARAMETER_TYPE, name, "string");
+        retval = NULL;
+    } else {
+        retval = g_strdup(qstring_get_str(qstring));
+    }
+
+    QDECREF(qstring);
+    return retval;
+}
+
+void object_property_set_bool(Object *obj, bool value,
+                              const char *name, Error **errp)
+{
+    QBool *qbool = qbool_from_int(value);
+    object_property_set_qobject(obj, QOBJECT(qbool), name, errp);
+
+    QDECREF(qbool);
+}
+
+bool object_property_get_bool(Object *obj, const char *name,
+                              Error **errp)
+{
+    QObject *ret = object_property_get_qobject(obj, name, errp);
+    QBool *qbool;
+    bool retval;
+
+    if (!ret) {
+        return false;
+    }
+    qbool = qobject_to_qbool(ret);
+    if (!qbool) {
+        error_set(errp, QERR_INVALID_PARAMETER_TYPE, name, "boolean");
+        retval = false;
+    } else {
+        retval = qbool_get_int(qbool);
+    }
+
+    QDECREF(qbool);
+    return retval;
+}
+
+void object_property_set_int(Object *obj, int64_t value,
+                             const char *name, Error **errp)
+{
+    QInt *qint = qint_from_int(value);
+    object_property_set_qobject(obj, QOBJECT(qint), name, errp);
+
+    QDECREF(qint);
+}
+
+int64_t object_property_get_int(Object *obj, const char *name,
+                                Error **errp)
+{
+    QObject *ret = object_property_get_qobject(obj, name, errp);
+    QInt *qint;
+    int64_t retval;
+
+    if (!ret) {
+        return -1;
+    }
+    qint = qobject_to_qint(ret);
+    if (!qint) {
+        error_set(errp, QERR_INVALID_PARAMETER_TYPE, name, "int");
+        retval = -1;
+    } else {
+        retval = qint_get_int(qint);
+    }
+
+    QDECREF(qint);
+    return retval;
+}
+
 const char *object_property_get_type(Object *obj, const char *name, Error **errp)
 {
     ObjectProperty *prop = object_property_find(obj, name);
@@ -957,8 +1054,8 @@ typedef struct StringProperty
     void (*set)(Object *, const char *, Error **);
 } StringProperty;
 
-static void object_property_get_str(Object *obj, Visitor *v, void *opaque,
-                                    const char *name, Error **errp)
+static void property_get_str(Object *obj, Visitor *v, void *opaque,
+                             const char *name, Error **errp)
 {
     StringProperty *prop = opaque;
     char *value;
@@ -970,8 +1067,8 @@ static void object_property_get_str(Object *obj, Visitor *v, void *opaque,
     }
 }
 
-static void object_property_set_str(Object *obj, Visitor *v, void *opaque,
-                                  const char *name, Error **errp)
+static void property_set_str(Object *obj, Visitor *v, void *opaque,
+                             const char *name, Error **errp)
 {
     StringProperty *prop = opaque;
     char *value;
@@ -987,8 +1084,8 @@ static void object_property_set_str(Object *obj, Visitor *v, void *opaque,
     g_free(value);
 }
 
-static void object_property_release_str(Object *obj, const char *name,
-                                      void *opaque)
+static void property_release_str(Object *obj, const char *name,
+                                 void *opaque)
 {
     StringProperty *prop = opaque;
     g_free(prop);
@@ -1005,8 +1102,8 @@ void object_property_add_str(Object *obj, const char *name,
     prop->set = set;
 
     object_property_add(obj, name, "string",
-                        get ? object_property_get_str : NULL,
-                        set ? object_property_set_str : NULL,
-                        object_property_release_str,
+                        get ? property_get_str : NULL,
+                        set ? property_set_str : NULL,
+                        property_release_str,
                         prop, errp);
 }
-- 
1.7.7.6

[Qemu-devel] [PATCH 06/16] qdev: remove direct calls to print/parse

[Paolo Bonzini]

There's no need to call into ->parse and ->print manually.  The
QOM legacy properties do that for us.

Furthermore, in some cases legacy and static properties have exactly
the same behavior, and we could drop the legacy properties right away.
Add an appropriate fallback to prepare for this.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-monitor.c    |   27 ++++++++++++++++-----------
 hw/qdev-properties.c |   26 ++++++++++----------------
 hw/qdev.c            |    9 +++++++++
 3 files changed, 35 insertions(+), 27 deletions(-)

diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index b8d8a9e..64505b4 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -487,21 +487,26 @@ static void qbus_print(Monitor *mon, BusState *bus, int indent);
 static void qdev_print_props(Monitor *mon, DeviceState *dev, Property *props,
                              const char *prefix, int indent)
 {
-    char buf[64];
-
     if (!props)
         return;
     while (props->name) {
-        /*
-         * TODO Properties without a print method are just for dirty
-         * hacks.  qdev_prop_ptr is the only such PropertyInfo.  It's
-         * marked for removal.  The test props->info->print should be
-         * removed along with it.
-         */
-        if (props->info->print) {
-            props->info->print(dev, props, buf, sizeof(buf));
-            qdev_printf("%s-prop: %s = %s\n", prefix, props->name, buf);
+        Error *err;
+        char *value;
+        char *legacy_name = g_strdup_printf("legacy-%s", props->name);
+        if (object_property_get_type(OBJECT(dev), legacy_name, NULL)) {
+            value = object_property_get_str(OBJECT(dev), legacy_name, &err);
+        } else {
+            value = object_property_get_str(OBJECT(dev), props->name, &err);
+        }
+        g_free(legacy_name);
+
+        if (err) {
+            error_free(err);
+            continue;
         }
+        qdev_printf("%s-prop: %s = %s\n", prefix, props->name,
+                    value && *value ? value : "<null>");
+        g_free(value);
         props++;
     }
 }
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index d34df30..7c41140 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -1024,24 +1024,18 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
 
 int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
 {
-    Property *prop;
-    int ret;
+    char *legacy_name;
+    Error *err;
 
-    prop = qdev_prop_find(dev, name);
-    /*
-     * TODO Properties without a parse method are just for dirty
-     * hacks.  qdev_prop_ptr is the only such PropertyInfo.  It's
-     * marked for removal.  The test !prop->info->parse should be
-     * removed along with it.
-     */
-    if (!prop || !prop->info->parse) {
-        qerror_report(QERR_PROPERTY_NOT_FOUND, object_get_typename(OBJECT(dev)), name);
-        return -1;
+    legacy_name = g_strdup_printf("legacy-%s", name);
+    if (object_property_get_type(OBJECT(dev), legacy_name, NULL)) {
+        object_property_set_str(OBJECT(dev), legacy_name, value, &err);
+    } else {
+        object_property_set_str(OBJECT(dev), name, value, &err);
     }
-    ret = prop->info->parse(dev, prop, value);
-    if (ret < 0) {
-        Error *err;
-        error_set_from_qdev_prop_error(&err, ret, dev, prop, value);
+    g_free(legacy_name);
+
+    if (err) {
         qerror_report_err(err);
         error_free(err);
         return -1;
diff --git a/hw/qdev.c b/hw/qdev.c
index e3b53b7..a731e41 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -581,6 +581,15 @@ void qdev_property_add_legacy(DeviceState *dev, Property *prop,
 void qdev_property_add_static(DeviceState *dev, Property *prop,
                               Error **errp)
 {
+    /*
+     * TODO qdev_prop_ptr does not have getters or setters.  It must
+     * go now that it can be replaced with links.  The test should be
+     * removed along with it, all static properties are read/write.
+     */
+    if (!prop->info->get && !prop->info->set) {
+        return;
+    }
+
     object_property_add(OBJECT(dev), prop->name, prop->info->name,
                         prop->info->get, prop->info->set,
                         NULL,
-- 
1.7.7.6

[Qemu-devel] [PATCH 07/16] qdev: allow reusing get/set for legacy property

[Paolo Bonzini]

In some cases, a legacy property does need a special print method
but not a special parse method.  In this case, we can reuse the get/set
from the static (non-legacy) property.

If neither parse nor print is needed, though, do not register the
legacy property at all.  The previous patch ensures that the right
fallback will be used.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-monitor.c    |    5 ++---
 hw/qdev-properties.c |    6 +++---
 hw/qdev.c            |   11 +++++++----
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
index 64505b4..e21bd50 100644
--- a/hw/qdev-monitor.c
+++ b/hw/qdev-monitor.c
@@ -489,8 +489,8 @@ static void qdev_print_props(Monitor *mon, DeviceState *dev, Property *props,
 {
     if (!props)
         return;
-    while (props->name) {
-        Error *err;
+    for (; props->name; props++) {
+        Error *err = NULL;
         char *value;
         char *legacy_name = g_strdup_printf("legacy-%s", props->name);
         if (object_property_get_type(OBJECT(dev), legacy_name, NULL)) {
@@ -507,7 +507,6 @@ static void qdev_print_props(Monitor *mon, DeviceState *dev, Property *props,
         qdev_printf("%s-prop: %s = %s\n", prefix, props->name,
                     value && *value ? value : "<null>");
         g_free(value);
-        props++;
     }
 }
 
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 7c41140..16f9b22 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -1025,13 +1025,13 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
 int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
 {
     char *legacy_name;
-    Error *err;
+    Error *err = NULL;
 
     legacy_name = g_strdup_printf("legacy-%s", name);
     if (object_property_get_type(OBJECT(dev), legacy_name, NULL)) {
-        object_property_set_str(OBJECT(dev), legacy_name, value, &err);
+        object_property_set_str(OBJECT(dev), value, legacy_name, &err);
     } else {
-        object_property_set_str(OBJECT(dev), name, value, &err);
+        object_property_set_str(OBJECT(dev), value, name, &err);
     }
     g_free(legacy_name);
 
diff --git a/hw/qdev.c b/hw/qdev.c
index a731e41..660ee38 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -550,21 +550,24 @@ static void qdev_set_legacy_property(Object *obj, Visitor *v, void *opaque,
  * Do not use this is new code!  Properties added through this interface will
  * 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.
+ * Legacy properties are string versions of other OOM properties.  The format
+ * of the string depends on the property type.
  */
 void qdev_property_add_legacy(DeviceState *dev, Property *prop,
                               Error **errp)
 {
     gchar *name, *type;
 
+    if (!prop->info->print && !prop->info->parse) {
+        return;
+    }
     name = g_strdup_printf("legacy-%s", prop->name);
     type = g_strdup_printf("legacy<%s>",
                            prop->info->legacy_name ?: prop->info->name);
 
     object_property_add(OBJECT(dev), name, type,
-                        prop->info->print ? qdev_get_legacy_property : NULL,
-                        prop->info->parse ? qdev_set_legacy_property : NULL,
+                        prop->info->print ? qdev_get_legacy_property : prop->info->get,
+                        prop->info->parse ? qdev_set_legacy_property : prop->info->set,
                         NULL,
                         prop, errp);
 
-- 
1.7.7.6

[Qemu-devel] [PATCH 08/16] qdev: remove parse method for string properties

[Paolo Bonzini]

We need the print method to put double quotes, but parsing is not special.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-properties.c |   11 -----------
 1 files changed, 0 insertions(+), 11 deletions(-)

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 16f9b22..0a293af 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -510,16 +510,6 @@ PropertyInfo qdev_prop_hex64 = {
 
 /* --- string --- */
 
-static int parse_string(DeviceState *dev, Property *prop, const char *str)
-{
-    char **ptr = qdev_get_prop_ptr(dev, prop);
-
-    if (*ptr)
-        g_free(*ptr);
-    *ptr = g_strdup(str);
-    return 0;
-}
-
 static void free_string(DeviceState *dev, Property *prop)
 {
     g_free(*(char **)qdev_get_prop_ptr(dev, prop));
@@ -581,7 +571,6 @@ PropertyInfo qdev_prop_string = {
     .name  = "string",
     .type  = PROP_TYPE_STRING,
     .size  = sizeof(char*),
-    .parse = parse_string,
     .print = print_string,
     .free  = free_string,
     .get   = get_string,
-- 
1.7.7.6

[Qemu-devel] [PATCH 09/16] qdev: remove parse/print methods for mac properties

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-properties.c |   61 ++++++++++++++++++++++++++++++++++---------------
 1 files changed, 42 insertions(+), 19 deletions(-)

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 0a293af..4fb5cf8 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -848,46 +848,69 @@ PropertyInfo qdev_prop_ptr = {
  *   01:02:03:04:05:06
  *   01-02-03-04-05-06
  */
-static int parse_mac(DeviceState *dev, Property *prop, const char *str)
+static void get_mac(Object *obj, Visitor *v, void *opaque,
+                    const char *name, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    MACAddr *mac = qdev_get_prop_ptr(dev, prop);
+    char buffer[2 * 6 + 5 + 1];
+    char *p = buffer;
+
+    snprintf(buffer, sizeof(buffer), "%02x:%02x:%02x:%02x:%02x:%02x",
+             mac->a[0], mac->a[1], mac->a[2],
+             mac->a[3], mac->a[4], mac->a[5]);
+
+    visit_type_str(v, &p, name, errp);
+}
+
+static void set_mac(Object *obj, Visitor *v, void *opaque,
+                    const char *name, Error **errp)
 {
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
     MACAddr *mac = qdev_get_prop_ptr(dev, prop);
+    Error *local_err = NULL;
     int i, pos;
-    char *p;
+    char *str, *p;
+
+    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;
+    }
 
     for (i = 0, pos = 0; i < 6; i++, pos += 3) {
         if (!qemu_isxdigit(str[pos]))
-            return -EINVAL;
+            goto inval;
         if (!qemu_isxdigit(str[pos+1]))
-            return -EINVAL;
+            goto inval;
         if (i == 5) {
             if (str[pos+2] != '\0')
-                return -EINVAL;
+                goto inval;
         } else {
             if (str[pos+2] != ':' && str[pos+2] != '-')
-                return -EINVAL;
+                goto inval;
         }
         mac->a[i] = strtol(str+pos, &p, 16);
     }
-    return 0;
-}
-
-static int print_mac(DeviceState *dev, Property *prop, char *dest, size_t len)
-{
-    MACAddr *mac = qdev_get_prop_ptr(dev, prop);
+    return;
 
-    return snprintf(dest, len, "%02x:%02x:%02x:%02x:%02x:%02x",
-                    mac->a[0], mac->a[1], mac->a[2],
-                    mac->a[3], mac->a[4], mac->a[5]);
+inval:
+    error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
 }
 
 PropertyInfo qdev_prop_macaddr = {
     .name  = "macaddr",
     .type  = PROP_TYPE_MACADDR,
     .size  = sizeof(MACAddr),
-    .parse = parse_mac,
-    .print = print_mac,
-    .get   = get_generic,
-    .set   = set_generic,
+    .get   = get_mac,
+    .set   = set_mac,
 };
 
 /* --- pci address --- */
-- 
1.7.7.6

[Qemu-devel] [PATCH 10/16] qdev: make the non-legacy pci address property accept an integer

[Paolo Bonzini]

PCI addresses are set with qdev_prop_uint32.  Thus we make the QOM
property accept a device and function encoded in an 8-bit integer,
instead of the magic dd.f hex string.

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

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 4fb5cf8..e4bcc6d 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -950,30 +950,19 @@ static int print_pci_devfn(DeviceState *dev, Property *prop, char *dest, size_t
     }
 }
 
-static void get_pci_devfn(Object *obj, Visitor *v, void *opaque,
-                          const char *name, Error **errp)
-{
-    DeviceState *dev = DEVICE(obj);
-    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,
+    .get   = get_int32,
+    .set   = set_int32,
+    /* FIXME: this should be -1...255, but the address is stored
+     * into an uint32_t rather than int32_t.
+     */
+    .min   = 0,
+    .max   = 0xFFFFFFFFULL,
 };
 
 /* --- public helpers --- */
-- 
1.7.7.6

[Qemu-devel] [PATCH 11/16] qdev: remove parse/print methods for pointer properties

[Paolo Bonzini]

Pointer properties (except for PROP_PTR of course) should not need a
legacy counterpart.  In the future, relative paths will ensure that
QEMU will support the same syntax as now for drives etc..

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-properties.c |  128 ++++++++++++++++++++++++++++----------------------
 1 files changed, 72 insertions(+), 56 deletions(-)

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index e4bcc6d..627d335 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -579,9 +579,8 @@ PropertyInfo qdev_prop_string = {
 
 /* --- drive --- */
 
-static int parse_drive(DeviceState *dev, Property *prop, const char *str)
+static int parse_drive(DeviceState *dev, const char *str, void **ptr)
 {
-    BlockDriverState **ptr = qdev_get_prop_ptr(dev, prop);
     BlockDriverState *bs;
 
     bs = bdrv_find(str);
@@ -603,35 +602,30 @@ static void free_drive(DeviceState *dev, Property *prop)
     }
 }
 
-static int print_drive(DeviceState *dev, Property *prop, char *dest, size_t len)
+static const char *print_drive(void *ptr)
 {
-    BlockDriverState **ptr = qdev_get_prop_ptr(dev, prop);
-    return snprintf(dest, len, "%s",
-                    *ptr ? bdrv_get_device_name(*ptr) : "<null>");
+    return bdrv_get_device_name(ptr);
 }
 
-static void get_generic(Object *obj, Visitor *v, void *opaque,
-                       const char *name, Error **errp)
+static void get_pointer(Object *obj, Visitor *v, Property *prop,
+                        const char *(*print)(void *ptr),
+                        const char *name, Error **errp)
 {
     DeviceState *dev = DEVICE(obj);
-    Property *prop = opaque;
     void **ptr = qdev_get_prop_ptr(dev, prop);
-    char buffer[1024];
-    char *p = buffer;
+    char *p;
 
-    buffer[0] = 0;
-    if (*ptr) {
-        prop->info->print(dev, prop, buffer, sizeof(buffer));
-    }
+    p = (char *) (*ptr ? print(*ptr) : "");
     visit_type_str(v, &p, name, errp);
 }
 
-static void set_generic(Object *obj, Visitor *v, void *opaque,
+static void set_pointer(Object *obj, Visitor *v, Property *prop,
+                        int (*parse)(DeviceState *dev, const char *str, void **ptr),
                         const char *name, Error **errp)
 {
     DeviceState *dev = DEVICE(obj);
-    Property *prop = opaque;
     Error *local_err = NULL;
+    void **ptr = qdev_get_prop_ptr(dev, prop);
     char *str;
     int ret;
 
@@ -650,36 +644,45 @@ static void set_generic(Object *obj, Visitor *v, void *opaque,
         error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
         return;
     }
-    ret = prop->info->parse(dev, prop, str);
+    ret = parse(dev, str, ptr);
     error_set_from_qdev_prop_error(errp, ret, dev, prop, str);
     g_free(str);
 }
 
+static void get_drive(Object *obj, Visitor *v, void *opaque,
+                      const char *name, Error **errp)
+{
+    get_pointer(obj, v, opaque, print_drive, name, errp);
+}
+
+static void set_drive(Object *obj, Visitor *v, void *opaque,
+                      const char *name, Error **errp)
+{
+    set_pointer(obj, v, opaque, parse_drive, name, errp);
+}
+
 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,
+    .get   = get_drive,
+    .set   = set_drive,
     .free  = free_drive,
 };
 
 /* --- character device --- */
 
-static int parse_chr(DeviceState *dev, Property *prop, const char *str)
+static int parse_chr(DeviceState *dev, const char *str, void **ptr)
 {
-    CharDriverState **ptr = qdev_get_prop_ptr(dev, prop);
-
-    *ptr = qemu_chr_find(str);
-    if (*ptr == NULL) {
+    CharDriverState *chr = qemu_chr_find(str);
+    if (chr == NULL) {
         return -ENOENT;
     }
-    if ((*ptr)->avail_connections < 1) {
+    if (chr->avail_connections < 1) {
         return -EEXIST;
     }
-    --(*ptr)->avail_connections;
+    *ptr = chr;
+    --chr->avail_connections;
     return 0;
 }
 
@@ -693,62 +696,75 @@ static void free_chr(DeviceState *dev, Property *prop)
 }
 
 
-static int print_chr(DeviceState *dev, Property *prop, char *dest, size_t len)
+static const char *print_chr(void *ptr)
 {
-    CharDriverState **ptr = qdev_get_prop_ptr(dev, prop);
+    CharDriverState *chr = ptr;
 
-    if (*ptr && (*ptr)->label) {
-        return snprintf(dest, len, "%s", (*ptr)->label);
-    } else {
-        return snprintf(dest, len, "<null>");
-    }
+    return chr->label ? chr->label : "";
+}
+
+static void get_chr(Object *obj, Visitor *v, void *opaque,
+                    const char *name, Error **errp)
+{
+    get_pointer(obj, v, opaque, print_chr, name, errp);
+}
+
+static void set_chr(Object *obj, Visitor *v, void *opaque,
+                    const char *name, Error **errp)
+{
+    set_pointer(obj, v, opaque, parse_chr, name, errp);
 }
 
 PropertyInfo qdev_prop_chr = {
     .name  = "chr",
     .type  = PROP_TYPE_CHR,
     .size  = sizeof(CharDriverState*),
-    .parse = parse_chr,
-    .print = print_chr,
-    .get   = get_generic,
-    .set   = set_generic,
+    .get   = get_chr,
+    .set   = set_chr,
     .free  = free_chr,
 };
 
 /* --- netdev device --- */
 
-static int parse_netdev(DeviceState *dev, Property *prop, const char *str)
+static int parse_netdev(DeviceState *dev, const char *str, void **ptr)
 {
-    VLANClientState **ptr = qdev_get_prop_ptr(dev, prop);
+    VLANClientState *netdev = qemu_find_netdev(str);
 
-    *ptr = qemu_find_netdev(str);
-    if (*ptr == NULL)
+    if (netdev == NULL) {
         return -ENOENT;
-    if ((*ptr)->peer) {
+    }
+    if (netdev->peer) {
         return -EEXIST;
     }
+    *ptr = netdev;
     return 0;
 }
 
-static int print_netdev(DeviceState *dev, Property *prop, char *dest, size_t len)
+static const char *print_netdev(void *ptr)
 {
-    VLANClientState **ptr = qdev_get_prop_ptr(dev, prop);
+    VLANClientState *netdev = ptr;
 
-    if (*ptr && (*ptr)->name) {
-        return snprintf(dest, len, "%s", (*ptr)->name);
-    } else {
-        return snprintf(dest, len, "<null>");
-    }
+    return netdev->name ? netdev->name : "";
+}
+
+static void get_netdev(Object *obj, Visitor *v, void *opaque,
+                       const char *name, Error **errp)
+{
+    get_pointer(obj, v, opaque, print_netdev, name, errp);
+}
+
+static void set_netdev(Object *obj, Visitor *v, void *opaque,
+                       const char *name, Error **errp)
+{
+    set_pointer(obj, v, opaque, parse_netdev, name, errp);
 }
 
 PropertyInfo qdev_prop_netdev = {
     .name  = "netdev",
     .type  = PROP_TYPE_NETDEV,
     .size  = sizeof(VLANClientState*),
-    .parse = parse_netdev,
-    .print = print_netdev,
-    .get   = get_generic,
-    .set   = set_generic,
+    .get   = get_netdev,
+    .set   = set_netdev,
 };
 
 /* --- vlan --- */
-- 
1.7.7.6

[Qemu-devel] [PATCH 12/16] qdev: let QOM free properties

[Paolo Bonzini]

Drop the special free callback.  Instead, register a "regular"
release method in the non-legacy property.

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

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 627d335..7efcc78 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -510,9 +510,10 @@ PropertyInfo qdev_prop_hex64 = {
 
 /* --- string --- */
 
-static void free_string(DeviceState *dev, Property *prop)
+static void release_string(Object *obj, const char *name, void *opaque)
 {
-    g_free(*(char **)qdev_get_prop_ptr(dev, prop));
+    Property *prop = opaque;
+    g_free(*(char **)qdev_get_prop_ptr(DEVICE(obj), prop));
 }
 
 static int print_string(DeviceState *dev, Property *prop, char *dest, size_t len)
@@ -572,7 +573,7 @@ PropertyInfo qdev_prop_string = {
     .type  = PROP_TYPE_STRING,
     .size  = sizeof(char*),
     .print = print_string,
-    .free  = free_string,
+    .release = release_string,
     .get   = get_string,
     .set   = set_string,
 };
@@ -592,8 +593,10 @@ static int parse_drive(DeviceState *dev, const char *str, void **ptr)
     return 0;
 }
 
-static void free_drive(DeviceState *dev, Property *prop)
+static void release_drive(Object *obj, const char *name, void *opaque)
 {
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
     BlockDriverState **ptr = qdev_get_prop_ptr(dev, prop);
 
     if (*ptr) {
@@ -667,7 +670,7 @@ PropertyInfo qdev_prop_drive = {
     .size  = sizeof(BlockDriverState *),
     .get   = get_drive,
     .set   = set_drive,
-    .free  = free_drive,
+    .release = release_drive,
 };
 
 /* --- character device --- */
@@ -686,8 +689,10 @@ static int parse_chr(DeviceState *dev, const char *str, void **ptr)
     return 0;
 }
 
-static void free_chr(DeviceState *dev, Property *prop)
+static void release_chr(Object *obj, const char *name, void *opaque)
 {
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
     CharDriverState **ptr = qdev_get_prop_ptr(dev, prop);
 
     if (*ptr) {
@@ -721,7 +726,7 @@ PropertyInfo qdev_prop_chr = {
     .size  = sizeof(CharDriverState*),
     .get   = get_chr,
     .set   = set_chr,
-    .free  = free_chr,
+    .release = release_chr,
 };
 
 /* --- netdev device --- */
diff --git a/hw/qdev.c b/hw/qdev.c
index 660ee38..f719f14 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -595,7 +595,7 @@ void qdev_property_add_static(DeviceState *dev, Property *prop,
 
     object_property_add(OBJECT(dev), prop->name, prop->info->name,
                         prop->info->get, prop->info->set,
-                        NULL,
+                        prop->info->release,
                         prop, errp);
 }
 
@@ -626,7 +626,6 @@ static void device_finalize(Object *obj)
 {
     DeviceState *dev = DEVICE(obj);
     BusState *bus;
-    Property *prop;
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
 
     if (dev->state == DEV_STATE_INITIALIZED) {
@@ -645,11 +644,6 @@ static void device_finalize(Object *obj)
         }
     }
     QTAILQ_REMOVE(&dev->parent_bus->children, dev, sibling);
-    for (prop = qdev_get_props(dev); prop && prop->name; prop++) {
-        if (prop->info->free) {
-            prop->info->free(dev, prop);
-        }
-    }
 }
 
 void device_reset(DeviceState *dev)
diff --git a/hw/qdev.h b/hw/qdev.h
index 1d9143f..dbd8928 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -143,9 +143,9 @@ struct PropertyInfo {
     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);
     ObjectPropertyAccessor *get;
     ObjectPropertyAccessor *set;
+    ObjectPropertyRelease *release;
 };
 
 typedef struct GlobalProperty {
-- 
1.7.7.6

[Qemu-devel] [PATCH 13/16] qdev: fix off-by-one

[Paolo Bonzini]

Integer properties did not work.

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

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 7efcc78..30abae2 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -151,7 +151,7 @@ static void set_int8(Object *obj, Visitor *v, void *opaque,
         error_propagate(errp, local_err);
         return;
     }
-    if (value > prop->info->min && value <= prop->info->max) {
+    if (value >= prop->info->min && value <= prop->info->max) {
         *ptr = value;
     } else {
         error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
@@ -259,7 +259,7 @@ static void set_int16(Object *obj, Visitor *v, void *opaque,
         error_propagate(errp, local_err);
         return;
     }
-    if (value > prop->info->min && value <= prop->info->max) {
+    if (value >= prop->info->min && value <= prop->info->max) {
         *ptr = value;
     } else {
         error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
@@ -333,7 +333,7 @@ static void set_int32(Object *obj, Visitor *v, void *opaque,
         error_propagate(errp, local_err);
         return;
     }
-    if (value > prop->info->min && value <= prop->info->max) {
+    if (value >= prop->info->min && value <= prop->info->max) {
         *ptr = value;
     } else {
         error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE,
-- 
1.7.7.6

[Qemu-devel] [PATCH 14/16] qdev: access properties via QOM

[Paolo Bonzini]

Do not poke anymore in the struct when accessing qdev properties.
Instead, ask the object to set the right value.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-addr.c       |    5 +++-
 hw/qdev-properties.c |   67 ++++++++++++++++++++++++++++++++++---------------
 hw/qdev.h            |    1 -
 3 files changed, 50 insertions(+), 23 deletions(-)

diff --git a/hw/qdev-addr.c b/hw/qdev-addr.c
index 5976dcd..8daa733 100644
--- a/hw/qdev-addr.c
+++ b/hw/qdev-addr.c
@@ -71,5 +71,8 @@ PropertyInfo qdev_prop_taddr = {
 
 void qdev_prop_set_taddr(DeviceState *dev, const char *name, target_phys_addr_t value)
 {
-    qdev_prop_set(dev, name, &value, PROP_TYPE_TADDR);
+    Error *errp = NULL;
+    object_property_set_int(OBJECT(dev), value, name, &errp);
+    assert(!errp);
+
 }
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 30abae2..ab7f522 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -1065,7 +1065,7 @@ int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
     return 0;
 }
 
-void qdev_prop_set(DeviceState *dev, const char *name, void *src, enum PropertyType type)
+static void qdev_prop_set(DeviceState *dev, const char *name, void *src, enum PropertyType type)
 {
     Property *prop;
 
@@ -1085,52 +1085,63 @@ void qdev_prop_set(DeviceState *dev, const char *name, void *src, enum PropertyT
 
 void qdev_prop_set_bit(DeviceState *dev, const char *name, bool value)
 {
-    qdev_prop_set(dev, name, &value, PROP_TYPE_BIT);
+    Error *errp = NULL;
+    object_property_set_bool(OBJECT(dev), value, name, &errp);
+    assert(!errp);
 }
 
 void qdev_prop_set_uint8(DeviceState *dev, const char *name, uint8_t value)
 {
-    qdev_prop_set(dev, name, &value, PROP_TYPE_UINT8);
+    Error *errp = NULL;
+    object_property_set_int(OBJECT(dev), value, name, &errp);
+    assert(!errp);
 }
 
 void qdev_prop_set_uint16(DeviceState *dev, const char *name, uint16_t value)
 {
-    qdev_prop_set(dev, name, &value, PROP_TYPE_UINT16);
+    Error *errp = NULL;
+    object_property_set_int(OBJECT(dev), value, name, &errp);
+    assert(!errp);
 }
 
 void qdev_prop_set_uint32(DeviceState *dev, const char *name, uint32_t value)
 {
-    qdev_prop_set(dev, name, &value, PROP_TYPE_UINT32);
+    Error *errp = NULL;
+    object_property_set_int(OBJECT(dev), value, name, &errp);
+    assert(!errp);
 }
 
 void qdev_prop_set_int32(DeviceState *dev, const char *name, int32_t value)
 {
-    qdev_prop_set(dev, name, &value, PROP_TYPE_INT32);
+    Error *errp = NULL;
+    object_property_set_int(OBJECT(dev), value, name, &errp);
+    assert(!errp);
 }
 
 void qdev_prop_set_uint64(DeviceState *dev, const char *name, uint64_t value)
 {
-    qdev_prop_set(dev, name, &value, PROP_TYPE_UINT64);
+    Error *errp = NULL;
+    object_property_set_int(OBJECT(dev), value, name, &errp);
+    assert(!errp);
 }
 
 void qdev_prop_set_string(DeviceState *dev, const char *name, char *value)
 {
-    qdev_prop_set(dev, name, &value, PROP_TYPE_STRING);
+    Error *errp = NULL;
+    object_property_set_str(OBJECT(dev), value, name, &errp);
+    assert(!errp);
 }
 
 int qdev_prop_set_drive(DeviceState *dev, const char *name, BlockDriverState *value)
 {
-    int res;
-
-    res = bdrv_attach_dev(value, dev);
-    if (res < 0) {
-        error_report("Can't attach drive %s to %s.%s: %s",
-                     bdrv_get_device_name(value),
-                     dev->id ? dev->id : object_get_typename(OBJECT(dev)),
-                     name, strerror(-res));
+    Error *errp = NULL;
+    object_property_set_str(OBJECT(dev), bdrv_get_device_name(value),
+                            name, &errp);
+    if (errp) {
+        qerror_report_err(errp);
+        error_free(errp);
         return -1;
     }
-    qdev_prop_set(dev, name, &value, PROP_TYPE_DRIVE);
     return 0;
 }
 
@@ -1142,22 +1153,36 @@ void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name, BlockDriverS
 }
 void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *value)
 {
-    qdev_prop_set(dev, name, &value, PROP_TYPE_CHR);
+    Error *errp = NULL;
+    assert(value->label);
+    object_property_set_str(OBJECT(dev), value->label, name, &errp);
+    assert(!errp);
 }
 
 void qdev_prop_set_netdev(DeviceState *dev, const char *name, VLANClientState *value)
 {
-    qdev_prop_set(dev, name, &value, PROP_TYPE_NETDEV);
+    Error *errp = NULL;
+    assert(value->name);
+    object_property_set_str(OBJECT(dev), value->name, name, &errp);
+    assert(!errp);
 }
 
 void qdev_prop_set_vlan(DeviceState *dev, const char *name, VLANState *value)
 {
-    qdev_prop_set(dev, name, &value, PROP_TYPE_VLAN);
+    Error *errp = NULL;
+    object_property_set_int(OBJECT(dev), value ? value->id : -1, name, &errp);
+    assert(!errp);
 }
 
 void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t *value)
 {
-    qdev_prop_set(dev, name, value, PROP_TYPE_MACADDR);
+    Error *errp = NULL;
+    char str[2 * 6 + 5 + 1];
+    snprintf(str, sizeof(str), "%02x:%02x:%02x:%02x:%02x:%02x",
+             value[0], value[1], value[2], value[3], value[4], value[5]);
+
+    object_property_set_str(OBJECT(dev), str, name, &errp);
+    assert(!errp);
 }
 
 void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
diff --git a/hw/qdev.h b/hw/qdev.h
index dbd8928..c0e5600 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -304,7 +304,6 @@ extern PropertyInfo qdev_prop_pci_devfn;
 void *qdev_get_prop_ptr(DeviceState *dev, Property *prop);
 int qdev_prop_exists(DeviceState *dev, const char *name);
 int qdev_prop_parse(DeviceState *dev, const char *name, const char *value);
-void qdev_prop_set(DeviceState *dev, const char *name, void *src, enum PropertyType type);
 void qdev_prop_set_bit(DeviceState *dev, const char *name, bool value);
 void qdev_prop_set_uint8(DeviceState *dev, const char *name, uint8_t value);
 void qdev_prop_set_uint16(DeviceState *dev, const char *name, uint16_t value);
-- 
1.7.7.6

[Qemu-devel] [PATCH 15/16] qdev: inline qdev_prop_set into qdev_prop_set_ptr

[Paolo Bonzini]

qdev_prop_set is not needed anymore except for hacks, simplify it and
inline it.

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

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index ab7f522..d7e5356 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -1065,24 +1065,6 @@ int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
     return 0;
 }
 
-static void qdev_prop_set(DeviceState *dev, const char *name, void *src, enum PropertyType type)
-{
-    Property *prop;
-
-    prop = qdev_prop_find(dev, name);
-    if (!prop) {
-        fprintf(stderr, "%s: property \"%s.%s\" not found\n",
-                __FUNCTION__, object_get_typename(OBJECT(dev)), name);
-        abort();
-    }
-    if (prop->info->type != type) {
-        fprintf(stderr, "%s: property \"%s.%s\" type mismatch\n",
-                __FUNCTION__, object_get_typename(OBJECT(dev)), name);
-        abort();
-    }
-    qdev_prop_cpy(dev, prop, src);
-}
-
 void qdev_prop_set_bit(DeviceState *dev, const char *name, bool value)
 {
     Error *errp = NULL;
@@ -1187,7 +1169,13 @@ void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t *value)
 
 void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
 {
-    qdev_prop_set(dev, name, &value, PROP_TYPE_PTR);
+    Property *prop;
+    void **ptr;
+
+    prop = qdev_prop_find(dev, name);
+    assert(prop && prop->info->type == PROP_TYPE_PTR);
+    ptr = qdev_get_prop_ptr(dev, prop);
+    *ptr = value;
 }
 
 void qdev_prop_set_defaults(DeviceState *dev, Property *props)
-- 
1.7.7.6

[Qemu-devel] [PATCH 16/16] qdev: initialize properties via QOM

[Paolo Bonzini]

Similarly, use the object properties also to set the default
values of the qdev properties.  This requires reordering
registration and initialization.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-properties.c |   20 +++++++-------------
 hw/qdev.c            |    4 ++--
 hw/qdev.h            |   11 +++++++----
 3 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index d7e5356..760240e 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -26,17 +26,6 @@ static void bit_prop_set(DeviceState *dev, Property *props, bool val)
         *p &= ~mask;
 }
 
-static void qdev_prop_cpy(DeviceState *dev, Property *props, void *src)
-{
-    if (props->info->type == PROP_TYPE_BIT) {
-        bool *defval = src;
-        bit_prop_set(dev, props, *defval);
-    } else {
-        char *dst = qdev_get_prop_ptr(dev, props);
-        memcpy(dst, src, props->info->size);
-    }
-}
-
 /* Bit */
 static int parse_bit(DeviceState *dev, Property *prop, const char *str)
 {
@@ -1180,12 +1169,17 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
 
 void qdev_prop_set_defaults(DeviceState *dev, Property *props)
 {
+    Object *obj = OBJECT(dev);
     if (!props)
         return;
     while (props->name) {
-        if (props->defval) {
-            qdev_prop_cpy(dev, props, props->defval);
+        Error *errp = NULL;
+        if (props->qtype == QTYPE_QBOOL) {
+            object_property_set_bool(obj, props->defval, props->name, &errp);
+        } else if (props->qtype == QTYPE_QINT) {
+            object_property_set_int(obj, props->defval, props->name, &errp);
         }
+        assert(!errp);
         props++;
     }
 }
diff --git a/hw/qdev.c b/hw/qdev.c
index f719f14..dc1d1a1 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -86,11 +86,11 @@ void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
     dev->parent_bus = bus;
     QTAILQ_INSERT_HEAD(&bus->children, dev, sibling);
 
-    qdev_prop_set_defaults(dev, dev->parent_bus->info->props);
     for (prop = qdev_get_bus_info(dev)->props; prop && prop->name; prop++) {
         qdev_property_add_legacy(dev, prop, NULL);
         qdev_property_add_static(dev, prop, NULL);
     }
+    qdev_prop_set_defaults(dev, dev->parent_bus->info->props);
 }
 
 /* Create a new device.  This only initializes the device state structure
@@ -612,13 +612,13 @@ static void device_initfn(Object *obj)
     dev->instance_id_alias = -1;
     dev->state = DEV_STATE_CREATED;
 
-    qdev_prop_set_defaults(dev, qdev_get_props(dev));
     for (prop = qdev_get_props(dev); prop && prop->name; prop++) {
         qdev_property_add_legacy(dev, prop, NULL);
         qdev_property_add_static(dev, prop, NULL);
     }
 
     object_property_add_str(OBJECT(dev), "type", qdev_get_type, NULL, NULL);
+    qdev_prop_set_defaults(dev, qdev_get_props(dev));
 }
 
 /* Unlink device from bus and free the structure.  */
diff --git a/hw/qdev.h b/hw/qdev.h
index c0e5600..60c226b 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -112,8 +112,9 @@ struct Property {
     const char   *name;
     PropertyInfo *info;
     int          offset;
-    int          bitnr;
-    void         *defval;
+    uint8_t      bitnr;
+    uint8_t      qtype;
+    int64_t      defval;
 };
 
 enum PropertyType {
@@ -252,7 +253,8 @@ extern PropertyInfo qdev_prop_pci_devfn;
         .info      = &(_prop),                                          \
         .offset    = offsetof(_state, _field)                           \
             + type_check(_type,typeof_field(_state, _field)),           \
-        .defval    = (_type[]) { _defval },                             \
+        .qtype     = QTYPE_QINT,                                        \
+        .defval    = (_type)_defval,                                    \
         }
 #define DEFINE_PROP_BIT(_name, _state, _field, _bit, _defval) {  \
         .name      = (_name),                                    \
@@ -260,7 +262,8 @@ extern PropertyInfo qdev_prop_pci_devfn;
         .bitnr    = (_bit),                                      \
         .offset    = offsetof(_state, _field)                    \
             + type_check(uint32_t,typeof_field(_state, _field)), \
-        .defval    = (bool[]) { (_defval) },                     \
+        .qtype     = QTYPE_QBOOL,                                \
+        .defval    = (bool)_defval,                              \
         }
 
 #define DEFINE_PROP_UINT8(_n, _s, _f, _d)                       \
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 01/16] qdev: fix hot-unplug

[Anthony Liguori]

On 02/02/2012 10:45 AM, Paolo Bonzini wrote:
> The reference that is returned by qdev_device_add is never given
> back, so that device_del does not cause the refcount to go to zero
> (and thus does nothing).
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   vl.c |    1 +
>   1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index d88a18c..c63af69 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1746,6 +1746,7 @@ static int device_init_func(QemuOpts *opts, void *opaque)
>       dev = qdev_device_add(opts);
>       if (!dev)
>           return -1;
> +    object_unref(OBJECT(dev));
>       return 0;

Is this still needed with qom-upstream.14?  I fixed a bug on .14 that involved 
child properties that was making device-del sometimes fail.

If it is, what's your test case?  I have a device_del test case that seems to be 
working right now without this patch.

Regards,

Anthony Liguori

>   }
>

Re: [Qemu-devel] [PATCH 02/16] qom: store object with correct type in interface links

[Anthony Liguori]

On 02/02/2012 10:45 AM, Paolo Bonzini wrote:
> When a link property's type is an interface, the code expects the
> implementation object (not the parent object) to be stored in the
> variable.  The parent object does not contain the right vtable.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   qom/object.c |    3 ++-
>   1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index cd517f6..de6484d 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -749,7 +749,8 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
>               target_type = g_strdup(&type[5]);
>               target_type[strlen(target_type) - 2] = 0;
>
> -            if (object_dynamic_cast(target, target_type)) {
> +            target = object_dynamic_cast(target, target_type);
> +            if (target) {
>                   object_ref(target);
>                   *child = target;

Very good catch.

Regards,

Anthony Liguori

>               } else {

Re: [Qemu-devel] [PATCH 01/16] qdev: fix hot-unplug

[Paolo Bonzini]

On 02/02/2012 06:03 PM, Anthony Liguori wrote:
>>
>
> Is this still needed with qom-upstream.14?  I fixed a bug on .14 that
> involved child properties that was making device-del sometimes fail.

Not sure, I tried with .13 but, from the look of it, it should still be 
there.  Regarding the .13->.14 diff:

- you need QTAILQ_FOREACH_SAFE in object_property_del_child.

- you need to check for the existence of the non-aliased name when 
accessing the alias table, because s390 does not have PCI.

> If it is, what's your test case?

I check that the device disappears from "info qtree".  I check with gdb 
that after object_unparent the refcount is zero.

Paolo

Re: [Qemu-devel] [PATCH 01/16] qdev: fix hot-unplug

[Anthony Liguori]

On 02/02/2012 11:29 AM, Paolo Bonzini wrote:
> On 02/02/2012 06:03 PM, Anthony Liguori wrote:
>>>
>>
>> Is this still needed with qom-upstream.14? I fixed a bug on .14 that
>> involved child properties that was making device-del sometimes fail.
>
> Not sure, I tried with .13 but, from the look of it, it should still be there.
> Regarding the .13->.14 diff:
>
> - you need QTAILQ_FOREACH_SAFE in object_property_del_child.

Ack.

>
> - you need to check for the existence of the non-aliased name when accessing the
> alias table, because s390 does not have PCI.

I don't think that's the right strategy as it means that s390 only works if we 
don't include the PCI objects in the build (regardless of whether it uses PCI). 
  This would be defeated if/when we move to having all device objects in a 
single shared library used by all of the qemu executables.

I'd prefer to just drop the aliases for s390.  I don't see a lot of value in it 
and I don't think there are tons of s390 users that will be affected.

>
>> If it is, what's your test case?
>
> I check that the device disappears from "info qtree". I check with gdb that
> after object_unparent the refcount is zero.

Ah, okay, I'll look at this more closely.  Thanks.

Regards,

Anthony Liguori

>
> Paolo
>

Re: [Qemu-devel] [PATCH 04/16] qom: add QObject-based property get/set wrappers

[Anthony Liguori]

On 02/02/2012 10:45 AM, Paolo Bonzini wrote:
> Move the creation of QmpInputVisitor and QmpOutputVisitor from
> qmp.c to qom/object.c, since it's the only practical way to access
> object properties.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   include/qemu/object.h |   24 ++++++++++++++++++++++++
>   qmp.c                 |   17 ++---------------
>   qom/object.c          |   29 +++++++++++++++++++++++++++++
>   3 files changed, 55 insertions(+), 15 deletions(-)

I don't want object.h to have a dependency on QObject.  We need to phase out 
QObject.

Couple things:

1) We shouldn't use generic interfaces to read/write properties from objects. 
We should use type-safe accessors provided by the types themselves.

2) If we want to get fancy, we can add property_set_int, etc. and then implement 
(1) via header files that just call these functions.

Regards,

Anthony Liguori

>
> diff --git a/include/qemu/object.h b/include/qemu/object.h
> index 947cf29..71090f2 100644
> --- a/include/qemu/object.h
> +++ b/include/qemu/object.h
> @@ -542,6 +542,18 @@ void object_property_get(Object *obj, struct Visitor *v, const char *name,
>                            struct Error **errp);
>
>   /**
> + * object_property_get_qobject:
> + * @obj: the object
> + * @name: the name of the property
> + * @errp: returns an error if this function fails
> + *
> + * Returns: the value of the property, converted to QObject, or NULL if
> + * an error occurs.
> + */
> +struct QObject *object_property_get_qobject(Object *obj, const char *name,
> +                                            struct Error **errp);
> +
> +/**
>    * object_property_set:
>    * @obj: the object
>    * @v: the visitor that will be used to write the property value.  This should
> @@ -556,6 +568,18 @@ void object_property_set(Object *obj, struct Visitor *v, const char *name,
>                            struct Error **errp);
>
>   /**
> + * object_property_set_qobject:
> + * @obj: the object
> + * @ret: The value that will be written to the property.
> + * @name: the name of the property
> + * @errp: returns an error if this function fails
> + *
> + * Writes a property to a object.
> + */
> +void object_property_set_qobject(Object *obj, struct QObject *qobj,
> +                                 const char *name, struct Error **errp);
> +
> +/**
>    * @object_property_get_type:
>    * @obj: the object
>    * @name: the name of the property
> diff --git a/qmp.c b/qmp.c
> index 45052cc..c7a81cc 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -21,8 +21,6 @@
>   #include "kvm.h"
>   #include "arch_init.h"
>   #include "hw/qdev.h"
> -#include "qapi/qmp-input-visitor.h"
> -#include "qapi/qmp-output-visitor.h"
>   #include "blockdev.h"
>
>   NameInfo *qmp_query_name(Error **errp)
> @@ -198,7 +196,6 @@ int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret)
>       const char *property = qdict_get_str(qdict, "property");
>       QObject *value = qdict_get(qdict, "value");
>       Error *local_err = NULL;
> -    QmpInputVisitor *mi;
>       Object *obj;
>
>       obj = object_resolve_path(path, NULL);
> @@ -207,10 +204,7 @@ int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret)
>           goto out;
>       }
>
> -    mi = qmp_input_visitor_new(value);
> -    object_property_set(obj, qmp_input_get_visitor(mi), property,&local_err);
> -
> -    qmp_input_visitor_cleanup(mi);
> +    object_property_set_qobject(obj, value, property,&local_err);
>
>   out:
>       if (local_err) {
> @@ -227,7 +221,6 @@ int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret)
>       const char *path = qdict_get_str(qdict, "path");
>       const char *property = qdict_get_str(qdict, "property");
>       Error *local_err = NULL;
> -    QmpOutputVisitor *mo;
>       Object *obj;
>
>       obj = object_resolve_path(path, NULL);
> @@ -236,13 +229,7 @@ int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret)
>           goto out;
>       }
>
> -    mo = qmp_output_visitor_new();
> -    object_property_get(obj, qmp_output_get_visitor(mo), property,&local_err);
> -    if (!local_err) {
> -        *ret = qmp_output_get_qobject(mo);
> -    }
> -
> -    qmp_output_visitor_cleanup(mo);
> +    *ret = object_property_get_qobject(obj, property,&local_err);
>
>   out:
>       if (local_err) {
> diff --git a/qom/object.c b/qom/object.c
> index 299e146..13c8bec 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -13,6 +13,8 @@
>   #include "qemu/object.h"
>   #include "qemu-common.h"
>   #include "qapi/qapi-visit-core.h"
> +#include "qapi/qmp-input-visitor.h"
> +#include "qapi/qmp-output-visitor.h"
>
>   #define MAX_INTERFACES 32
>
> @@ -646,6 +648,33 @@ void object_property_set(Object *obj, Visitor *v, const char *name,
>       }
>   }
>
> +void object_property_set_qobject(Object *obj, QObject *value,
> +                                 const char *name, Error **errp)
> +{
> +    QmpInputVisitor *mi;
> +    mi = qmp_input_visitor_new(value);
> +    object_property_set(obj, qmp_input_get_visitor(mi), name, errp);
> +
> +    qmp_input_visitor_cleanup(mi);
> +}
> +
> +QObject *object_property_get_qobject(Object *obj, const char *name,
> +                                     Error **errp)
> +{
> +    QObject *ret = NULL;
> +    Error *local_err = NULL;
> +    QmpOutputVisitor *mo;
> +
> +    mo = qmp_output_visitor_new();
> +    object_property_get(obj, qmp_output_get_visitor(mo), name,&local_err);
> +    if (!local_err) {
> +        ret = qmp_output_get_qobject(mo);
> +    }
> +    error_propagate(errp, local_err);
> +    qmp_output_visitor_cleanup(mo);
> +    return ret;
> +}
> +
>   const char *object_property_get_type(Object *obj, const char *name, Error **errp)
>   {
>       ObjectProperty *prop = object_property_find(obj, name);

Re: [Qemu-devel] [PATCH 01/16] qdev: fix hot-unplug

[Alexander Graf]

On 02.02.2012, at 20:01, Anthony Liguori wrote:

> On 02/02/2012 11:29 AM, Paolo Bonzini wrote:
>> On 02/02/2012 06:03 PM, Anthony Liguori wrote:
>>>> 
>>> 
>>> Is this still needed with qom-upstream.14? I fixed a bug on .14 that
>>> involved child properties that was making device-del sometimes fail.
>> 
>> Not sure, I tried with .13 but, from the look of it, it should still be there.
>> Regarding the .13->.14 diff:
>> 
>> - you need QTAILQ_FOREACH_SAFE in object_property_del_child.
> 
> Ack.
> 
>> 
>> - you need to check for the existence of the non-aliased name when accessing the
>> alias table, because s390 does not have PCI.
> 
> I don't think that's the right strategy as it means that s390 only works if we don't include the PCI objects in the build (regardless of whether it uses PCI).  This would be defeated if/when we move to having all device objects in a single shared library used by all of the qemu executables.
> 
> I'd prefer to just drop the aliases for s390.  I don't see a lot of value in it and I don't think there are tons of s390 users that will be affected.

The reason for the aliases is to make -drive and -net work. If you have alternatives to aliases there, I'm happy to go with them.


Alex

Re: [Qemu-devel] [PATCH 04/16] qom: add QObject-based property get/set wrappers

[Andreas Färber]

Am 02.02.2012 20:06, schrieb Anthony Liguori:
> On 02/02/2012 10:45 AM, Paolo Bonzini wrote:
>> Move the creation of QmpInputVisitor and QmpOutputVisitor from
>> qmp.c to qom/object.c, since it's the only practical way to access
>> object properties.
>>
>> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>> ---
>>   include/qemu/object.h |   24 ++++++++++++++++++++++++
>>   qmp.c                 |   17 ++---------------
>>   qom/object.c          |   29 +++++++++++++++++++++++++++++
>>   3 files changed, 55 insertions(+), 15 deletions(-)
> 
> I don't want object.h to have a dependency on QObject.  We need to phase
> out QObject.

We did get that dependency though by your move of the property code to
object.c. As you will see shortly, we now need qobject-obj-y and
qapi-obj-y plus some stubs to make the user emulators compile with QOM.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 04/16] qom: add QObject-based property get/set wrappers

[Paolo Bonzini]

On 02/02/2012 08:06 PM, Anthony Liguori wrote:
> I don't want object.h to have a dependency on QObject.  We need to phase
> out QObject.

The header doesn't.

> Couple things:
>
> 1) We shouldn't use generic interfaces to read/write properties from
> objects. We should use type-safe accessors provided by the types
> themselves.
>
> 2) If we want to get fancy, we can add property_set_int, etc. and then
> implement (1) via header files that just call these functions.

That's what patch 5 does.  But writing visitors in C is a royal PITA. 
The only sane way to do so is via QObject.

Paolo

Re: [Qemu-devel] [PATCH 04/16] qom: add QObject-based property get/set wrappers

[Paolo Bonzini]

On 02/02/2012 08:24 PM, Paolo Bonzini wrote:
>>
>> 1) We shouldn't use generic interfaces to read/write properties from
>> objects. We should use type-safe accessors provided by the types
>> themselves.

That doesn't change the fact that we need simple wrappers using C types 
(at various levels: object_property_set_qobject, object_property_set, 
qdev_set_*) to implement these type-safe accessors on top of dynamic 
properties.

>> 2) If we want to get fancy, we can add property_set_int, etc. and then
>> implement (1) via header files that just call these functions.
>
> That's what patch 5 does.  But writing visitors in C is a royal PITA.
> The only sane way to do so is via QObject.

BTW, I don't really think it's possible to proceed on this except by 
accepting compromises.  We need to be the #1 QOM client, _now_ or it 
will remain buggy & bitrot.

Paolo

Re: [Qemu-devel] [PATCH 04/16] qom: add QObject-based property get/set wrappers

[Anthony Liguori]

On 02/02/2012 01:24 PM, Paolo Bonzini wrote:
> On 02/02/2012 08:06 PM, Anthony Liguori wrote:
>> I don't want object.h to have a dependency on QObject. We need to phase
>> out QObject.
>
> The header doesn't.
>
>> Couple things:
>>
>> 1) We shouldn't use generic interfaces to read/write properties from
>> objects. We should use type-safe accessors provided by the types
>> themselves.
>>
>> 2) If we want to get fancy, we can add property_set_int, etc. and then
>> implement (1) via header files that just call these functions.
>
> That's what patch 5 does. But writing visitors in C is a royal PITA. The only
> sane way to do so is via QObject.

You just need a variant visitor.  It's pretty simple to do, essentially:

typedef struct VariantVisitor
{
     Visitor parent;
     enum { VV_INT, VV_STR } kind;
     union { int64_t v_int; char *v_str };
} VariantVisitor;

/* input */
static void visit_int(...)
{
    v->kind = TYPE_INT;
    v->v_int = *value;
}

/* output */
static void visit_int(...)
{
    assert(v->kind == TYPE_INT);
    *value = v->v_int;
}

void variant_visitor_set_int(VariantVisitor *v, int64_t value)
{
    v->kind = TYPE_INT;
    v->v_int = value;
}

The only types that matter are int and string so the variant visitor is pretty 
simple.

Regards,

Anthony Liguori

>
> Paolo
>
>

Re: [Qemu-devel] [PATCH 04/16] qom: add QObject-based property get/set wrappers

[Anthony Liguori]

On 02/02/2012 01:29 PM, Paolo Bonzini wrote:
> On 02/02/2012 08:24 PM, Paolo Bonzini wrote:
>>>
>>> 1) We shouldn't use generic interfaces to read/write properties from
>>> objects. We should use type-safe accessors provided by the types
>>> themselves.
>
> That doesn't change the fact that we need simple wrappers using C types (at
> various levels: object_property_set_qobject, object_property_set, qdev_set_*) to
> implement these type-safe accessors on top of dynamic properties.
>
>>> 2) If we want to get fancy, we can add property_set_int, etc. and then
>>> implement (1) via header files that just call these functions.
>>
>> That's what patch 5 does. But writing visitors in C is a royal PITA.
>> The only sane way to do so is via QObject.
>
> BTW, I don't really think it's possible to proceed on this except by accepting
> compromises. We need to be the #1 QOM client, _now_ or it will remain buggy &
> bitrot.

Not disagreeing at all with the goal, just the implementation :-)

We can pretty easily avoid a QObject dependency.  I can throw together that 
patch if you'd like.

Regards,

Anthony Liguori

>
> Paolo
>

Re: [Qemu-devel] [PATCH 01/16] qdev: fix hot-unplug

[Anthony Liguori]

On 02/02/2012 01:07 PM, Alexander Graf wrote:
>
> On 02.02.2012, at 20:01, Anthony Liguori wrote:
>
>> On 02/02/2012 11:29 AM, Paolo Bonzini wrote:
>>> On 02/02/2012 06:03 PM, Anthony Liguori wrote:
>>>>>
>>>>
>>>> Is this still needed with qom-upstream.14? I fixed a bug on .14 that
>>>> involved child properties that was making device-del sometimes fail.
>>>
>>> Not sure, I tried with .13 but, from the look of it, it should still be there.
>>> Regarding the .13->.14 diff:
>>>
>>> - you need QTAILQ_FOREACH_SAFE in object_property_del_child.
>>
>> Ack.
>>
>>>
>>> - you need to check for the existence of the non-aliased name when accessing the
>>> alias table, because s390 does not have PCI.
>>
>> I don't think that's the right strategy as it means that s390 only works if we don't include the PCI objects in the build (regardless of whether it uses PCI).  This would be defeated if/when we move to having all device objects in a single shared library used by all of the qemu executables.
>>
>> I'd prefer to just drop the aliases for s390.  I don't see a lot of value in it and I don't think there are tons of s390 users that will be affected.
>
> The reason for the aliases is to make -drive and -net work. If you have alternatives to aliases there, I'm happy to go with them.

We can simply do a const char *target_get_virtio_net_type(void) in arch_init.c.

Not pretty, but we can later fix the -drive/-net calls to not require this.

Regards,

Anthony Liguori

>
>
> Alex
>
>

Re: [Qemu-devel] [PATCH 09/16] qdev: remove parse/print methods for mac properties

[Anthony Liguori]

On 02/02/2012 10:45 AM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   hw/qdev-properties.c |   61 ++++++++++++++++++++++++++++++++++---------------
>   1 files changed, 42 insertions(+), 19 deletions(-)
>
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index 0a293af..4fb5cf8 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -848,46 +848,69 @@ PropertyInfo qdev_prop_ptr = {
>    *   01:02:03:04:05:06
>    *   01-02-03-04-05-06
>    */
> -static int parse_mac(DeviceState *dev, Property *prop, const char *str)
> +static void get_mac(Object *obj, Visitor *v, void *opaque,
> +                    const char *name, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    MACAddr *mac = qdev_get_prop_ptr(dev, prop);
> +    char buffer[2 * 6 + 5 + 1];
> +    char *p = buffer;
> +
> +    snprintf(buffer, sizeof(buffer), "%02x:%02x:%02x:%02x:%02x:%02x",
> +             mac->a[0], mac->a[1], mac->a[2],
> +             mac->a[3], mac->a[4], mac->a[5]);
> +
> +    visit_type_str(v,&p, name, errp);
> +}

Part of me wonders, given that we can structure properties in a more friendly 
way, would we still do macs as strings?

Would we instead do a list of ints or something like that?

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 10/16] qdev: make the non-legacy pci address property accept an integer

[Anthony Liguori]

On 02/02/2012 10:45 AM, Paolo Bonzini wrote:
> PCI addresses are set with qdev_prop_uint32.  Thus we make the QOM
> property accept a device and function encoded in an 8-bit integer,
> instead of the magic dd.f hex string.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>

Isn't this a compatibility breaker?

Won't this break libvirt's usage of -device addr=0.2 ?

Regards,

Anthony Liguori

> ---
>   hw/qdev-properties.c |   25 +++++++------------------
>   1 files changed, 7 insertions(+), 18 deletions(-)
>
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index 4fb5cf8..e4bcc6d 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -950,30 +950,19 @@ static int print_pci_devfn(DeviceState *dev, Property *prop, char *dest, size_t
>       }
>   }
>
> -static void get_pci_devfn(Object *obj, Visitor *v, void *opaque,
> -                          const char *name, Error **errp)
> -{
> -    DeviceState *dev = DEVICE(obj);
> -    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,
> +    .get   = get_int32,
> +    .set   = set_int32,
> +    /* FIXME: this should be -1...255, but the address is stored
> +     * into an uint32_t rather than int32_t.
> +     */
> +    .min   = 0,
> +    .max   = 0xFFFFFFFFULL,
>   };
>
>   /* --- public helpers --- */

Re: [Qemu-devel] [PATCH 04/16] qom: add QObject-based property get/set wrappers

[Paolo Bonzini]

On 02/02/2012 08:36 PM, Anthony Liguori wrote:
> The only types that matter are int and string so the variant visitor is
> pretty simple.

Sure, only ~150 lines of code.  I also do not disagree with the goals 
(mine and yours), just with the priorities. :)

Paolo

Re: [Qemu-devel] [PATCH 10/16] qdev: make the non-legacy pci address property accept an integer

[Paolo Bonzini]

On 02/02/2012 09:07 PM, Anthony Liguori wrote:
>>
>> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>
> Isn't this a compatibility breaker?
>
> Won't this break libvirt's usage of -device addr=0.2 ?

Nope, the legacy property still keeps the dd.f format.  This is only for 
QOM (and internal use by qdev).

Paolo

Re: [Qemu-devel] [PATCH 01/16] qdev: fix hot-unplug

[Alexander Graf]

On 02.02.2012, at 21:03, Anthony Liguori wrote:

> On 02/02/2012 01:07 PM, Alexander Graf wrote:
>> 
>> On 02.02.2012, at 20:01, Anthony Liguori wrote:
>> 
>>> On 02/02/2012 11:29 AM, Paolo Bonzini wrote:
>>>> On 02/02/2012 06:03 PM, Anthony Liguori wrote:
>>>>>> 
>>>>> 
>>>>> Is this still needed with qom-upstream.14? I fixed a bug on .14 that
>>>>> involved child properties that was making device-del sometimes fail.
>>>> 
>>>> Not sure, I tried with .13 but, from the look of it, it should still be there.
>>>> Regarding the .13->.14 diff:
>>>> 
>>>> - you need QTAILQ_FOREACH_SAFE in object_property_del_child.
>>> 
>>> Ack.
>>> 
>>>> 
>>>> - you need to check for the existence of the non-aliased name when accessing the
>>>> alias table, because s390 does not have PCI.
>>> 
>>> I don't think that's the right strategy as it means that s390 only works if we don't include the PCI objects in the build (regardless of whether it uses PCI).  This would be defeated if/when we move to having all device objects in a single shared library used by all of the qemu executables.
>>> 
>>> I'd prefer to just drop the aliases for s390.  I don't see a lot of value in it and I don't think there are tons of s390 users that will be affected.
>> 
>> The reason for the aliases is to make -drive and -net work. If you have alternatives to aliases there, I'm happy to go with them.
> 
> We can simply do a const char *target_get_virtio_net_type(void) in arch_init.c.
> 
> Not pretty, but we can later fix the -drive/-net calls to not require this.

Anything that works. The only reason to have the aliases for me really was to not have target awareness in -drive and -net. So if you're feeling better with an arch callback, I'm definitely fine with that too.

Alex

Re: [Qemu-devel] [PATCH 04/16] qom: add QObject-based property get/set wrappers

[Anthony Liguori]

On 02/02/2012 01:21 PM, Andreas Färber wrote:
> Am 02.02.2012 20:06, schrieb Anthony Liguori:
>> On 02/02/2012 10:45 AM, Paolo Bonzini wrote:
>>> Move the creation of QmpInputVisitor and QmpOutputVisitor from
>>> qmp.c to qom/object.c, since it's the only practical way to access
>>> object properties.
>>>
>>> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>>> ---
>>>    include/qemu/object.h |   24 ++++++++++++++++++++++++
>>>    qmp.c                 |   17 ++---------------
>>>    qom/object.c          |   29 +++++++++++++++++++++++++++++
>>>    3 files changed, 55 insertions(+), 15 deletions(-)
>>
>> I don't want object.h to have a dependency on QObject.  We need to phase
>> out QObject.
>
> We did get that dependency though by your move of the property code to
> object.c. As you will see shortly, we now need qobject-obj-y and
> qapi-obj-y plus some stubs to make the user emulators compile with QOM.
>

That's an implementation detail of Error, that's not because QObject is used 
anywhere in QOM.

Regards,

Anthony Liguori

> Andreas
>

Re: [Qemu-devel] [PATCH 04/16] qom: add QObject-based property get/set wrappers

[Anthony Liguori]

On 02/02/2012 02:08 PM, Paolo Bonzini wrote:
> On 02/02/2012 08:36 PM, Anthony Liguori wrote:
>> The only types that matter are int and string so the variant visitor is
>> pretty simple.
>
> Sure, only ~150 lines of code. I also do not disagree with the goals (mine and
> yours), just with the priorities. :)

That's fine, it's a priority for me, so I'm happy to send a patch to your series.

I think it's important to maintain strict modularity at the core layer of QOM.

Regards,

Anthony Liguori

>
> Paolo
>

Re: [Qemu-devel] [PATCH 07/16] qdev: allow reusing get/set for legacy property

[Andreas Färber]

Am 02.02.2012 17:45, schrieb Paolo Bonzini:
> In some cases, a legacy property does need a special print method
> but not a special parse method.  In this case, we can reuse the get/set
> from the static (non-legacy) property.
> 
> If neither parse nor print is needed, though, do not register the
> legacy property at all.  The previous patch ensures that the right
> fallback will be used.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/qdev-monitor.c    |    5 ++---
>  hw/qdev-properties.c |    6 +++---
>  hw/qdev.c            |   11 +++++++----
>  3 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
> index 64505b4..e21bd50 100644
> --- a/hw/qdev-monitor.c
> +++ b/hw/qdev-monitor.c
> @@ -489,8 +489,8 @@ static void qdev_print_props(Monitor *mon, DeviceState *dev, Property *props,
>  {
>      if (!props)
>          return;
> -    while (props->name) {
> -        Error *err;
> +    for (; props->name; props++) {
> +        Error *err = NULL;

Do either of these fix a bug? Should be mentioned or avoided.

>          char *value;
>          char *legacy_name = g_strdup_printf("legacy-%s", props->name);
>          if (object_property_get_type(OBJECT(dev), legacy_name, NULL)) {
> @@ -507,7 +507,6 @@ static void qdev_print_props(Monitor *mon, DeviceState *dev, Property *props,
>          qdev_printf("%s-prop: %s = %s\n", prefix, props->name,
>                      value && *value ? value : "<null>");
>          g_free(value);
> -        props++;
>      }
>  }
>  
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index 7c41140..16f9b22 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -1025,13 +1025,13 @@ void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
>  int qdev_prop_parse(DeviceState *dev, const char *name, const char *value)
>  {
>      char *legacy_name;
> -    Error *err;
> +    Error *err = NULL;
>  
>      legacy_name = g_strdup_printf("legacy-%s", name);
>      if (object_property_get_type(OBJECT(dev), legacy_name, NULL)) {
> -        object_property_set_str(OBJECT(dev), legacy_name, value, &err);
> +        object_property_set_str(OBJECT(dev), value, legacy_name, &err);
>      } else {
> -        object_property_set_str(OBJECT(dev), name, value, &err);
> +        object_property_set_str(OBJECT(dev), value, name, &err);
>      }

Bugfix?

>      g_free(legacy_name);
>  

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 07/16] qdev: allow reusing get/set for legacy property

[Paolo Bonzini]

On 02/02/2012 11:38 PM, Andreas Färber wrote:
> Am 02.02.2012 17:45, schrieb Paolo Bonzini:
>> In some cases, a legacy property does need a special print method
>> but not a special parse method.  In this case, we can reuse the get/set
>> from the static (non-legacy) property.
>>
>> If neither parse nor print is needed, though, do not register the
>> legacy property at all.  The previous patch ensures that the right
>> fallback will be used.
>>
>> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>> ---
>>   hw/qdev-monitor.c    |    5 ++---
>>   hw/qdev-properties.c |    6 +++---
>>   hw/qdev.c            |   11 +++++++----
>>   3 files changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/qdev-monitor.c b/hw/qdev-monitor.c
>> index 64505b4..e21bd50 100644
>> --- a/hw/qdev-monitor.c
>> +++ b/hw/qdev-monitor.c
>> @@ -489,8 +489,8 @@ static void qdev_print_props(Monitor *mon, DeviceState *dev, Property *props,
>>   {
>>       if (!props)
>>           return;
>> -    while (props->name) {
>> -        Error *err;
>> +    for (; props->name; props++) {
>> +        Error *err = NULL;
>
> Do either of these fix a bug? Should be mentioned or avoided. [...]
>
> Bugfix?

Yes, I squashed these in the wrong patch, thanks for catching it.

Paolo

Re: [Qemu-devel] [PATCH 02/16] qom: store object with correct type in interface links

[Paolo Bonzini]

On 02/02/2012 06:05 PM, Anthony Liguori wrote:
> On 02/02/2012 10:45 AM, Paolo Bonzini wrote:
>> When a link property's type is an interface, the code expects the
>> implementation object (not the parent object) to be stored in the
>> variable. The parent object does not contain the right vtable.
>>
>> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>> ---
>> qom/object.c | 3 ++-
>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/qom/object.c b/qom/object.c
>> index cd517f6..de6484d 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -749,7 +749,8 @@ static void object_set_link_property(Object *obj,
>> Visitor *v, void *opaque,
>> target_type = g_strdup(&type[5]);
>> target_type[strlen(target_type) - 2] = 0;
>>
>> - if (object_dynamic_cast(target, target_type)) {
>> + target = object_dynamic_cast(target, target_type);
>> + if (target) {
>> object_ref(target);
>> *child = target;
>
> Very good catch.

But when we implement type-based search for partial paths it will be 
fixed automatically (because object_resolve_path will have to do a 
dynamic cast on its own).  Let's do that instead.

I'll rebase the patch while travelling, since I have to convert 
LostTickPolicy as well.  Let's look at this after the weekend.

Paolo

Re: [Qemu-devel] [PATCH 10/16] qdev: make the non-legacy pci address property accept an integer

[Anthony Liguori]

On 02/02/2012 02:19 PM, Paolo Bonzini wrote:
> On 02/02/2012 09:07 PM, Anthony Liguori wrote:
>>>
>>> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>>
>> Isn't this a compatibility breaker?
>>
>> Won't this break libvirt's usage of -device addr=0.2 ?
>
> Nope, the legacy property still keeps the dd.f format. This is only for QOM (and
> internal use by qdev).

Ah, I forgot we duplicate the properties here.

Since there is now a programmatic mapping between legacy properties types and 
non-legacy property types, could we remove the legacy properties that now have 
well behaved types and add some code to device_add to maintain compatibility?

Regards,

Anthony Liguori

> Paolo
>

Re: [Qemu-devel] [PATCH 01/16] qdev: fix hot-unplug

[Anthony Liguori]

On 02/02/2012 10:45 AM, Paolo Bonzini wrote:
> The reference that is returned by qdev_device_add is never given
> back, so that device_del does not cause the refcount to go to zero
> (and thus does nothing).
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>

This isn't needed in qom-upstream.14.  Here's why:

object_init does not increase the reference count

object_property_add_child increases the reference count
object_new increases the reference count

object_delete decrements the reference count
object_property_del_child decreases the reference count

object_delete calls object_property_del_child(obj->parent, obj)

qdev_device_add calls object_new and object_property_add_child
  -> ref == 2

qdev_device_del calls object_delete
  -> ref -= 2

In qom-upstream.13, object_delete wasn't calling object_property_del_child which 
is why you saw the behavior you did.  This problem would still exist with a 
composed device so dropping the reference here isn't enough.

Regards,

Anthony Liguori

> ---
>   vl.c |    1 +
>   1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index d88a18c..c63af69 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1746,6 +1746,7 @@ static int device_init_func(QemuOpts *opts, void *opaque)
>       dev = qdev_device_add(opts);
>       if (!dev)
>           return -1;
> +    object_unref(OBJECT(dev));
>       return 0;
>   }
>

Re: [Qemu-devel] [PATCH 01/16] qdev: fix hot-unplug

[Anthony Liguori]

On 02/02/2012 01:07 PM, Alexander Graf wrote:
>
> On 02.02.2012, at 20:01, Anthony Liguori wrote:
>
>> On 02/02/2012 11:29 AM, Paolo Bonzini wrote:
>>> On 02/02/2012 06:03 PM, Anthony Liguori wrote:
>>>>>
>>>>
>>>> Is this still needed with qom-upstream.14? I fixed a bug on .14 that
>>>> involved child properties that was making device-del sometimes fail.
>>>
>>> Not sure, I tried with .13 but, from the look of it, it should still be there.
>>> Regarding the .13->.14 diff:
>>>
>>> - you need QTAILQ_FOREACH_SAFE in object_property_del_child.
>>
>> Ack.
>>
>>>
>>> - you need to check for the existence of the non-aliased name when accessing the
>>> alias table, because s390 does not have PCI.
>>
>> I don't think that's the right strategy as it means that s390 only works if we don't include the PCI objects in the build (regardless of whether it uses PCI).  This would be defeated if/when we move to having all device objects in a single shared library used by all of the qemu executables.
>>
>> I'd prefer to just drop the aliases for s390.  I don't see a lot of value in it and I don't think there are tons of s390 users that will be affected.
>
> The reason for the aliases is to make -drive and -net work. If you have alternatives to aliases there, I'm happy to go with them.

Um, but I see (in s390-virtio.c):


     for(i = 0; i < nb_nics; i++) {
         NICInfo *nd = &nd_table[i];
         DeviceState *dev;

         if (!nd->model) {
             nd->model = g_strdup("virtio");
         }

         if (strcmp(nd->model, "virtio")) {
             fprintf(stderr, "S390 only supports VirtIO nics\n");
             exit(1);
         }

         dev = qdev_create((BusState *)s390_bus, "virtio-net-s390");
         qdev_set_nic_properties(dev, nd);
         qdev_init_nofail(dev);
     }

     /* Create VirtIO disk drives */
     for(i = 0; i < MAX_BLK_DEVS; i++) {
         DriveInfo *dinfo;
         DeviceState *dev;

         dinfo = drive_get(IF_IDE, 0, i);
         if (!dinfo) {
             continue;
         }

         dev = qdev_create((BusState *)s390_bus, "virtio-blk-s390");
         qdev_prop_set_drive_nofail(dev, "drive", dinfo->bdrv);
         qdev_init_nofail(dev);
     }

So s390 totally ignores the -drive if parameter and will only accept virtio for 
-net.

 From what I can tell, it's not an issue.  But if we need it, we can do:

diff --git a/arch_init.h b/arch_init.h
index 828256c..bfbd9e1 100644
--- a/arch_init.h
+++ b/arch_init.h
@@ -32,4 +32,9 @@ int tcg_available(void);
  int kvm_available(void);
  int xen_available(void);

+static inline int target_get_arch(void)
+{
+    return arch_type;
+}
+
  #endif
diff --git a/blockdev.c b/blockdev.c
index 7e4c548..caa9205 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -565,7 +565,15 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
      case IF_VIRTIO:
          /* add virtio block device */
          opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0);
-        qemu_opt_set(opts, "driver", "virtio-blk");
+        switch(target_get_arch()) {
+        case QEMU_ARCH_S390X:
+            qemu_opt_set(opts, "driver", "virtio-blk-s390");
+            break;
+        default:
+            qemu_opt_set(opts, "driver", "virtio-blk-pci");
+            break;
+        }
+
          qemu_opt_set(opts, "drive", dinfo->id);
          if (devaddr)
              qemu_opt_set(opts, "addr", devaddr);
diff --git a/blockdev.c b/blockdev.c
index 7e4c548..caa9205 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -565,7 +565,15 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
      case IF_VIRTIO:
          /* add virtio block device */
          opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0);
-        qemu_opt_set(opts, "driver", "virtio-blk");
+        switch(target_get_arch()) {
+        case QEMU_ARCH_S390X:
+            qemu_opt_set(opts, "driver", "virtio-blk-s390");
+            break;
+        default:
+            qemu_opt_set(opts, "driver", "virtio-blk-pci");
+            break;
+        }
+
          qemu_opt_set(opts, "drive", dinfo->id);
          if (devaddr)
              qemu_opt_set(opts, "addr", devaddr);


Can you confirm what we actually need here?

Regards,

Anthony Liguori

>
>
> Alex
>
>

Re: [Qemu-devel] [PATCH 01/16] qdev: fix hot-unplug

[Alexander Graf]

On 03.02.2012, at 17:37, Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 02/02/2012 01:07 PM, Alexander Graf wrote:
>> 
>> On 02.02.2012, at 20:01, Anthony Liguori wrote:
>> 
>>> On 02/02/2012 11:29 AM, Paolo Bonzini wrote:
>>>> On 02/02/2012 06:03 PM, Anthony Liguori wrote:
>>>>>> 
>>>>> 
>>>>> Is this still needed with qom-upstream.14? I fixed a bug on .14 that
>>>>> involved child properties that was making device-del sometimes fail.
>>>> 
>>>> Not sure, I tried with .13 but, from the look of it, it should still be there.
>>>> Regarding the .13->.14 diff:
>>>> 
>>>> - you need QTAILQ_FOREACH_SAFE in object_property_del_child.
>>> 
>>> Ack.
>>> 
>>>> 
>>>> - you need to check for the existence of the non-aliased name when accessing the
>>>> alias table, because s390 does not have PCI.
>>> 
>>> I don't think that's the right strategy as it means that s390 only works if we don't include the PCI objects in the build (regardless of whether it uses PCI).  This would be defeated if/when we move to having all device objects in a single shared library used by all of the qemu executables.
>>> 
>>> I'd prefer to just drop the aliases for s390.  I don't see a lot of value in it and I don't think there are tons of s390 users that will be affected.
>> 
>> The reason for the aliases is to make -drive and -net work. If you have alternatives to aliases there, I'm happy to go with them.
> 
> Um, but I see (in s390-virtio.c):
> 
> 
>    for(i = 0; i < nb_nics; i++) {
>        NICInfo *nd = &nd_table[i];
>        DeviceState *dev;
> 
>        if (!nd->model) {
>            nd->model = g_strdup("virtio");
>        }
> 
>        if (strcmp(nd->model, "virtio")) {
>            fprintf(stderr, "S390 only supports VirtIO nics\n");
>            exit(1);
>        }
> 
>        dev = qdev_create((BusState *)s390_bus, "virtio-net-s390");
>        qdev_set_nic_properties(dev, nd);
>        qdev_init_nofail(dev);
>    }
> 
>    /* Create VirtIO disk drives */
>    for(i = 0; i < MAX_BLK_DEVS; i++) {
>        DriveInfo *dinfo;
>        DeviceState *dev;
> 
>        dinfo = drive_get(IF_IDE, 0, i);
>        if (!dinfo) {
>            continue;
>        }
> 
>        dev = qdev_create((BusState *)s390_bus, "virtio-blk-s390");
>        qdev_prop_set_drive_nofail(dev, "drive", dinfo->bdrv);
>        qdev_init_nofail(dev);
>    }
> 
> So s390 totally ignores the -drive if

Nope, since virtio drives aren't handled through the IF_ legacy stuff but through qden instantiation. We only fake virtio disks for -hda here (which should be replaced by a default_virtio option in the machine config).

> parameter and will only accept virtio for -net.

It only supports virtio at all, yes. No MMIO there ;).

> 
> From what I can tell, it's not an issue.  But if we need it, we can do:
> 
> diff --git a/arch_init.h b/arch_init.h
> index 828256c..bfbd9e1 100644
> --- a/arch_init.h
> +++ b/arch_init.h
> @@ -32,4 +32,9 @@ int tcg_available(void);
> int kvm_available(void);
> int xen_available(void);
> 
> +static inline int target_get_arch(void)
> +{
> +    return arch_type;

We could also have a machine type field that could be PCI or S390, right? I somehow don't like globals.

And just because it's s390 doesn't tell us anything. Maybe someone clever will find a way to expose PCI in a later machine type and use that for all devices?

The same thing goes for arm and their mmio virtio too btw.

> +}
> +
> #endif
> diff --git a/blockdev.c b/blockdev.c
> index 7e4c548..caa9205 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -565,7 +565,15 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>     case IF_VIRTIO:
>         /* add virtio block device */
>         opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0);
> -        qemu_opt_set(opts, "driver", "virtio-blk");
> +        switch(target_get_arch()) {
> +        case QEMU_ARCH_S390X:
> +            qemu_opt_set(opts, "driver", "virtio-blk-s390");
> +            break;
> +        default:
> +            qemu_opt_set(opts, "driver", "virtio-blk-pci");
> +            break;
> +        }
> +
>         qemu_opt_set(opts, "drive", dinfo->id);
>         if (devaddr)
>             qemu_opt_set(opts, "addr", devaddr);
> diff --git a/blockdev.c b/blockdev.c
> index 7e4c548..caa9205 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -565,7 +565,15 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>     case IF_VIRTIO:
>         /* add virtio block device */
>         opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0);
> -        qemu_opt_set(opts, "driver", "virtio-blk");
> +        switch(target_get_arch()) {
> +        case QEMU_ARCH_S390X:
> +            qemu_opt_set(opts, "driver", "virtio-blk-s390");
> +            break;
> +        default:
> +            qemu_opt_set(opts, "driver", "virtio-blk-pci");
> +            break;
> +        }
> +
>         qemu_opt_set(opts, "drive", dinfo->id);
>         if (devaddr)
>             qemu_opt_set(opts, "addr", devaddr);
> 
> 
> Can you confirm what we actually need here?

Every time you grep for virtio-xxx-pci in the code without an s390 branch it's wrong. Pretty simple, eh? :)

Alex

> 
> Regards,
> 
> Anthony Liguori
> 
>> 
>> 
>> Alex
>> 
>> 
> 

Re: [Qemu-devel] [PATCH 01/16] qdev: fix hot-unplug

[Anthony Liguori]

On 02/03/2012 10:57 AM, Alexander Graf wrote:
>
>
> On 03.02.2012, at 17:37, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>
>> On 02/02/2012 01:07 PM, Alexander Graf wrote:
>>>
>>> On 02.02.2012, at 20:01, Anthony Liguori wrote:
>>>
>>>> On 02/02/2012 11:29 AM, Paolo Bonzini wrote:
>>>>> On 02/02/2012 06:03 PM, Anthony Liguori wrote:
>>>>>>>
>>>>>>
>>>>>> Is this still needed with qom-upstream.14? I fixed a bug on .14 that
>>>>>> involved child properties that was making device-del sometimes fail.
>>>>>
>>>>> Not sure, I tried with .13 but, from the look of it, it should still be there.
>>>>> Regarding the .13->.14 diff:
>>>>>
>>>>> - you need QTAILQ_FOREACH_SAFE in object_property_del_child.
>>>>
>>>> Ack.
>>>>
>>>>>
>>>>> - you need to check for the existence of the non-aliased name when accessing the
>>>>> alias table, because s390 does not have PCI.
>>>>
>>>> I don't think that's the right strategy as it means that s390 only works if we don't include the PCI objects in the build (regardless of whether it uses PCI).  This would be defeated if/when we move to having all device objects in a single shared library used by all of the qemu executables.
>>>>
>>>> I'd prefer to just drop the aliases for s390.  I don't see a lot of value in it and I don't think there are tons of s390 users that will be affected.
>>>
>>> The reason for the aliases is to make -drive and -net work. If you have alternatives to aliases there, I'm happy to go with them.
>>
>> Um, but I see (in s390-virtio.c):
>>
>>
>>     for(i = 0; i<  nb_nics; i++) {
>>         NICInfo *nd =&nd_table[i];
>>         DeviceState *dev;
>>
>>         if (!nd->model) {
>>             nd->model = g_strdup("virtio");
>>         }
>>
>>         if (strcmp(nd->model, "virtio")) {
>>             fprintf(stderr, "S390 only supports VirtIO nics\n");
>>             exit(1);
>>         }
>>
>>         dev = qdev_create((BusState *)s390_bus, "virtio-net-s390");
>>         qdev_set_nic_properties(dev, nd);
>>         qdev_init_nofail(dev);
>>     }
>>
>>     /* Create VirtIO disk drives */
>>     for(i = 0; i<  MAX_BLK_DEVS; i++) {
>>         DriveInfo *dinfo;
>>         DeviceState *dev;
>>
>>         dinfo = drive_get(IF_IDE, 0, i);
>>         if (!dinfo) {
>>             continue;
>>         }
>>
>>         dev = qdev_create((BusState *)s390_bus, "virtio-blk-s390");
>>         qdev_prop_set_drive_nofail(dev, "drive", dinfo->bdrv);
>>         qdev_init_nofail(dev);
>>     }
>>
>> So s390 totally ignores the -drive if
>
> Nope, since virtio drives aren't handled through the IF_ legacy stuff but through qden instantiation. We only fake virtio disks for -hda here (which should be replaced by a default_virtio option in the machine config).
>
>> parameter and will only accept virtio for -net.
>
> It only supports virtio at all, yes. No MMIO there ;).
>
>>
>>  From what I can tell, it's not an issue.  But if we need it, we can do:
>>
>> diff --git a/arch_init.h b/arch_init.h
>> index 828256c..bfbd9e1 100644
>> --- a/arch_init.h
>> +++ b/arch_init.h
>> @@ -32,4 +32,9 @@ int tcg_available(void);
>> int kvm_available(void);
>> int xen_available(void);
>>
>> +static inline int target_get_arch(void)
>> +{
>> +    return arch_type;
>
> We could also have a machine type field that could be PCI or S390, right? I somehow don't like globals.
>
> And just because it's s390 doesn't tell us anything. Maybe someone clever will find a way to expose PCI in a later machine type and use that for all devices?
>
> The same thing goes for arm and their mmio virtio too btw.

Right, you're just pointing out though that the current code is dumb.  I full 
heartedly agree with you :-)

If you added PCI to the build for s390, aliases wouldn't do what you expect anymore.

>
>> +}
>> +
>> #endif
>> diff --git a/blockdev.c b/blockdev.c
>> index 7e4c548..caa9205 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -565,7 +565,15 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>      case IF_VIRTIO:
>>          /* add virtio block device */
>>          opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0);
>> -        qemu_opt_set(opts, "driver", "virtio-blk");
>> +        switch(target_get_arch()) {
>> +        case QEMU_ARCH_S390X:
>> +            qemu_opt_set(opts, "driver", "virtio-blk-s390");
>> +            break;
>> +        default:
>> +            qemu_opt_set(opts, "driver", "virtio-blk-pci");
>> +            break;
>> +        }
>> +
>>          qemu_opt_set(opts, "drive", dinfo->id);
>>          if (devaddr)
>>              qemu_opt_set(opts, "addr", devaddr);
>> diff --git a/blockdev.c b/blockdev.c
>> index 7e4c548..caa9205 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -565,7 +565,15 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>      case IF_VIRTIO:
>>          /* add virtio block device */
>>          opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0);
>> -        qemu_opt_set(opts, "driver", "virtio-blk");
>> +        switch(target_get_arch()) {
>> +        case QEMU_ARCH_S390X:
>> +            qemu_opt_set(opts, "driver", "virtio-blk-s390");
>> +            break;
>> +        default:
>> +            qemu_opt_set(opts, "driver", "virtio-blk-pci");
>> +            break;
>> +        }
>> +
>>          qemu_opt_set(opts, "drive", dinfo->id);
>>          if (devaddr)
>>              qemu_opt_set(opts, "addr", devaddr);
>>
>>
>> Can you confirm what we actually need here?
>
> Every time you grep for virtio-xxx-pci in the code without an s390 branch it's wrong. Pretty simple, eh? :)


Okay, I'm going to go with this change as this is the only place that it seems 
to exist.

Regards,

Anthony Liguori

> Alex
>
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>>
>>>
>>> Alex
>>>
>>>
>>
>

Re: [Qemu-devel] [PATCH 10/16] qdev: make the non-legacy pci address property accept an integer

[Paolo Bonzini]

On 02/03/2012 03:14 PM, Anthony Liguori wrote:
>>>
>>
>> Nope, the legacy property still keeps the dd.f format. This is only
>> for QOM (and
>> internal use by qdev).
>
> Ah, I forgot we duplicate the properties here.
>
> Since there is now a programmatic mapping between legacy properties
> types and non-legacy property types, could we remove the legacy
> properties that now have well behaved types and add some code to
> device_add to maintain compatibility?

I'm not sure... we would trade removal of an ugly concept (the legacy 
properties) with addition of a layering violation (poking into the 
DeviceState subclasses).

We could hide them in qom-list/qom-get, but I'm not sure this would buy 
us anything, either.

Paolo

Re: [Qemu-devel] [PATCH 01/16] qdev: fix hot-unplug

[Paolo Bonzini]

On 02/03/2012 03:27 PM, Anthony Liguori wrote:
> On 02/02/2012 10:45 AM, Paolo Bonzini wrote:
>> The reference that is returned by qdev_device_add is never given
>> back, so that device_del does not cause the refcount to go to zero
>> (and thus does nothing).
>>
>> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>
> This isn't needed in qom-upstream.14. Here's why:
>
> object_init does not increase the reference count
>
> object_property_add_child increases the reference count
> object_new increases the reference count
>
> object_delete decrements the reference count
> object_property_del_child decreases the reference count
>
> object_delete calls object_property_del_child(obj->parent, obj)
>
> qdev_device_add calls object_new and object_property_add_child
> -> ref == 2
>
> qdev_device_del calls object_delete
> -> ref -= 2
>
> In qom-upstream.13, object_delete wasn't calling
> object_property_del_child which is why you saw the behavior you did.
> This problem would still exist with a composed device so dropping the
> reference here isn't enough.

I trust you for now. :)

It really seems like my patch is obviously correct so, if it's not 
needed anymore there may be another bug elsewhere that masks it.

Paolo

Re: [Qemu-devel] [PATCH 10/16] qdev: make the non-legacy pci address property accept an integer

[Paolo Bonzini]

On 02/04/2012 01:21 AM, Paolo Bonzini wrote:
> I'm not sure... we would trade removal of an ugly concept (the legacy
> properties) with addition of a layering violation (poking into the
> DeviceState subclasses).

The main problem here is that you said no to a hierarchy of property 
classes.  This is what would be good here: being able to say "does this 
property have legacy print/parse methods?" and call them if available 
from device_add.

So, you can choose your poison. :)  For now I think the idea in this 
patch series is good enough for its purpose (which is to actually _use_ 
QOM), we can tweak the design and really eliminate the legacy properties 
later.  I don't mind going through multiple iterations as long as the 
state after each iteration is clearly better than before.
f
Paolo

Re: [Qemu-devel] [PATCH 10/16] qdev: make the non-legacy pci address property accept an integer

[Anthony Liguori]

On 02/03/2012 06:43 PM, Paolo Bonzini wrote:
> On 02/04/2012 01:21 AM, Paolo Bonzini wrote:
>> I'm not sure... we would trade removal of an ugly concept (the legacy
>> properties) with addition of a layering violation (poking into the
>> DeviceState subclasses).
>
> The main problem here is that you said no to a hierarchy of property classes.
> This is what would be good here: being able to say "does this property have
> legacy print/parse methods?" and call them if available from device_add.
>
> So, you can choose your poison. :) For now I think the idea in this patch series
> is good enough for its purpose (which is to actually _use_ QOM),

Yeah, I was just thinking out loud.  My plan is to pull your series into my 
qom-rebase branch.

The last few commits on https://github.com/aliguori/qemu/tree/qom-rebase.12 have 
a variant visitor and accessors that use it.

Regards,

Anthony Liguori

> we can tweak
> the design and really eliminate the legacy properties later. I don't mind going
> through multiple iterations as long as the state after each iteration is clearly
> better than before.
> f
> Paolo

Re: [Qemu-devel] [PATCH 01/16] qdev: fix hot-unplug

[Anthony Liguori]

On 02/03/2012 06:27 PM, Paolo Bonzini wrote:
> On 02/03/2012 03:27 PM, Anthony Liguori wrote:
>> On 02/02/2012 10:45 AM, Paolo Bonzini wrote:
>>> The reference that is returned by qdev_device_add is never given
>>> back, so that device_del does not cause the refcount to go to zero
>>> (and thus does nothing).
>>>
>>> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>>
>> This isn't needed in qom-upstream.14. Here's why:
>>
>> object_init does not increase the reference count
>>
>> object_property_add_child increases the reference count
>> object_new increases the reference count
>>
>> object_delete decrements the reference count
>> object_property_del_child decreases the reference count
>>
>> object_delete calls object_property_del_child(obj->parent, obj)
>>
>> qdev_device_add calls object_new and object_property_add_child
>> -> ref == 2
>>
>> qdev_device_del calls object_delete
>> -> ref -= 2
>>
>> In qom-upstream.13, object_delete wasn't calling
>> object_property_del_child which is why you saw the behavior you did.
>> This problem would still exist with a composed device so dropping the
>> reference here isn't enough.
>
> I trust you for now. :)
>
> It really seems like my patch is obviously correct so, if it's not needed
> anymore there may be another bug elsewhere that masks it.

There's no object_ref() in qdev_device_add().  The 2 references come from adding 
a child link to /peripheral and via object_new().

object_free() drops a reference (it's called in qdev_device_del()) and in the 
process of calling object_free(), it also calls object_unparent() which will 
drop the reference from the parent.

I'm not thrilled about the way reference counting is done now.  Perhaps we 
should do a gobject style floating reference...

Regards,

Anthony Liguori

>
> Paolo
>
>

Re: [Qemu-devel] [PATCH 10/16] qdev: make the non-legacy pci address property accept an integer

[Paolo Bonzini]

On 02/04/2012 04:00 AM, Anthony Liguori wrote:
>>
>
> Yeah, I was just thinking out loud.  My plan is to pull your series into
> my qom-rebase branch.
>
> The last few commits on
> https://github.com/aliguori/qemu/tree/qom-rebase.12 have a variant
> visitor and accessors that use it.

Ok, I'll cherry-pick from there and send out an updated patch series 
later today.  I have some more bugfixes and documentation improvements.

I started looking at PROP_PTR.  It's divided in 4:

- things that do not need PROP_PTR (smbus_eeprom, vmmouse);

- OMAP, which can be done now;

- things that need your 4/4 to set up links to devices;

- things that need CPU objects;

Paolo

Re: [Qemu-devel] [PATCH 01/16] qdev: fix hot-unplug

[Paolo Bonzini]

On 02/04/2012 04:03 AM, Anthony Liguori wrote:
> There's no object_ref() in qdev_device_add().  The 2 references come
> from adding a child link to /peripheral and via object_new().

Sure, but there's when the object_new() reference becomes unreachable. 
At this point, if it weren't for /peripheral the device should have 
disappeared.

> object_free() drops a reference (it's called in qdev_device_del()) and
> in the process of calling object_free(), it also calls object_unparent()
> which will drop the reference from the parent.
>
> I'm not thrilled about the way reference counting is done now.  Perhaps
> we should do a gobject style floating reference...

I'm not sure that's a problem.  Rather, the problem is that we are 
(still) mixing manual memory management and refcounting by making 
object_delete drop a reference.

Can you remind me of why you have object_unref separate from 
object_delete?  Is it because you must not delete objects that were 
object_initialize'd rather than object_new'd?  Perhaps we can take care 
of that with a flag elsewhere saying "do not free this object when 
object_unref drops the last ref" (only finalize it).

Thanks for analyzing the behavior.  We don't have to get it right 
immediately as long as we know what's going on, the transition is not 
complete anyway.

Paolo

Re: [Qemu-devel] [PATCH 10/16] qdev: make the non-legacy pci address property accept an integer

[Paolo Bonzini]

On 02/04/2012 07:42 AM, Paolo Bonzini wrote:
> Ok, I'll cherry-pick from there and send out an updated patch series
> later today.

... that won't work unfortunately.  Enums are broken (not your fault 
really, I sent this before losttickpolicy went in); I really dislike 
having a single object that can magically act both as input and output 
visitor; and there is no documentation in the header for the new functions.

I'll stick with the QObject version for now, but move it to a separate file.

Paolo

Re: [Qemu-devel] [PATCH 01/16] qdev: fix hot-unplug

[Anthony Liguori]

On 02/04/2012 12:51 AM, Paolo Bonzini wrote:
> On 02/04/2012 04:03 AM, Anthony Liguori wrote:
>> There's no object_ref() in qdev_device_add(). The 2 references come
>> from adding a child link to /peripheral and via object_new().
>
> Sure, but there's when the object_new() reference becomes unreachable. At this
> point, if it weren't for /peripheral the device should have disappeared.
>
>> object_free() drops a reference (it's called in qdev_device_del()) and
>> in the process of calling object_free(), it also calls object_unparent()
>> which will drop the reference from the parent.
>>
>> I'm not thrilled about the way reference counting is done now. Perhaps
>> we should do a gobject style floating reference...
>
> I'm not sure that's a problem. Rather, the problem is that we are (still) mixing
> manual memory management and refcounting by making object_delete drop a reference.
>
> Can you remind me of why you have object_unref separate from object_delete? Is
> it because you must not delete objects that were object_initialize'd rather than
> object_new'd? Perhaps we can take care of that with a flag elsewhere saying "do
> not free this object when object_unref drops the last ref" (only finalize it).

I really didn't want to bake allocation into the type interface.  I think 
there's another more robust way to handle this.

We need signal support.  I'd really like to have generic signals that showed up 
a signal<> property type, that could also be registered over QMP, but in the 
interim, we can probably just an ad-hoc NotifierList and do something better later.

Anyway, we should have a "destroy" signal and a "delete" signal.  destroy is 
fired when object_finalize() is called at the very beginning of the function. 
delete is fired at the very end of object_finalize().

We would change object_delete() to call object_unref and then object_finalize(). 
  We would connect a "delete" event in object_new() that would free the memory.

This would also allow object_property_add_child() to connect to the "destroy" 
event such that it could remove the reference it holds on the child.

I'll work up a patch.  I've got some ideas about how to do generic signals too 
but I'd prefer to wait to introduce all of that.

Regards,

Anthony Liguori


> Thanks for analyzing the behavior. We don't have to get it right immediately as
> long as we know what's going on, the transition is not complete anyway.
>
> Paolo
>