[Qemu-devel] [PATCH 0/4] qom: link property fixes

[Qemu-devel] [PATCH 0/4] qom: link property fixes

[Stefan Hajnoczi]

There are two problems with QOM link properties:

1. There are refcount leaks in many object_property_add_link() callers.
2. There is no way to prevent link properties from being set after a device is
   realized.

This patch series fixes these issues by extending the
object_property_add_link() function.

Stefan Hajnoczi (4):
  qom: split object_property_set_link()
  qom: make QOM link property unref optional
  qom: add set() argument to object_property_add_link()
  virtio-rng: avoid default_backend refcount leak

 hw/core/qdev-properties.c    |  12 ++++
 hw/core/qdev.c               |   9 ++-
 hw/dma/xilinx_axidma.c       |  16 ++++--
 hw/net/xilinx_axienet.c      |  16 ++++--
 hw/pcmcia/pxa2xx.c           |   5 +-
 hw/s390x/s390-virtio-bus.c   |   4 +-
 hw/s390x/virtio-ccw.c        |   4 +-
 hw/virtio/virtio-pci.c       |   4 +-
 hw/virtio/virtio-rng.c       |   7 ++-
 include/hw/qdev-properties.h |  11 ++++
 include/qom/object.h         |  19 ++++++-
 qom/object.c                 | 128 ++++++++++++++++++++++++++++++++-----------
 ui/console.c                 |   4 +-
 13 files changed, 190 insertions(+), 49 deletions(-)

-- 
1.8.5.3

[Qemu-devel] [PATCH 1/4] qom: split object_property_set_link()

[Stefan Hajnoczi]

The path resolution logic in object_property_set_link() should be a
separate function.  This makes the code easier to read and maintain.

More importantly, the error behavior of object_property_set_link() is
dangerous.  It sets the link property object to NULL if an error occurs.
A setter function should either succeed or fail, it shouldn't leave the
value NULL on failure.

This patch splits the code and fixes the error case so the old link
property object is left in place on failure.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qom/object.c | 79 ++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 48 insertions(+), 31 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 660859c..c11ea1e 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1039,48 +1039,65 @@ static void object_get_link_property(Object *obj, Visitor *v, void *opaque,
     }
 }
 
