[Qemu-devel] [PATCH v4 0/5] v4:

[Qemu-devel] [PATCH v4 0/5] v4:

[Stefan Hajnoczi]

v3:
 * Split first commit for easier reviewing [afaerber]
 * Turn bool argument into flags bitmask [pbonzini]
 * Rename set() callback to check() [pbonzini]
 * Rename object_property_default_set_link() to
   object_property_allow_set_link()
 * Rename qdev_prop_default_set_link() to
   qdev_prop_allow_set_link_before_realize()

v2:
 * A NULL set() callback should make the property read-only

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 (5):
  qom: split object_property_set_link()
  qom: don't make link NULL on object_property_set_link() failure
  qom: make QOM link property unref optional
  qom: add check() argument to object_property_add_link()
  virtio-rng: avoid default_backend refcount leak

 hw/core/qdev-properties.c    |  12 ++++
 hw/core/qdev.c               |  10 +++-
 hw/dma/xilinx_axidma.c       |  20 +++++--
 hw/net/xilinx_axienet.c      |  20 +++++--
 hw/pcmcia/pxa2xx.c           |   4 +-
 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         |  29 +++++++++-
 qom/object.c                 | 130 ++++++++++++++++++++++++++++++++-----------
 ui/console.c                 |   5 +-
 13 files changed, 211 insertions(+), 49 deletions(-)

-- 
1.8.5.3

[Qemu-devel] [PATCH v4 1/5] 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.

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

diff --git a/qom/object.c b/qom/object.c
index a2a1ffa..da49474 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1039,17 +1039,49 @@ static void object_get_link_property(Object *obj, Visitor *v, void *opaque,
     }
 }
 
+/* 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)
+{
+    const char *type;
+    gchar *target_type;
+    bool ambiguous = false;
+    Object *target;
+
+    /* Go from link<FOO> to FOO.  */
+    type = object_property_get_type(obj, name, NULL);
+    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 {
+            error_set(errp, QERR_DEVICE_NOT_FOUND, path);
+        }
+        target = NULL;
+    }
+    g_free(target_type);
+
+    return target;
+}
+
 static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
                                      const char *name, Error **errp)
 {
     Object **child = opaque;
     Object *old_target;
-    bool ambiguous = false;
-    const char *type;
     char *path;
-    gchar *target_type;
-
-    type = object_property_get_type(obj, name, NULL);
 
     visit_type_str(v, &path, name, errp);
 
@@ -1059,24 +1091,11 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
     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) {
+        target = object_resolve_link(obj, name, path, errp);
+        if (target) {
             object_ref(target);
             *child = target;
-        } 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);
-            }
         }
-        g_free(target_type);
     }
 
     g_free(path);
-- 
1.8.5.3

[Qemu-devel] [PATCH v4 2/5] qom: don't make link NULL on object_property_set_link() failure

[Stefan Hajnoczi]

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.

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

diff --git a/qom/object.c b/qom/object.c
index da49474..25fc04c 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1079,27 +1079,28 @@ static Object *object_resolve_link(Object *obj, const char *name,
 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;
-    char *path;
-
-    visit_type_str(v, &path, name, errp);
+    Object *old_target = *child;
+    Object *new_target = NULL;
+    char *path = NULL;
 
-    old_target = *child;
-    *child = NULL;
+    visit_type_str(v, &path, name, &local_err);
 
-    if (strcmp(path, "") != 0) {
-        Object *target;
-
-        target = object_resolve_link(obj, name, path, errp);
-        if (target) {
-            object_ref(target);
-            *child = target;
-        }
+    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 v4 3/5] 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 a flags argument to object_property_add_link() so the
caller can specify which refcount behavior they require.  The new
OBJ_PROP_LINK_UNREF_ON_RELEASE flag causes the link pointer to be
unreferenced when the property is deleted.

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             | 10 ++++++----
 hw/dma/xilinx_axidma.c     | 16 ++++++++++++----
 hw/net/xilinx_axienet.c    | 16 ++++++++++++----
 hw/pcmcia/pxa2xx.c         |  2 +-
 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       | 11 ++++++++++-
 qom/object.c               | 36 +++++++++++++++++++++++++++++++++---
 ui/console.c               |  4 +++-
 11 files changed, 85 insertions(+), 22 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 9f0a522..a182917 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -97,8 +97,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
     snprintf(name, sizeof(name), "child[%d]", kid->index);
     object_property_add_link(OBJECT(bus), name,
                              object_get_typename(OBJECT(child)),
-                             (Object **)&kid->child,
-                             NULL);
+                             (Object **)&kid->child, 0, NULL);
 }
 
 void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