-static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
-                                     const char *name, Error **errp)
+/* object_resolve_link:
+ *
+ * Lookup an object and ensure its type matches the link property type.  This
+ * is similar to object_resolve_path() except type verification against the
+ * link property is performed.
+ *
+ * Returns: The matched object or NULL on path lookup failures.
+ */
+static Object *object_resolve_link(Object *obj, const char *name,
+                                   const char *path, Error **errp)
 {
-    Object **child = opaque;
-    Object *old_target;
-    bool ambiguous = false;
     const char *type;
-    char *path;
     gchar *target_type;
+    bool ambiguous = false;
+    Object *target;
 
+    /* Go from link<FOO> to FOO.  */
     type = object_property_get_type(obj, name, NULL);
-
-    visit_type_str(v, &path, name, errp);
-
-    old_target = *child;
-    *child = NULL;
-
-    if (strcmp(path, "") != 0) {
-        Object *target;
-
-        /* Go from link<FOO> to FOO.  */
-        target_type = g_strndup(&type[5], strlen(type) - 6);
-        target = object_resolve_path_type(path, target_type, &ambiguous);
-
-        if (ambiguous) {
-            error_set(errp, QERR_AMBIGUOUS_PATH, path);
-        } else if (target) {
-            object_ref(target);
-            *child = target;
+    target_type = g_strndup(&type[5], strlen(type) - 6);
+    target = object_resolve_path_type(path, target_type, &ambiguous);
+
+    if (ambiguous) {
+        error_set(errp, QERR_AMBIGUOUS_PATH, path);
+    } else if (!target) {
+        target = object_resolve_path(path, &ambiguous);
+        if (target || ambiguous) {
+            error_set(errp, QERR_INVALID_PARAMETER_TYPE, name, target_type);
         } else {
-            target = object_resolve_path(path, &ambiguous);
-            if (target || ambiguous) {
-                error_set(errp, QERR_INVALID_PARAMETER_TYPE, name, target_type);
-            } else {
-                error_set(errp, QERR_DEVICE_NOT_FOUND, path);
-            }
+            error_set(errp, QERR_DEVICE_NOT_FOUND, path);
         }
-        g_free(target_type);
     }
+    g_free(target_type);
 
+    return target;
+}
+
+static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
+                                     const char *name, Error **errp)
+{
+    Error *local_err = NULL;
+    Object **child = opaque;
+    Object *old_target = *child;
+    Object *new_target = NULL;
+    char *path = NULL;
+
+    visit_type_str(v, &path, name, &local_err);
+    if (!local_err && strcmp(path, "") != 0) {
+        new_target = object_resolve_link(obj, name, path, &local_err);
+    }
     g_free(path);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
 
+    if (new_target) {
+        object_ref(new_target);
+    }
+    *child = new_target;
     if (old_target != NULL) {
         object_unref(old_target);
     }
-- 
1.8.5.3

[Qemu-devel] [PATCH 2/4] qom: make QOM link property unref optional

[Stefan Hajnoczi]

Some object_property_add_link() callers expect property deletion to
unref the link property object.  Other callers expect to manage the
refcount themselves.  The former are currently broken and therefore leak
the link property object.

This patch adds the bool unref_on_release argument to
object_property_add_link() so the caller can specify which refcount
behavior they require.

This fixes refcount leaks in qdev.c, xilinx_axidma.c, xilinx_axienet.c,
s390-virtio-bus.c, virtio-pci.c, virtio-rng.c, and ui/console.c.

Rationale for refcount behavior:

 * hw/core/qdev.c
   - bus children are explicitly unreferenced, don't interfere
   - parent_bus is essentially a read-only property that doesn't hold a
     refcount, don't unref
   - hotplug_handler is leaked, do unref

 * hw/dma/xilinx_axidma.c
   - rx stream "dma" links are set using set_link, therefore they
     need unref
   - tx streams are set using set_link, therefore they need unref

 * hw/net/xilinx_axienet.c
   - same reasoning as hw/dma/xilinx_axidma.c

 * hw/pcmcia/pxa2xx.c
   - pxa2xx bypasses set_link and therefore does not use refcounts

 * hw/s390x/s390-virtio-bus.c
 * hw/virtio/virtio-pci.c
 * hw/virtio/virtio-rng.c
 * ui/console.c
   - set_link is used and there is no explicit unref, do unref

Cc: "Andreas Färber" <afaerber@suse.de>
Cc: Peter Crosthwaite <peter.crosthwaite@petalogix.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: Anthony Liguori <aliguori@amazon.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/core/qdev.c             |  7 +++++--
 hw/dma/xilinx_axidma.c     | 12 ++++++++----
 hw/net/xilinx_axienet.c    | 12 ++++++++----
 hw/pcmcia/pxa2xx.c         |  4 +++-
 hw/s390x/s390-virtio-bus.c |  3 ++-
 hw/s390x/virtio-ccw.c      |  3 ++-
 hw/virtio/virtio-pci.c     |  3 ++-
 hw/virtio/virtio-rng.c     |  3 ++-
 include/qom/object.h       |  7 ++++++-
 qom/object.c               | 39 ++++++++++++++++++++++++++++++++++-----
 ui/console.c               |  3 ++-
 11 files changed, 74 insertions(+), 22 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index c0b857f..273c737 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -98,6 +98,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
     object_property_add_link(OBJECT(bus), name,
                              object_get_typename(OBJECT(child)),
                              (Object **)&kid->child,
+                             false, /* return ownership on prop deletion */
                              NULL);
 }
 
@@ -761,7 +762,8 @@ static void device_initfn(Object *obj)
     } while (class != object_class_by_name(TYPE_DEVICE));
 
     object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
-                             (Object **)&dev->parent_bus, &error_abort);
+                             (Object **)&dev->parent_bus, false,
+                             &error_abort);
 }
 
 static void device_post_init(Object *obj)
@@ -881,7 +883,8 @@ static void qbus_initfn(Object *obj)
     QTAILQ_INIT(&bus->children);
     object_property_add_link(obj, QDEV_HOTPLUG_HANDLER_PROPERTY,
                              TYPE_HOTPLUG_HANDLER,
-                             (Object **)&bus->hotplug_handler, NULL);
+                             (Object **)&bus->hotplug_handler,
+                             true, NULL);
 }
 
 static char *default_bus_get_fw_dev_path(DeviceState *dev)
diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index 19f07b3..cc5826d 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -537,9 +537,11 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
     Error *local_errp = NULL;
 
     object_property_add_link(OBJECT(ds), "dma", TYPE_XILINX_AXI_DMA,
-                             (Object **)&ds->dma, &local_errp);
+                             (Object **)&ds->dma,
+                             true, &local_errp);
     object_property_add_link(OBJECT(cs), "dma", TYPE_XILINX_AXI_DMA,
-                             (Object **)&cs->dma, &local_errp);
+                             (Object **)&cs->dma,
+                             true, &local_errp);
     if (local_errp) {
         goto xilinx_axidma_realize_fail;
     }
@@ -571,10 +573,12 @@ static void xilinx_axidma_init(Object *obj)
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 
     object_property_add_link(obj, "axistream-connected", TYPE_STREAM_SLAVE,
-                             (Object **)&s->tx_data_dev, &error_abort);
+                             (Object **)&s->tx_data_dev,
+                             true, &error_abort);
     object_property_add_link(obj, "axistream-control-connected",
                              TYPE_STREAM_SLAVE,
-                             (Object **)&s->tx_control_dev, &error_abort);
+                             (Object **)&s->tx_control_dev,
+                             true, &error_abort);
 
     object_initialize(&s->rx_data_dev, sizeof(s->rx_data_dev),
                       TYPE_XILINX_AXI_DMA_DATA_STREAM);
diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
index 0bd5eda..385e059 100644
--- a/hw/net/xilinx_axienet.c
+++ b/hw/net/xilinx_axienet.c
@@ -945,9 +945,11 @@ static void xilinx_enet_realize(DeviceState *dev, Error **errp)
     Error *local_errp = NULL;
 
     object_property_add_link(OBJECT(ds), "enet", "xlnx.axi-ethernet",
-                             (Object **) &ds->enet, &local_errp);
+                             (Object **) &ds->enet,
+                             true, &local_errp);
     object_property_add_link(OBJECT(cs), "enet", "xlnx.axi-ethernet",
-                             (Object **) &cs->enet, &local_errp);
+                             (Object **) &cs->enet,
+                             true, &local_errp);
     if (local_errp) {
         goto xilinx_enet_realize_fail;
     }
@@ -982,10 +984,12 @@ static void xilinx_enet_init(Object *obj)
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 
     object_property_add_link(obj, "axistream-connected", TYPE_STREAM_SLAVE,
-                             (Object **) &s->tx_data_dev, &error_abort);
+                             (Object **) &s->tx_data_dev,
+                             true, &error_abort);
     object_property_add_link(obj, "axistream-control-connected",
                              TYPE_STREAM_SLAVE,
-                             (Object **) &s->tx_control_dev, &error_abort);
+                             (Object **) &s->tx_control_dev,
+                             true, &error_abort);
 
     object_initialize(&s->rx_data_dev, sizeof(s->rx_data_dev),
                       TYPE_XILINX_AXI_ENET_DATA_STREAM);
diff --git a/hw/pcmcia/pxa2xx.c b/hw/pcmcia/pxa2xx.c
index 8f17596..f55a806 100644
--- a/hw/pcmcia/pxa2xx.c
+++ b/hw/pcmcia/pxa2xx.c
@@ -198,7 +198,9 @@ static void pxa2xx_pcmcia_initfn(Object *obj)
     s->slot.irq = qemu_allocate_irqs(pxa2xx_pcmcia_set_irq, s, 1)[0];
 
     object_property_add_link(obj, "card", TYPE_PCMCIA_CARD,
-                             (Object **)&s->card, NULL);
+                             (Object **)&s->card,
+                             false, /* don't unref on prop deletion */
+                             NULL);
 }
 
 /* Insert a new card into a slot */
diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index e4fc353..0c9ff92 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -313,7 +313,8 @@ static void s390_virtio_rng_instance_init(Object *obj)
     object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_RNG);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
     object_property_add_link(obj, "rng", TYPE_RNG_BACKEND,
-                             (Object **)&dev->vdev.conf.rng, NULL);
+                             (Object **)&dev->vdev.conf.rng,
+                             true, NULL);
 }
 
 static uint64_t s390_virtio_device_vq_token(VirtIOS390Device *dev, int vq)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index f6e0e3e..2fe782b 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1184,7 +1184,8 @@ static void virtio_ccw_rng_instance_init(Object *obj)
     object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_RNG);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
     object_property_add_link(obj, "rng", TYPE_RNG_BACKEND,
-                             (Object **)&dev->vdev.conf.rng, NULL);
+                             (Object **)&dev->vdev.conf.rng,
+                             true, NULL);
 }
 
 static Property virtio_ccw_rng_properties[] = {
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 7b91841..e91de26 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1517,7 +1517,8 @@ static void virtio_rng_initfn(Object *obj)
     object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_RNG);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
     object_property_add_link(obj, "rng", TYPE_RNG_BACKEND,
-                             (Object **)&dev->vdev.conf.rng, NULL);
+                             (Object **)&dev->vdev.conf.rng,
+                             true, NULL);
 
 }
 
diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index a16e3bc..54ae848 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -223,7 +223,8 @@ static void virtio_rng_initfn(Object *obj)
     VirtIORNG *vrng = VIRTIO_RNG(obj);
 
     object_property_add_link(obj, "rng", TYPE_RNG_BACKEND,
-                             (Object **)&vrng->conf.rng, NULL);
+                             (Object **)&vrng->conf.rng,
+                             true, NULL);
 }
 
 static const TypeInfo virtio_rng_info = {
diff --git a/include/qom/object.h b/include/qom/object.h
index 9c7c361..276d02e 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1065,6 +1065,8 @@ void object_property_add_child(Object *obj, const char *name,
  * @name: the name of the property
  * @type: the qobj type of the link
  * @child: a pointer to where the link object reference is stored
+ * @unref_on_release: whether to release the link object reference when
+ *                    the property is deleted
  * @errp: if an error occurs, a pointer to an area to store the area
  *
  * Links establish relationships between objects.  Links are unidirectional
@@ -1076,10 +1078,13 @@ void object_property_add_child(Object *obj, const char *name,
  * Ownership of the pointer that @child points to is transferred to the
  * link property.  The reference count for <code>*@child</code> is
  * managed by the property from after the function returns till the
- * property is deleted with object_property_del().
+ * property is deleted with object_property_del().  If
+ * <code>@unref_on_release</code> is true, the reference count is
+ * decremented when the property is deleted.
  */
 void object_property_add_link(Object *obj, const char *name,
                               const char *type, Object **child,
+                              bool unref_on_release,
                               Error **errp);
 
 /**
diff --git a/qom/object.c b/qom/object.c
index c11ea1e..89c5358 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1023,10 +1023,16 @@ out:
     g_free(type);
 }
 
+typedef struct {
+    Object **child;
+    bool unref_on_release;
+} LinkProperty;
+
 static void object_get_link_property(Object *obj, Visitor *v, void *opaque,
                                      const char *name, Error **errp)
 {
-    Object **child = opaque;
+    LinkProperty *lprop = opaque;
+    Object **child = lprop->child;
     gchar *path;
 
     if (*child) {
@@ -1079,8 +1085,8 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
                                      const char *name, Error **errp)
 {
     Error *local_err = NULL;
-    Object **child = opaque;
-    Object *old_target = *child;
+    LinkProperty *prop = opaque;
+    Object *old_target = *prop->child;
     Object *new_target = NULL;
     char *path = NULL;
 
@@ -1097,24 +1103,47 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
     if (new_target) {
         object_ref(new_target);
     }
-    *child = new_target;
+    *prop->child = new_target;
     if (old_target != NULL) {
         object_unref(old_target);
     }
 }
 
+static void object_release_link_property(Object *obj, const char *name,
+                                         void *opaque)
+{
+    LinkProperty *prop = opaque;
+
+    if (prop->unref_on_release && *prop->child) {
+        object_unref(*prop->child);
+    }
+    g_free(prop);
+}
+
 void object_property_add_link(Object *obj, const char *name,
                               const char *type, Object **child,
+                              bool unref_on_release,
                               Error **errp)
 {
+    Error *local_err = NULL;
+    LinkProperty *prop = g_malloc(sizeof(*prop));
     gchar *full_type;
 
+    prop->child = child;
+    prop->unref_on_release = unref_on_release;
+
     full_type = g_strdup_printf("link<%s>", type);
 
     object_property_add(obj, name, full_type,
                         object_get_link_property,
                         object_set_link_property,
-                        NULL, child, errp);
+                        object_release_link_property,
+                        prop,
+                        &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        g_free(prop);
+    }
 
     g_free(full_type);
 }
diff --git a/ui/console.c b/ui/console.c
index 502e160..af2d617 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1178,7 +1178,8 @@ static QemuConsole *new_console(DisplayState *ds, console_type_t console_type)
     obj = object_new(TYPE_QEMU_CONSOLE);
     s = QEMU_CONSOLE(obj);
     object_property_add_link(obj, "device", TYPE_DEVICE,
-                             (Object **)&s->device, &local_err);
+                             (Object **)&s->device,
+                             true, &local_err);
 
     if (!active_console || ((active_console->console_type != GRAPHIC_CONSOLE) &&
         (console_type == GRAPHIC_CONSOLE))) {
-- 
1.8.5.3

[Qemu-devel] [PATCH 3/4] qom: add set() argument to object_property_add_link()

[Stefan Hajnoczi]

There are currently three types of object_property_add_link() callers:

1. The link property may be set at any time.
2. The link property of a DeviceState instance may only be set before
   realize.
3. The link property may never be set, it is read-only.

Something similar can already be achieved with
object_property_add_str()'s set() argument.  Follow its example and add
a set() argument to object_property_add_link().

Also provide default set() functions for case #1 and #2.  Case #3 is
covered by passing a NULL function pointer.

Cc: "Andreas Färber" <afaerber@suse.de>
Cc: Peter Crosthwaite <peter.crosthwaite@petalogix.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: Anthony Liguori <aliguori@amazon.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/core/qdev-properties.c    | 12 ++++++++++++
 hw/core/qdev.c               |  4 +++-
 hw/dma/xilinx_axidma.c       |  4 ++++
 hw/net/xilinx_axienet.c      |  4 ++++
 hw/pcmcia/pxa2xx.c           |  1 +
 hw/s390x/s390-virtio-bus.c   |  1 +
 hw/s390x/virtio-ccw.c        |  1 +
 hw/virtio/virtio-pci.c       |  1 +
 hw/virtio/virtio-rng.c       |  1 +
 include/hw/qdev-properties.h | 11 +++++++++++
 include/qom/object.h         | 12 ++++++++++++
 qom/object.c                 | 18 ++++++++++++++++++
 ui/console.c                 |  1 +
 13 files changed, 70 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 77d0c66..2858f11 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -21,6 +21,18 @@ void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
     }
 }
 
+void qdev_prop_default_set_link(Object *obj, const char *name,
+                                Object *val, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+
+    if (dev->realized) {
+        error_setg(errp, "Attempt to set link property '%s' on device '%s' "
+                   "(type '%s') after it was realized",
+                   name, dev->id, object_get_typename(obj));
+    }
+}
+
 void *qdev_get_prop_ptr(DeviceState *dev, Property *prop)
 {
     void *ptr = dev;
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 273c737..c40fd8b 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -98,6 +98,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
     object_property_add_link(OBJECT(bus), name,
                              object_get_typename(OBJECT(child)),
                              (Object **)&kid->child,
+                             NULL, /* read-only property */
                              false, /* return ownership on prop deletion */
                              NULL);
 }
@@ -762,7 +763,7 @@ static void device_initfn(Object *obj)
     } while (class != object_class_by_name(TYPE_DEVICE));
 
     object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