@@ -824,7 +823,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, 0,
+                             &error_abort);
 }
 
 static void device_post_init(Object *obj)
@@ -944,7 +944,9 @@ 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,
+                             OBJ_PROP_LINK_UNREF_ON_RELEASE,
+                             NULL);
     object_property_add_bool(obj, "realized",
                              bus_get_realized, bus_set_realized, NULL);
 }
diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index 19f07b3..c8fda39 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -537,9 +537,13 @@ 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,
+                             OBJ_PROP_LINK_UNREF_ON_RELEASE,
+                             &local_errp);
     object_property_add_link(OBJECT(cs), "dma", TYPE_XILINX_AXI_DMA,
-                             (Object **)&cs->dma, &local_errp);
+                             (Object **)&cs->dma,
+                             OBJ_PROP_LINK_UNREF_ON_RELEASE,
+                             &local_errp);
     if (local_errp) {
         goto xilinx_axidma_realize_fail;
     }
@@ -571,10 +575,14 @@ 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,
+                             OBJ_PROP_LINK_UNREF_ON_RELEASE,
+                             &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,
+                             OBJ_PROP_LINK_UNREF_ON_RELEASE,
+                             &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..7ecf925 100644
--- a/hw/net/xilinx_axienet.c
+++ b/hw/net/xilinx_axienet.c
@@ -945,9 +945,13 @@ 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,
+                             OBJ_PROP_LINK_UNREF_ON_RELEASE,
+                             &local_errp);
     object_property_add_link(OBJECT(cs), "enet", "xlnx.axi-ethernet",
-                             (Object **) &cs->enet, &local_errp);
+                             (Object **) &cs->enet,
+                             OBJ_PROP_LINK_UNREF_ON_RELEASE,
+                             &local_errp);
     if (local_errp) {
         goto xilinx_enet_realize_fail;
     }
@@ -982,10 +986,14 @@ 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,
+                             OBJ_PROP_LINK_UNREF_ON_RELEASE,
+                             &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,
+                             OBJ_PROP_LINK_UNREF_ON_RELEASE,
+                             &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..6949214 100644
--- a/hw/pcmcia/pxa2xx.c
+++ b/hw/pcmcia/pxa2xx.c
@@ -198,7 +198,7 @@ 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, 0, 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..930b4f7 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,
+                             OBJ_PROP_LINK_UNREF_ON_RELEASE, 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 a01801e..aebb2de 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1272,7 +1272,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,
+                             OBJ_PROP_LINK_UNREF_ON_RELEASE, NULL);
 }
 
 static Property virtio_ccw_rng_properties[] = {
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 7b91841..eebb819 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,
+                             OBJ_PROP_LINK_UNREF_ON_RELEASE, NULL);
 
 }
 
diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index a16e3bc..2efda8b 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,
+                             OBJ_PROP_LINK_UNREF_ON_RELEASE, NULL);
 }
 
 static const TypeInfo virtio_rng_info = {
diff --git a/include/qom/object.h b/include/qom/object.h
index 4cd7704..9feb441 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1067,12 +1067,18 @@ Object *object_resolve_path_component(Object *parent, const gchar *part);
 void object_property_add_child(Object *obj, const char *name,
                                Object *child, Error **errp);
 
+typedef enum {
+    /* Unref the link pointer when the property is deleted */
+    OBJ_PROP_LINK_UNREF_ON_RELEASE = 0x1,
+} ObjectPropertyLinkFlags;
+
 /**
  * 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
+ * @flags: additional options for the link
  * @errp: if an error occurs, a pointer to an area to store the area
  *
  * Links establish relationships between objects.  Links are unidirectional
@@ -1084,10 +1090,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 the
+ * <code>@flags</code> <code>OBJ_PROP_LINK_UNREF_ON_RELEASE</code> bit is set,
+ * 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,
+                              ObjectPropertyLinkFlags flags,
                               Error **errp);
 
 /**
diff --git a/qom/object.c b/qom/object.c
index 25fc04c..60aea9c 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1023,10 +1023,16 @@ out:
     g_free(type);
 }
 
+typedef struct {
+    Object **child;
+    ObjectPropertyLinkFlags flags;
+} 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) {
@@ -1080,7 +1086,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;
+    LinkProperty *prop = opaque;
+    Object **child = prop->child;
     Object *old_target = *child;
     Object *new_target = NULL;
     char *path = NULL;
@@ -1106,18 +1113,41 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
     }
 }
 
+static void object_release_link_property(Object *obj, const char *name,
+                                         void *opaque)
+{
+    LinkProperty *prop = opaque;
+
+    if ((prop->flags & OBJ_PROP_LINK_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,
+                              ObjectPropertyLinkFlags flags,
                               Error **errp)
 {
+    Error *local_err = NULL;
+    LinkProperty *prop = g_malloc(sizeof(*prop));
     gchar *full_type;
 
+    prop->child = child;
+    prop->flags = flags;
+
     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 4df251d..9974212 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1180,7 +1180,9 @@ 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,
+                             OBJ_PROP_LINK_UNREF_ON_RELEASE,
+                             &local_err);
     object_property_add_uint32_ptr(obj, "head",
                                    &s->head, &local_err);
 
-- 
1.8.5.3

[Qemu-devel] [PATCH v4 4/5] qom: add check() 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 check() argument to object_property_add_link().

Also provide default check() 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               |  8 ++++++--
 hw/dma/xilinx_axidma.c       |  4 ++++
 hw/net/xilinx_axienet.c      |  4 ++++
 hw/pcmcia/pxa2xx.c           |  4 +++-
 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         | 18 ++++++++++++++++++
 qom/object.c                 | 18 +++++++++++++++++-
 ui/console.c                 |  1 +
 13 files changed, 80 insertions(+), 4 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 77d0c66..c67acf5 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_allow_set_link_before_realize(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 a182917..97acf62 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -97,7 +97,10 @@ static void bus_add_child(BusState *bus, DeviceState *child)
     snprintf(name, sizeof(name), "child[%d]", kid->index);
     object_property_add_link(OBJECT(bus), name,
                              object_get_typename(OBJECT(child)),
-                             (Object **)&kid->child, 0, NULL);
+                             (Object **)&kid->child,
+                             NULL, /* read-only property */
+                             0, /* return ownership on prop deletion */
+                             NULL);
 }
 
 void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