-                             (Object **)&dev->parent_bus, false,
+                             (Object **)&dev->parent_bus, NULL, false,
                              &error_abort);
 }
 
@@ -884,6 +885,7 @@ static void qbus_initfn(Object *obj)
     object_property_add_link(obj, QDEV_HOTPLUG_HANDLER_PROPERTY,
                              TYPE_HOTPLUG_HANDLER,
                              (Object **)&bus->hotplug_handler,
+                             object_property_default_set_link,
                              true, NULL);
 }
 
diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index cc5826d..a0da529 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -538,9 +538,11 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
 
     object_property_add_link(OBJECT(ds), "dma", TYPE_XILINX_AXI_DMA,
                              (Object **)&ds->dma,
+                             object_property_default_set_link,
                              true, &local_errp);
     object_property_add_link(OBJECT(cs), "dma", TYPE_XILINX_AXI_DMA,
                              (Object **)&cs->dma,
+                             object_property_default_set_link,
                              true, &local_errp);
     if (local_errp) {
         goto xilinx_axidma_realize_fail;
@@ -574,10 +576,12 @@ static void xilinx_axidma_init(Object *obj)
 
     object_property_add_link(obj, "axistream-connected", TYPE_STREAM_SLAVE,
                              (Object **)&s->tx_data_dev,
+                             qdev_prop_default_set_link,
                              true, &error_abort);
     object_property_add_link(obj, "axistream-control-connected",
                              TYPE_STREAM_SLAVE,
                              (Object **)&s->tx_control_dev,
+                             qdev_prop_default_set_link,
                              true, &error_abort);
 
     object_initialize(&s->rx_data_dev, sizeof(s->rx_data_dev),
diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
index 385e059..4e139b7 100644
--- a/hw/net/xilinx_axienet.c
+++ b/hw/net/xilinx_axienet.c
@@ -946,9 +946,11 @@ static void xilinx_enet_realize(DeviceState *dev, Error **errp)
 
     object_property_add_link(OBJECT(ds), "enet", "xlnx.axi-ethernet",
                              (Object **) &ds->enet,
+                             object_property_default_set_link,
                              true, &local_errp);
     object_property_add_link(OBJECT(cs), "enet", "xlnx.axi-ethernet",
                              (Object **) &cs->enet,
+                             object_property_default_set_link,
                              true, &local_errp);
     if (local_errp) {
         goto xilinx_enet_realize_fail;
@@ -985,10 +987,12 @@ static void xilinx_enet_init(Object *obj)
 
     object_property_add_link(obj, "axistream-connected", TYPE_STREAM_SLAVE,
                              (Object **) &s->tx_data_dev,
+                             qdev_prop_default_set_link,
                              true, &error_abort);
     object_property_add_link(obj, "axistream-control-connected",
                              TYPE_STREAM_SLAVE,
                              (Object **) &s->tx_control_dev,
+                             qdev_prop_default_set_link,
                              true, &error_abort);
 
     object_initialize(&s->rx_data_dev, sizeof(s->rx_data_dev),
diff --git a/hw/pcmcia/pxa2xx.c b/hw/pcmcia/pxa2xx.c
index f55a806..ee113e1 100644
--- a/hw/pcmcia/pxa2xx.c
+++ b/hw/pcmcia/pxa2xx.c
@@ -199,6 +199,7 @@ static void pxa2xx_pcmcia_initfn(Object *obj)
 
     object_property_add_link(obj, "card", TYPE_PCMCIA_CARD,
                              (Object **)&s->card,
+                             NULL, /* read-only property */
                              false, /* don't unref on prop deletion */
                              NULL);
 }
diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index 0c9ff92..027558b 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -314,6 +314,7 @@ static void s390_virtio_rng_instance_init(Object *obj)
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
     object_property_add_link(obj, "rng", TYPE_RNG_BACKEND,
                              (Object **)&dev->vdev.conf.rng,
+                             qdev_prop_default_set_link,
                              true, NULL);
 }
 
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 2fe782b..53fa86e 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1185,6 +1185,7 @@ static void virtio_ccw_rng_instance_init(Object *obj)
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
     object_property_add_link(obj, "rng", TYPE_RNG_BACKEND,
                              (Object **)&dev->vdev.conf.rng,
+                             qdev_prop_default_set_link,
                              true, NULL);
 }
 
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index e91de26..2b3c93f 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1518,6 +1518,7 @@ static void virtio_rng_initfn(Object *obj)
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
     object_property_add_link(obj, "rng", TYPE_RNG_BACKEND,
                              (Object **)&dev->vdev.conf.rng,
+                             qdev_prop_default_set_link,
                              true, NULL);
 
 }
diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index 54ae848..8efe0aa 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -224,6 +224,7 @@ static void virtio_rng_initfn(Object *obj)
 
     object_property_add_link(obj, "rng", TYPE_RNG_BACKEND,
                              (Object **)&vrng->conf.rng,
+                             qdev_prop_default_set_link,
                              true, NULL);
 }
 
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 0c0babf..da35cb3 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -201,4 +201,15 @@ void qdev_property_add_static(DeviceState *dev, Property *prop, Error **errp);
  */
 void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
                                  Error **errp);
+
+/**
+ * @qdev_prop_default_set_link:
+ *
+ * Set the Error object if an attempt is made to set the link after realize.
+ * This function should be used as the set() argument to
+ * object_property_add_link().
+ */
+void qdev_prop_default_set_link(Object *obj, const char *name,
+                                Object *val, Error **errp);
+
 #endif
diff --git a/include/qom/object.h b/include/qom/object.h
index 276d02e..fc25888 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1060,11 +1060,21 @@ void object_property_add_child(Object *obj, const char *name,
                                Object *child, Error **errp);
 
 /**
+ * object_property_default_set_link:
+ *
+ * The default implementation of the object_property_add_link() set callback
+ * function.  It allows the link property to be set and never returns an error.
+ */
+void object_property_default_set_link(Object *, const char *,
+                                      Object *, Error **);
+
+/**
  * object_property_add_link:
  * @obj: the object to add a property to
  * @name: the name of the property
  * @type: the qobj type of the link
  * @child: a pointer to where the link object reference is stored
+ * @set: the setter or NULL if the property is read-only
  * @unref_on_release: whether to release the link object reference when
  *                    the property is deleted
  * @errp: if an error occurs, a pointer to an area to store the area
@@ -1084,6 +1094,8 @@ void object_property_add_child(Object *obj, const char *name,
  */
 void object_property_add_link(Object *obj, const char *name,
                               const char *type, Object **child,
+                              void (*set)(Object *obj, const char *name,
+                                          Object *val, Error **errp),
                               bool unref_on_release,
                               Error **errp);
 
diff --git a/qom/object.c b/qom/object.c
index 89c5358..c430286 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1023,9 +1023,16 @@ out:
     g_free(type);
 }
 
+void object_property_default_set_link(Object *obj, const char *name,
+                                      Object *val, Error **errp)
+{
+    /* Allow the link to be set, always */
+}
+
 typedef struct {
     Object **child;
     bool unref_on_release;
+    void (*set)(Object *, const char *, Object *, Error **);
 } LinkProperty;
 
 static void object_get_link_property(Object *obj, Visitor *v, void *opaque,
@@ -1100,6 +1107,14 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
         return;
     }
 
+    if (prop->set) {
+        prop->set(obj, name, new_target, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
+
     if (new_target) {
         object_ref(new_target);
     }
@@ -1122,6 +1137,8 @@ static void object_release_link_property(Object *obj, const char *name,
 
 void object_property_add_link(Object *obj, const char *name,
                               const char *type, Object **child,
+                              void (*set)(Object *, const char *,
+                                          Object *, Error **),
                               bool unref_on_release,
                               Error **errp)
 {
@@ -1130,6 +1147,7 @@ void object_property_add_link(Object *obj, const char *name,
     gchar *full_type;
 
     prop->child = child;
+    prop->set = set;
     prop->unref_on_release = unref_on_release;
 
     full_type = g_strdup_printf("link<%s>", type);
diff --git a/ui/console.c b/ui/console.c
index af2d617..6220654 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1179,6 +1179,7 @@ static QemuConsole *new_console(DisplayState *ds, console_type_t console_type)
     s = QEMU_CONSOLE(obj);
     object_property_add_link(obj, "device", TYPE_DEVICE,
                              (Object **)&s->device,
+                             object_property_default_set_link,
                              true, &local_err);
 
     if (!active_console || ((active_console->console_type != GRAPHIC_CONSOLE) &&
-- 
1.8.5.3

[Qemu-devel] [PATCH 4/4] virtio-rng: avoid default_backend refcount leak

[Stefan Hajnoczi]

QOM child properties take a reference to the object and release it when
the property is deleted.  Therefore we should unref the default_backend
after we have added it as a child property.

Cc: KONRAD Frederic <fred.konrad@greensocs.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio-rng.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index 8efe0aa..ddba756 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -162,6 +162,9 @@ static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
                                   OBJECT(vrng->conf.default_backend),
                                   NULL);
 
+        /* The child property took a reference, we can safely drop ours now */
+        object_unref(OBJECT(vrng->conf.default_backend));
+
         object_property_set_link(OBJECT(dev),
                                  OBJECT(vrng->conf.default_backend),
                                  "rng", NULL);
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH 0/4] qom: link property fixes

[Paolo Bonzini]

Il 04/03/2014 22:45, Stefan Hajnoczi ha scritto:
> There are two problems with QOM link properties:
>
> 1. There are refcount leaks in many object_property_add_link() callers.
> 2. There is no way to prevent link properties from being set after a device is
>    realized.
>
> This patch series fixes these issues by extending the
> object_property_add_link() function.

Thanks for looking into this!  Just two (mostly) cosmetic comments:

1) in patch 2 we could use a flags argument instead of a "bool".  This 
is more easily extensible and self-documenting.

2) in patch 3 what you're adding is not exactly a setter; it is more 
like a check.  I think we should either switch to a get/set model (more 
consistent with other properties, but more boilerplate too), or rename 
it to something else than "check".

Paolo

> Stefan Hajnoczi (4):
>   qom: split object_property_set_link()
>   qom: make QOM link property unref optional
>   qom: add set() argument to object_property_add_link()
>   virtio-rng: avoid default_backend refcount leak
>
>  hw/core/qdev-properties.c    |  12 ++++
>  hw/core/qdev.c               |   9 ++-
>  hw/dma/xilinx_axidma.c       |  16 ++++--
>  hw/net/xilinx_axienet.c      |  16 ++++--
>  hw/pcmcia/pxa2xx.c           |   5 +-
>  hw/s390x/s390-virtio-bus.c   |   4 +-
>  hw/s390x/virtio-ccw.c        |   4 +-
>  hw/virtio/virtio-pci.c       |   4 +-
>  hw/virtio/virtio-rng.c       |   7 ++-
>  include/hw/qdev-properties.h |  11 ++++
>  include/qom/object.h         |  19 ++++++-
>  qom/object.c                 | 128 ++++++++++++++++++++++++++++++++-----------
>  ui/console.c                 |   4 +-
>  13 files changed, 190 insertions(+), 49 deletions(-)
>

Re: [Qemu-devel] [PATCH 0/4] qom: link property fixes

[Andreas Färber]

Am 05.03.2014 09:14, schrieb Paolo Bonzini:
> Il 04/03/2014 22:45, Stefan Hajnoczi ha scritto:
>> There are two problems with QOM link properties:
>>
>> 1. There are refcount leaks in many object_property_add_link() callers.
>> 2. There is no way to prevent link properties from being set after a
>> device is
>>    realized.
>>
>> This patch series fixes these issues by extending the
>> object_property_add_link() function.
> 
> Thanks for looking into this!  Just two (mostly) cosmetic comments:
> 
> 1) in patch 2 we could use a flags argument instead of a "bool".  This
> is more easily extensible and self-documenting.

I wonder if we can do this more cleverly: The two ways a link<> property
can get set that I can think of are either a) assigning the field a new
value in C code or b) using object_property_set_link() with a textual
path. Can't the latter simply save an unref flag within the property's
opaque when used?

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 1/4] qom: split object_property_set_link()