@@ -823,7 +826,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, 0,
+                             (Object **)&dev->parent_bus, NULL, 0,
                              &error_abort);
 }
 
@@ -945,6 +948,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_allow_set_link,
                              OBJ_PROP_LINK_UNREF_ON_RELEASE,
                              NULL);
     object_property_add_bool(obj, "realized",
diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index c8fda39..14b887b 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -538,10 +538,12 @@ 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_allow_set_link,
                              OBJ_PROP_LINK_UNREF_ON_RELEASE,
                              &local_errp);
     object_property_add_link(OBJECT(cs), "dma", TYPE_XILINX_AXI_DMA,
                              (Object **)&cs->dma,
+                             object_property_allow_set_link,
                              OBJ_PROP_LINK_UNREF_ON_RELEASE,
                              &local_errp);
     if (local_errp) {
@@ -576,11 +578,13 @@ static void xilinx_axidma_init(Object *obj)
 
     object_property_add_link(obj, "axistream-connected", TYPE_STREAM_SLAVE,
                              (Object **)&s->tx_data_dev,
+                             qdev_prop_allow_set_link_before_realize,
                              OBJ_PROP_LINK_UNREF_ON_RELEASE,
                              &error_abort);
     object_property_add_link(obj, "axistream-control-connected",
                              TYPE_STREAM_SLAVE,
                              (Object **)&s->tx_control_dev,
+                             qdev_prop_allow_set_link_before_realize,
                              OBJ_PROP_LINK_UNREF_ON_RELEASE,
                              &error_abort);
 
diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
index 7ecf925..839d97c 100644
--- a/hw/net/xilinx_axienet.c
+++ b/hw/net/xilinx_axienet.c
@@ -946,10 +946,12 @@ static void xilinx_enet_realize(DeviceState *dev, Error **errp)
 
     object_property_add_link(OBJECT(ds), "enet", "xlnx.axi-ethernet",
                              (Object **) &ds->enet,
+                             object_property_allow_set_link,
                              OBJ_PROP_LINK_UNREF_ON_RELEASE,
                              &local_errp);
     object_property_add_link(OBJECT(cs), "enet", "xlnx.axi-ethernet",
                              (Object **) &cs->enet,
+                             object_property_allow_set_link,
                              OBJ_PROP_LINK_UNREF_ON_RELEASE,
                              &local_errp);
     if (local_errp) {
@@ -987,11 +989,13 @@ static void xilinx_enet_init(Object *obj)
 
     object_property_add_link(obj, "axistream-connected", TYPE_STREAM_SLAVE,
                              (Object **) &s->tx_data_dev,
+                             qdev_prop_allow_set_link_before_realize,
                              OBJ_PROP_LINK_UNREF_ON_RELEASE,
                              &error_abort);
     object_property_add_link(obj, "axistream-control-connected",
                              TYPE_STREAM_SLAVE,
                              (Object **) &s->tx_control_dev,
+                             qdev_prop_allow_set_link_before_realize,
                              OBJ_PROP_LINK_UNREF_ON_RELEASE,
                              &error_abort);
 
diff --git a/hw/pcmcia/pxa2xx.c b/hw/pcmcia/pxa2xx.c
index 6949214..96f3774 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, 0, NULL);
+                             (Object **)&s->card,
+                             NULL, /* read-only property */
+                             0, 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 930b4f7..9c71afa 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_allow_set_link_before_realize,
                              OBJ_PROP_LINK_UNREF_ON_RELEASE, NULL);
 }
 
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index aebb2de..2bf0af8 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1273,6 +1273,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_allow_set_link_before_realize,
                              OBJ_PROP_LINK_UNREF_ON_RELEASE, NULL);
 }
 
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index eebb819..ce97514 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_allow_set_link_before_realize,
                              OBJ_PROP_LINK_UNREF_ON_RELEASE, NULL);
 
 }
diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index 2efda8b..cbf0138 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_allow_set_link_before_realize,
                              OBJ_PROP_LINK_UNREF_ON_RELEASE, NULL);
 }
 
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 3c000ee..68aaed6 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -204,4 +204,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_allow_set_link_before_realize:
+ *
+ * Set the Error object if an attempt is made to set the link after realize.
+ * This function should be used as the check() argument to
+ * object_property_add_link().
+ */
+void qdev_prop_allow_set_link_before_realize(Object *obj, const char *name,
+                                             Object *val, Error **errp);
+
 #endif
diff --git a/include/qom/object.h b/include/qom/object.h
index 9feb441..a641dcd 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1073,11 +1073,22 @@ typedef enum {
 } ObjectPropertyLinkFlags;
 
 /**
+ * object_property_allow_set_link:
+ *
+ * The default implementation of the object_property_add_link() check()
+ * callback function.  It allows the link property to be set and never returns
+ * an error.
+ */
+void object_property_allow_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
+ * @check: callback to veto setting or NULL if the property is read-only
  * @flags: additional options for the link
  * @errp: if an error occurs, a pointer to an area to store the area
  *
@@ -1087,6 +1098,11 @@ typedef enum {
  *
  * Links form the graph in the object model.
  *
+ * The <code>@check()</code> callback is invoked when
+ * object_property_set_link() is called and can raise an error to prevent the
+ * link being set.  If <code>@check</code> is NULL, the property is read-only
+ * and cannot be set.
+ *
  * 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
@@ -1096,6 +1112,8 @@ typedef enum {
  */
 void object_property_add_link(Object *obj, const char *name,
                               const char *type, Object **child,
+                              void (*check)(Object *obj, const char *name,
+                                            Object *val, Error **errp),
                               ObjectPropertyLinkFlags flags,
                               Error **errp);
 
diff --git a/qom/object.c b/qom/object.c
index 60aea9c..c9e8bcd 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1023,8 +1023,15 @@ out:
     g_free(type);
 }
 
+void object_property_allow_set_link(Object *obj, const char *name,
+                                    Object *val, Error **errp)
+{
+    /* Allow the link to be set, always */
+}
+
 typedef struct {
     Object **child;
+    void (*check)(Object *, const char *, Object *, Error **);
     ObjectPropertyLinkFlags flags;
 } LinkProperty;
 
@@ -1104,6 +1111,12 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
         return;
     }
 
+    prop->check(obj, name, new_target, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
     if (new_target) {
         object_ref(new_target);
     }
@@ -1126,6 +1139,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 (*check)(Object *, const char *,
+                                            Object *, Error **),
                               ObjectPropertyLinkFlags flags,
                               Error **errp)
 {
@@ -1134,13 +1149,14 @@ void object_property_add_link(Object *obj, const char *name,
     gchar *full_type;
 
     prop->child = child;
+    prop->check = check;
     prop->flags = flags;
 
     full_type = g_strdup_printf("link<%s>", type);
 
     object_property_add(obj, name, full_type,
                         object_get_link_property,
-                        object_set_link_property,
+                        check ? object_set_link_property : NULL,
                         object_release_link_property,
                         prop,
                         &local_err);
diff --git a/ui/console.c b/ui/console.c
index 9974212..e057755 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1181,6 +1181,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_allow_set_link,
                              OBJ_PROP_LINK_UNREF_ON_RELEASE,
                              &local_err);
     object_property_add_uint32_ptr(obj, "head",
-- 
1.8.5.3

[Qemu-devel] [PATCH v4 5/5] 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 cbf0138..b6ab361 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 v4 0/5] v4:

[Stefan Hajnoczi]

On Tue, Mar 18, 2014 at 01:49:38PM +0100, Stefan Hajnoczi wrote:

This patch submission is broken.  I'll resend.

Stefan