[Andreas Färber]

Am 04.03.2014 22:45, schrieb Stefan Hajnoczi:
> The path resolution logic in object_property_set_link() should be a
> separate function.  This makes the code easier to read and maintain.
> 
> More importantly, the error behavior of object_property_set_link() is
> dangerous.  It sets the link property object to NULL if an error occurs.
> A setter function should either succeed or fail, it shouldn't leave the
> value NULL on failure.
> 
> This patch splits the code and fixes the error case so the old link
> property object is left in place on failure.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Since this code movement is a bit invasive due to the reindent (looks OK
though), can we do this in two steps please? First split, then fix?

Regards,
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 0/4] qom: link property fixes

[Paolo Bonzini]

Il 05/03/2014 10:24, Andreas Färber ha scritto:
> Am 05.03.2014 09:14, schrieb Paolo Bonzini:
>> Il 04/03/2014 22:45, Stefan Hajnoczi ha scritto:
>>> There are two problems with QOM link properties:
>>>
>>> 1. There are refcount leaks in many object_property_add_link() callers.
>>> 2. There is no way to prevent link properties from being set after a
>>> device is
>>>    realized.
>>>
>>> This patch series fixes these issues by extending the
>>> object_property_add_link() function.
>>
>> Thanks for looking into this!  Just two (mostly) cosmetic comments:
>>
>> 1) in patch 2 we could use a flags argument instead of a "bool".  This
>> is more easily extensible and self-documenting.
>
> I wonder if we can do this more cleverly: The two ways a link<> property
> can get set that I can think of are either a) assigning the field a new
> value in C code or b) using object_property_set_link() with a textual
> path. Can't the latter simply save an unref flag within the property's
> opaque when used?

Sounds too magic...

Paolo