We commonly plug devices into their bus right when we create them,
like this:

    dev = qdev_create(bus, type_name);

Note that @dev is a weak reference.  The reference from @bus to @dev
is the only strong one.

We realize at some later time, either with

    object_property_set_bool(OBJECT(dev), true, "realized", errp);

or its convenience wrapper

    qdev_init_nofail(dev);

If @dev still has no QOM parent then, realizing makes the
/machine/unattached/ orphanage its QOM parent.

Note that the device returned by qdev_create() is plugged into a bus,
but doesn't have a QOM parent, yet.  Until it acquires one,
unrealizing the bus will hang in bus_unparent():

    while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
        DeviceState *dev = kid->child;
        object_unparent(OBJECT(dev));
    }

object_unparent() does nothing when its argument has no QOM parent,
and the loop spins forever.

Device state "no QOM parent, but plugged into bus" is dangerous.

Paolo suggested to delay plugging into the bus until realize.  We need
to plug into the parent bus before we call the device's realize
method, in case it uses the parent bus.  So the dangerous state still
exists, but only within realization, where we can manage it safely.

This commit creates infrastructure to do this:

    dev = qdev_new(type_name);
    ...
    qdev_realize_and_unref(dev, bus, errp)

Note that @dev becomes a strong reference here.
qdev_realize_and_unref() drops it.  There is also plain
qdev_realize(), which doesn't drop it.

The remainder of this series will convert all users to this new
interface.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: Alistair Francis <alistair@alistair23.me>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/hw/qdev-core.h | 11 ++++-
 hw/core/bus.c          | 14 +++++++
 hw/core/qdev.c         | 94 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 118 insertions(+), 1 deletion(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index b870b27966..fba29308f7 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -57,7 +57,7 @@ typedef void (*BusUnrealize)(BusState *bus);
  * After successful realization, setting static properties will fail.
  *
  * As an interim step, the #DeviceState:realized property can also be
- * set with qdev_init_nofail().
+ * set with qdev_realize() or qdev_init_nofail().
  * In the future, devices will propagate this state change to their children
  * and along busses they expose.
  * The point in time will be deferred to machine creation, so that values
@@ -322,7 +322,13 @@ compat_props_add(GPtrArray *arr,
 
 DeviceState *qdev_create(BusState *bus, const char *name);
 DeviceState *qdev_try_create(BusState *bus, const char *name);
+DeviceState *qdev_new(const char *name);
+DeviceState *qdev_try_new(const char *name);
 void qdev_init_nofail(DeviceState *dev);
+bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp);
+bool qdev_realize_and_unref(DeviceState *dev, BusState *bus, Error **errp);
+void qdev_unrealize(DeviceState *dev);
+
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
                                  int required_for_version);
 HotplugHandler *qdev_get_bus_hotplug_handler(DeviceState *dev);
@@ -411,6 +417,9 @@ typedef int (qdev_walkerfn)(DeviceState *dev, void *opaque);
 void qbus_create_inplace(void *bus, size_t size, const char *typename,
                          DeviceState *parent, const char *name);
 BusState *qbus_create(const char *typename, DeviceState *parent, const char *name);
+bool qbus_realize(BusState *bus, Error **errp);
+void qbus_unrealize(BusState *bus);
+
 /* Returns > 0 if either devfn or busfn skip walk somewhere in cursion,
  *         < 0 if either devfn or busfn terminate walk somewhere in cursion,
  *           0 otherwise. */
diff --git a/hw/core/bus.c b/hw/core/bus.c
index 08c5eab24a..bf622604a3 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -169,6 +169,20 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
     return bus;
 }
 
+bool qbus_realize(BusState *bus, Error **errp)
+{
+    Error *err = NULL;
+
+    object_property_set_bool(OBJECT(bus), true, "realized", &err);
+    error_propagate(errp, err);
+    return !err;
+}
+
+void qbus_unrealize(BusState *bus)
+{
+    object_property_set_bool(OBJECT(bus), true, "realized", &error_abort);
+}
+
 static bool bus_get_realized(Object *obj, Error **errp)
 {
     BusState *bus = BUS(obj);
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index a68ba674db..82deeb7841 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -176,6 +176,32 @@ DeviceState *qdev_try_create(BusState *bus, const char *type)
     return dev;
 }
 
+/*
+ * Create a device on the heap.
+ * A type @name must exist.
+ * This only initializes the device state structure and allows
+ * properties to be set.  The device still needs to be realized.  See
+ * qdev-core.h.
+ */
+DeviceState *qdev_new(const char *name)
+{
+    return DEVICE(object_new(name));
+}
+
+/*
+ * Try to create a device on the heap.
+ * This is like qdev_new(), except it returns %NULL when type @name
+ * does not exist.
+ */
+DeviceState *qdev_try_new(const char *name)
+{
+    if (!object_class_by_name(name)) {
+        return NULL;
+    }
+
+    return DEVICE(object_new(name));
+}
+
 static QTAILQ_HEAD(, DeviceListener) device_listeners
     = QTAILQ_HEAD_INITIALIZER(device_listeners);
 
@@ -427,6 +453,70 @@ void qdev_init_nofail(DeviceState *dev)
     object_unref(OBJECT(dev));
 }
 
+/*
+ * Realize @dev.
+ * @dev must not be plugged into a bus.
+ * Plug @dev into @bus if non-null, else into the main system bus.
+ * This takes a reference to @dev.
+ * If @dev has no QOM parent, make one up, taking another reference.
+ * On success, return true.
+ * On failure, store an error through @errp and return false.
+ */
+bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp)
+{
+    Error *err = NULL;
+
+    assert(!dev->realized && !dev->parent_bus);
+
+    if (!bus) {
+        /*
+         * Assert that the device really is a SysBusDevice before we
+         * put it onto the sysbus.  Non-sysbus devices which aren't
+         * being put onto a bus should be realized with
+         * object_property_set_bool(OBJECT(dev), true, "realized",
+         * errp);
+         */
+        g_assert(object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE));
+        bus = sysbus_get_default();
+    }
+
+    qdev_set_parent_bus(dev, bus);
+
+    object_ref(OBJECT(dev));
+    object_property_set_bool(OBJECT(dev), true, "realized", &err);
+    if (err) {
+        error_propagate_prepend(errp, err,
+                                "Initialization of device %s failed: ",
+                                object_get_typename(OBJECT(dev)));
+    }
+    object_unref(OBJECT(dev));
+    return !err;
+}
+
+/*
+ * Realize @dev and drop a reference.
+ * This is like qdev_realize(), except it steals a reference rather
+ * than take one to plug @dev into @bus.  On failure, it drops that
+ * reference instead.  Intended use:
+ *     dev = qdev_new();
+ *     [...]
+ *     qdev_realize_and_unref(dev, bus, errp);
+ * Now @dev can go away without further ado.
+ */
+bool qdev_realize_and_unref(DeviceState *dev, BusState *bus, Error **errp)
+{
+    bool ret;
+
+    ret = qdev_realize(dev, bus, errp);
+    object_unref(OBJECT(dev));
+    return ret;
+}
+
+void qdev_unrealize(DeviceState *dev)
+{
+    object_property_set_bool(OBJECT(dev), false, "realized", &error_abort);
+}
+
 static int qdev_assert_realized_properly(Object *obj, void *opaque)
 {
     DeviceState *dev = DEVICE(object_dynamic_cast(obj, TYPE_DEVICE));
@@ -1002,6 +1092,10 @@ post_realize_fail:
 fail:
     error_propagate(errp, local_err);
     if (unattached_parent) {
+        /*
+         * Beware, this doesn't just revert
+         * object_property_add_child(), it also runs bus_remove()!
+         */
         object_unparent(OBJECT(dev));
         unattached_count--;
     }
-- 
2.21.1

On Tue, May 19, 2020 at 8:11 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> We commonly plug devices into their bus right when we create them,
> like this:
>
>     dev = qdev_create(bus, type_name);
>
> Note that @dev is a weak reference.  The reference from @bus to @dev
> is the only strong one.
>
> We realize at some later time, either with
>
>     object_property_set_bool(OBJECT(dev), true, "realized", errp);
>
> or its convenience wrapper
>
>     qdev_init_nofail(dev);
>
> If @dev still has no QOM parent then, realizing makes the
> /machine/unattached/ orphanage its QOM parent.
>
> Note that the device returned by qdev_create() is plugged into a bus,
> but doesn't have a QOM parent, yet.  Until it acquires one,
> unrealizing the bus will hang in bus_unparent():
>
>     while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
>         DeviceState *dev = kid->child;
>         object_unparent(OBJECT(dev));
>     }
>
> object_unparent() does nothing when its argument has no QOM parent,
> and the loop spins forever.
>
> Device state "no QOM parent, but plugged into bus" is dangerous.
>
> Paolo suggested to delay plugging into the bus until realize.  We need
> to plug into the parent bus before we call the device's realize
> method, in case it uses the parent bus.  So the dangerous state still
> exists, but only within realization, where we can manage it safely.
>
> This commit creates infrastructure to do this:
>
>     dev = qdev_new(type_name);
>     ...
>     qdev_realize_and_unref(dev, bus, errp)
>
> Note that @dev becomes a strong reference here.
> qdev_realize_and_unref() drops it.  There is also plain
> qdev_realize(), which doesn't drop it.
>
> The remainder of this series will convert all users to this new
> interface.
>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: Alistair Francis <alistair@alistair23.me>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/hw/qdev-core.h | 11 ++++-
>  hw/core/bus.c          | 14 +++++++
>  hw/core/qdev.c         | 94 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 118 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index b870b27966..fba29308f7 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -57,7 +57,7 @@ typedef void (*BusUnrealize)(BusState *bus);
>   * After successful realization, setting static properties will fail.
>   *
>   * As an interim step, the #DeviceState:realized property can also be
> - * set with qdev_init_nofail().
> + * set with qdev_realize() or qdev_init_nofail().
>   * In the future, devices will propagate this state change to their children
>   * and along busses they expose.
>   * The point in time will be deferred to machine creation, so that values
> @@ -322,7 +322,13 @@ compat_props_add(GPtrArray *arr,
>
>  DeviceState *qdev_create(BusState *bus, const char *name);
>  DeviceState *qdev_try_create(BusState *bus, const char *name);
> +DeviceState *qdev_new(const char *name);
> +DeviceState *qdev_try_new(const char *name);
>  void qdev_init_nofail(DeviceState *dev);
> +bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp);
> +bool qdev_realize_and_unref(DeviceState *dev, BusState *bus, Error **errp);
> +void qdev_unrealize(DeviceState *dev);
> +
>  void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>                                   int required_for_version);
>  HotplugHandler *qdev_get_bus_hotplug_handler(DeviceState *dev);
> @@ -411,6 +417,9 @@ typedef int (qdev_walkerfn)(DeviceState *dev, void *opaque);
>  void qbus_create_inplace(void *bus, size_t size, const char *typename,
>                           DeviceState *parent, const char *name);
>  BusState *qbus_create(const char *typename, DeviceState *parent, const char *name);
> +bool qbus_realize(BusState *bus, Error **errp);
> +void qbus_unrealize(BusState *bus);
> +
>  /* Returns > 0 if either devfn or busfn skip walk somewhere in cursion,
>   *         < 0 if either devfn or busfn terminate walk somewhere in cursion,
>   *           0 otherwise. */
> diff --git a/hw/core/bus.c b/hw/core/bus.c
> index 08c5eab24a..bf622604a3 100644
> --- a/hw/core/bus.c
> +++ b/hw/core/bus.c
> @@ -169,6 +169,20 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
>      return bus;
>  }
>
> +bool qbus_realize(BusState *bus, Error **errp)
> +{
> +    Error *err = NULL;
> +
> +    object_property_set_bool(OBJECT(bus), true, "realized", &err);
> +    error_propagate(errp, err);
> +    return !err;
> +}
> +
> +void qbus_unrealize(BusState *bus)
> +{
> +    object_property_set_bool(OBJECT(bus), true, "realized", &error_abort);

Not false?

Alistair

> +}
> +
>  static bool bus_get_realized(Object *obj, Error **errp)
>  {
>      BusState *bus = BUS(obj);
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index a68ba674db..82deeb7841 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -176,6 +176,32 @@ DeviceState *qdev_try_create(BusState *bus, const char *type)
>      return dev;
>  }
>
> +/*
> + * Create a device on the heap.
> + * A type @name must exist.
> + * This only initializes the device state structure and allows
> + * properties to be set.  The device still needs to be realized.  See
> + * qdev-core.h.
> + */
> +DeviceState *qdev_new(const char *name)
> +{
> +    return DEVICE(object_new(name));
> +}
> +
> +/*
> + * Try to create a device on the heap.
> + * This is like qdev_new(), except it returns %NULL when type @name
> + * does not exist.
> + */
> +DeviceState *qdev_try_new(const char *name)
> +{
> +    if (!object_class_by_name(name)) {
> +        return NULL;
> +    }
> +
> +    return DEVICE(object_new(name));
> +}
> +
>  static QTAILQ_HEAD(, DeviceListener) device_listeners
>      = QTAILQ_HEAD_INITIALIZER(device_listeners);
>
> @@ -427,6 +453,70 @@ void qdev_init_nofail(DeviceState *dev)
>      object_unref(OBJECT(dev));
>  }
>
> +/*
> + * Realize @dev.
> + * @dev must not be plugged into a bus.
> + * Plug @dev into @bus if non-null, else into the main system bus.
> + * This takes a reference to @dev.
> + * If @dev has no QOM parent, make one up, taking another reference.
> + * On success, return true.
> + * On failure, store an error through @errp and return false.
> + */
> +bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp)
> +{
> +    Error *err = NULL;
> +
> +    assert(!dev->realized && !dev->parent_bus);
> +
> +    if (!bus) {
> +        /*
> +         * Assert that the device really is a SysBusDevice before we
> +         * put it onto the sysbus.  Non-sysbus devices which aren't
> +         * being put onto a bus should be realized with
> +         * object_property_set_bool(OBJECT(dev), true, "realized",
> +         * errp);
> +         */
> +        g_assert(object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE));
> +        bus = sysbus_get_default();
> +    }
> +
> +    qdev_set_parent_bus(dev, bus);
> +
> +    object_ref(OBJECT(dev));
> +    object_property_set_bool(OBJECT(dev), true, "realized", &err);
> +    if (err) {
> +        error_propagate_prepend(errp, err,
> +                                "Initialization of device %s failed: ",
> +                                object_get_typename(OBJECT(dev)));
> +    }
> +    object_unref(OBJECT(dev));
> +    return !err;
> +}
> +
> +/*
> + * Realize @dev and drop a reference.
> + * This is like qdev_realize(), except it steals a reference rather
> + * than take one to plug @dev into @bus.  On failure, it drops that
> + * reference instead.  Intended use:
> + *     dev = qdev_new();
> + *     [...]
> + *     qdev_realize_and_unref(dev, bus, errp);
> + * Now @dev can go away without further ado.
> + */
> +bool qdev_realize_and_unref(DeviceState *dev, BusState *bus, Error **errp)
> +{
> +    bool ret;
> +
> +    ret = qdev_realize(dev, bus, errp);
> +    object_unref(OBJECT(dev));
> +    return ret;
> +}
> +
> +void qdev_unrealize(DeviceState *dev)
> +{
> +    object_property_set_bool(OBJECT(dev), false, "realized", &error_abort);
> +}
> +
>  static int qdev_assert_realized_properly(Object *obj, void *opaque)
>  {
>      DeviceState *dev = DEVICE(object_dynamic_cast(obj, TYPE_DEVICE));
> @@ -1002,6 +1092,10 @@ post_realize_fail:
>  fail:
>      error_propagate(errp, local_err);
>      if (unattached_parent) {
> +        /*
> +         * Beware, this doesn't just revert
> +         * object_property_add_child(), it also runs bus_remove()!
> +         */
>          object_unparent(OBJECT(dev));
>          unattached_count--;
>      }
> --
> 2.21.1
>
>

Alistair Francis <alistair23@gmail.com> writes:

> On Tue, May 19, 2020 at 8:11 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> We commonly plug devices into their bus right when we create them,
>> like this:
>>
>>     dev = qdev_create(bus, type_name);
>>
>> Note that @dev is a weak reference.  The reference from @bus to @dev
>> is the only strong one.
>>
>> We realize at some later time, either with
>>
>>     object_property_set_bool(OBJECT(dev), true, "realized", errp);
>>
>> or its convenience wrapper
>>
>>     qdev_init_nofail(dev);
>>
>> If @dev still has no QOM parent then, realizing makes the
>> /machine/unattached/ orphanage its QOM parent.
>>
>> Note that the device returned by qdev_create() is plugged into a bus,
>> but doesn't have a QOM parent, yet.  Until it acquires one,
>> unrealizing the bus will hang in bus_unparent():
>>
>>     while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
>>         DeviceState *dev = kid->child;
>>         object_unparent(OBJECT(dev));
>>     }
>>
>> object_unparent() does nothing when its argument has no QOM parent,
>> and the loop spins forever.
>>
>> Device state "no QOM parent, but plugged into bus" is dangerous.
>>
>> Paolo suggested to delay plugging into the bus until realize.  We need
>> to plug into the parent bus before we call the device's realize
>> method, in case it uses the parent bus.  So the dangerous state still
>> exists, but only within realization, where we can manage it safely.
>>
>> This commit creates infrastructure to do this:
>>
>>     dev = qdev_new(type_name);
>>     ...
>>     qdev_realize_and_unref(dev, bus, errp)
>>
>> Note that @dev becomes a strong reference here.
>> qdev_realize_and_unref() drops it.  There is also plain
>> qdev_realize(), which doesn't drop it.
>>
>> The remainder of this series will convert all users to this new
>> interface.
>>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>> Cc: Alistair Francis <alistair@alistair23.me>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> Cc: David Gibson <david@gibson.dropbear.id.au>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  include/hw/qdev-core.h | 11 ++++-
>>  hw/core/bus.c          | 14 +++++++
>>  hw/core/qdev.c         | 94 ++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 118 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index b870b27966..fba29308f7 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -57,7 +57,7 @@ typedef void (*BusUnrealize)(BusState *bus);
>>   * After successful realization, setting static properties will fail.
>>   *
>>   * As an interim step, the #DeviceState:realized property can also be
>> - * set with qdev_init_nofail().
>> + * set with qdev_realize() or qdev_init_nofail().
>>   * In the future, devices will propagate this state change to their children
>>   * and along busses they expose.
>>   * The point in time will be deferred to machine creation, so that values
>> @@ -322,7 +322,13 @@ compat_props_add(GPtrArray *arr,
>>
>>  DeviceState *qdev_create(BusState *bus, const char *name);
>>  DeviceState *qdev_try_create(BusState *bus, const char *name);
>> +DeviceState *qdev_new(const char *name);
>> +DeviceState *qdev_try_new(const char *name);
>>  void qdev_init_nofail(DeviceState *dev);
>> +bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp);
>> +bool qdev_realize_and_unref(DeviceState *dev, BusState *bus, Error **errp);
>> +void qdev_unrealize(DeviceState *dev);
>> +
>>  void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>>                                   int required_for_version);
>>  HotplugHandler *qdev_get_bus_hotplug_handler(DeviceState *dev);
>> @@ -411,6 +417,9 @@ typedef int (qdev_walkerfn)(DeviceState *dev, void *opaque);
>>  void qbus_create_inplace(void *bus, size_t size, const char *typename,
>>                           DeviceState *parent, const char *name);
>>  BusState *qbus_create(const char *typename, DeviceState *parent, const char *name);
>> +bool qbus_realize(BusState *bus, Error **errp);
>> +void qbus_unrealize(BusState *bus);
>> +
>>  /* Returns > 0 if either devfn or busfn skip walk somewhere in cursion,
>>   *         < 0 if either devfn or busfn terminate walk somewhere in cursion,
>>   *           0 otherwise. */
>> diff --git a/hw/core/bus.c b/hw/core/bus.c
>> index 08c5eab24a..bf622604a3 100644
>> --- a/hw/core/bus.c
>> +++ b/hw/core/bus.c
>> @@ -169,6 +169,20 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
>>      return bus;
>>  }
>>
>> +bool qbus_realize(BusState *bus, Error **errp)
>> +{
>> +    Error *err = NULL;
>> +
>> +    object_property_set_bool(OBJECT(bus), true, "realized", &err);
>> +    error_propagate(errp, err);
>> +    return !err;
>> +}
>> +
>> +void qbus_unrealize(BusState *bus)
>> +{
>> +    object_property_set_bool(OBJECT(bus), true, "realized", &error_abort);
>
> Not false?
>
> Alistair

Reasons it's &error_abort:

1. PATCH 06 and 07 transform variations of

      object_property_set_bool(..., false, "realized", &error_abort);

   to

      qdev_unrealize(...);

   No untransformed unrealization remain.  Thus, we always abort on
   unrealization error before this series.

2. If unrealize could fail, we'd be in deep trouble.  Recent commit
   b69c3c21a5 "qdev: Unrealize must not fail" explains:

   Devices may have component devices and buses.

   Device realization may fail.  Realization is recursive: a device's
   realize() method realizes its components, and device_set_realized()
   realizes its buses (which should in turn realize the devices on that
   bus, except bus_set_realized() doesn't implement that, yet).

   When realization of a component or bus fails, we need to roll back:
   unrealize everything we realized so far.  If any of these unrealizes
   failed, the device would be left in an inconsistent state.  Must not
   happen.

   device_set_realized() lets it happen: it ignores errors in the roll
   back code starting at label child_realize_fail.

   Since realization is recursive, unrealization must be recursive, too.
   But how could a partly failed unrealize be rolled back?  We'd have to
   re-realize, which can fail.  This design is fundamentally broken.

   device_set_realized() does not roll back at all.  Instead, it keeps
   unrealizing, ignoring further errors.

   It can screw up even for a device with no buses: if the lone
   dc->unrealize() fails, it still unregisters vmstate, and calls
   listeners' unrealize() callback.

   bus_set_realized() does not roll back either.  Instead, it stops
   unrealizing.

   Fortunately, no unrealize method can fail, as we'll see below.

Clearer now?

With any luck, people will use the simpler qdev_unrealize() and
qbus_unrealize(), which is the form that doesn't let them get the error
handling wrong.  I like it when interfaces make misuse hard :)

On Tue, May 19, 2020 at 9:26 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Alistair Francis <alistair23@gmail.com> writes:
>
> > On Tue, May 19, 2020 at 8:11 AM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> We commonly plug devices into their bus right when we create them,
> >> like this:
> >>
> >>     dev = qdev_create(bus, type_name);
> >>
> >> Note that @dev is a weak reference.  The reference from @bus to @dev
> >> is the only strong one.
> >>
> >> We realize at some later time, either with
> >>
> >>     object_property_set_bool(OBJECT(dev), true, "realized", errp);
> >>
> >> or its convenience wrapper
> >>
> >>     qdev_init_nofail(dev);
> >>
> >> If @dev still has no QOM parent then, realizing makes the
> >> /machine/unattached/ orphanage its QOM parent.
> >>
> >> Note that the device returned by qdev_create() is plugged into a bus,
> >> but doesn't have a QOM parent, yet.  Until it acquires one,
> >> unrealizing the bus will hang in bus_unparent():
> >>
> >>     while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
> >>         DeviceState *dev = kid->child;
> >>         object_unparent(OBJECT(dev));
> >>     }
> >>
> >> object_unparent() does nothing when its argument has no QOM parent,
> >> and the loop spins forever.
> >>
> >> Device state "no QOM parent, but plugged into bus" is dangerous.
> >>
> >> Paolo suggested to delay plugging into the bus until realize.  We need
> >> to plug into the parent bus before we call the device's realize
> >> method, in case it uses the parent bus.  So the dangerous state still
> >> exists, but only within realization, where we can manage it safely.
> >>
> >> This commit creates infrastructure to do this:
> >>
> >>     dev = qdev_new(type_name);
> >>     ...
> >>     qdev_realize_and_unref(dev, bus, errp)
> >>
> >> Note that @dev becomes a strong reference here.
> >> qdev_realize_and_unref() drops it.  There is also plain
> >> qdev_realize(), which doesn't drop it.
> >>
> >> The remainder of this series will convert all users to this new
> >> interface.
> >>
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> >> Cc: Alistair Francis <alistair@alistair23.me>
> >> Cc: Gerd Hoffmann <kraxel@redhat.com>
> >> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >> Cc: David Gibson <david@gibson.dropbear.id.au>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  include/hw/qdev-core.h | 11 ++++-
> >>  hw/core/bus.c          | 14 +++++++
> >>  hw/core/qdev.c         | 94 ++++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 118 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> >> index b870b27966..fba29308f7 100644
> >> --- a/include/hw/qdev-core.h
> >> +++ b/include/hw/qdev-core.h
> >> @@ -57,7 +57,7 @@ typedef void (*BusUnrealize)(BusState *bus);
> >>   * After successful realization, setting static properties will fail.
> >>   *
> >>   * As an interim step, the #DeviceState:realized property can also be
> >> - * set with qdev_init_nofail().
> >> + * set with qdev_realize() or qdev_init_nofail().
> >>   * In the future, devices will propagate this state change to their children
> >>   * and along busses they expose.
> >>   * The point in time will be deferred to machine creation, so that values
> >> @@ -322,7 +322,13 @@ compat_props_add(GPtrArray *arr,
> >>
> >>  DeviceState *qdev_create(BusState *bus, const char *name);
> >>  DeviceState *qdev_try_create(BusState *bus, const char *name);
> >> +DeviceState *qdev_new(const char *name);
> >> +DeviceState *qdev_try_new(const char *name);
> >>  void qdev_init_nofail(DeviceState *dev);
> >> +bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp);
> >> +bool qdev_realize_and_unref(DeviceState *dev, BusState *bus, Error **errp);
> >> +void qdev_unrealize(DeviceState *dev);
> >> +
> >>  void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
> >>                                   int required_for_version);
> >>  HotplugHandler *qdev_get_bus_hotplug_handler(DeviceState *dev);
> >> @@ -411,6 +417,9 @@ typedef int (qdev_walkerfn)(DeviceState *dev, void *opaque);
> >>  void qbus_create_inplace(void *bus, size_t size, const char *typename,
> >>                           DeviceState *parent, const char *name);
> >>  BusState *qbus_create(const char *typename, DeviceState *parent, const char *name);
> >> +bool qbus_realize(BusState *bus, Error **errp);
> >> +void qbus_unrealize(BusState *bus);
> >> +
> >>  /* Returns > 0 if either devfn or busfn skip walk somewhere in cursion,
> >>   *         < 0 if either devfn or busfn terminate walk somewhere in cursion,
> >>   *           0 otherwise. */
> >> diff --git a/hw/core/bus.c b/hw/core/bus.c
> >> index 08c5eab24a..bf622604a3 100644
> >> --- a/hw/core/bus.c
> >> +++ b/hw/core/bus.c
> >> @@ -169,6 +169,20 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
> >>      return bus;
> >>  }
> >>
> >> +bool qbus_realize(BusState *bus, Error **errp)
> >> +{
> >> +    Error *err = NULL;
> >> +
> >> +    object_property_set_bool(OBJECT(bus), true, "realized", &err);
> >> +    error_propagate(errp, err);
> >> +    return !err;
> >> +}
> >> +
> >> +void qbus_unrealize(BusState *bus)
> >> +{
> >> +    object_property_set_bool(OBJECT(bus), true, "realized", &error_abort);
> >
> > Not false?
> >
> > Alistair
>
> Reasons it's &error_abort:

I meant why is this not setting the bool to false instead of true?

>
> 1. PATCH 06 and 07 transform variations of
>
>       object_property_set_bool(..., false, "realized", &error_abort);
>
>    to
>
>       qdev_unrealize(...);
>
>    No untransformed unrealization remain.  Thus, we always abort on
>    unrealization error before this series.
>
> 2. If unrealize could fail, we'd be in deep trouble.  Recent commit
>    b69c3c21a5 "qdev: Unrealize must not fail" explains:
>
>    Devices may have component devices and buses.
>
>    Device realization may fail.  Realization is recursive: a device's
>    realize() method realizes its components, and device_set_realized()
>    realizes its buses (which should in turn realize the devices on that
>    bus, except bus_set_realized() doesn't implement that, yet).
>
>    When realization of a component or bus fails, we need to roll back:
>    unrealize everything we realized so far.  If any of these unrealizes
>    failed, the device would be left in an inconsistent state.  Must not
>    happen.

Makes sense. Maybe worth putting this in a comment here?

>
>    device_set_realized() lets it happen: it ignores errors in the roll
>    back code starting at label child_realize_fail.
>
>    Since realization is recursive, unrealization must be recursive, too.
>    But how could a partly failed unrealize be rolled back?  We'd have to
>    re-realize, which can fail.  This design is fundamentally broken.
>
>    device_set_realized() does not roll back at all.  Instead, it keeps
>    unrealizing, ignoring further errors.
>
>    It can screw up even for a device with no buses: if the lone
>    dc->unrealize() fails, it still unregisters vmstate, and calls
>    listeners' unrealize() callback.
>
>    bus_set_realized() does not roll back either.  Instead, it stops
>    unrealizing.
>
>    Fortunately, no unrealize method can fail, as we'll see below.
>
> Clearer now?

Clear on the error_abort.

>
> With any luck, people will use the simpler qdev_unrealize() and
> qbus_unrealize(), which is the form that doesn't let them get the error
> handling wrong.  I like it when interfaces make misuse hard :)

Sounds good :)

Alistair

>

Alistair Francis <alistair23@gmail.com> writes:

> On Tue, May 19, 2020 at 9:26 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Alistair Francis <alistair23@gmail.com> writes:
>>
>> > On Tue, May 19, 2020 at 8:11 AM Markus Armbruster <armbru@redhat.com> wrote:
>> >>
>> >> We commonly plug devices into their bus right when we create them,
>> >> like this:
>> >>
>> >>     dev = qdev_create(bus, type_name);
>> >>
>> >> Note that @dev is a weak reference.  The reference from @bus to @dev
>> >> is the only strong one.
>> >>
>> >> We realize at some later time, either with
>> >>
>> >>     object_property_set_bool(OBJECT(dev), true, "realized", errp);
>> >>
>> >> or its convenience wrapper
>> >>
>> >>     qdev_init_nofail(dev);
>> >>
>> >> If @dev still has no QOM parent then, realizing makes the
>> >> /machine/unattached/ orphanage its QOM parent.
>> >>
>> >> Note that the device returned by qdev_create() is plugged into a bus,
>> >> but doesn't have a QOM parent, yet.  Until it acquires one,
>> >> unrealizing the bus will hang in bus_unparent():
>> >>
>> >>     while ((kid = QTAILQ_FIRST(&bus->children)) != NULL) {
>> >>         DeviceState *dev = kid->child;
>> >>         object_unparent(OBJECT(dev));
>> >>     }
>> >>
>> >> object_unparent() does nothing when its argument has no QOM parent,
>> >> and the loop spins forever.
>> >>
>> >> Device state "no QOM parent, but plugged into bus" is dangerous.
>> >>
>> >> Paolo suggested to delay plugging into the bus until realize.  We need
>> >> to plug into the parent bus before we call the device's realize
>> >> method, in case it uses the parent bus.  So the dangerous state still
>> >> exists, but only within realization, where we can manage it safely.
>> >>
>> >> This commit creates infrastructure to do this:
>> >>
>> >>     dev = qdev_new(type_name);
>> >>     ...
>> >>     qdev_realize_and_unref(dev, bus, errp)
>> >>
>> >> Note that @dev becomes a strong reference here.
>> >> qdev_realize_and_unref() drops it.  There is also plain
>> >> qdev_realize(), which doesn't drop it.
>> >>
>> >> The remainder of this series will convert all users to this new
>> >> interface.
>> >>
>> >> Cc: Michael S. Tsirkin <mst@redhat.com>
>> >> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>> >> Cc: Alistair Francis <alistair@alistair23.me>
>> >> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> >> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> >> Cc: David Gibson <david@gibson.dropbear.id.au>
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> ---
>> >>  include/hw/qdev-core.h | 11 ++++-
>> >>  hw/core/bus.c          | 14 +++++++
>> >>  hw/core/qdev.c         | 94 ++++++++++++++++++++++++++++++++++++++++++
>> >>  3 files changed, 118 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> >> index b870b27966..fba29308f7 100644
>> >> --- a/include/hw/qdev-core.h
>> >> +++ b/include/hw/qdev-core.h
>> >> @@ -57,7 +57,7 @@ typedef void (*BusUnrealize)(BusState *bus);
>> >>   * After successful realization, setting static properties will fail.
>> >>   *
>> >>   * As an interim step, the #DeviceState:realized property can also be
>> >> - * set with qdev_init_nofail().
>> >> + * set with qdev_realize() or qdev_init_nofail().
>> >>   * In the future, devices will propagate this state change to their children
>> >>   * and along busses they expose.
>> >>   * The point in time will be deferred to machine creation, so that values
>> >> @@ -322,7 +322,13 @@ compat_props_add(GPtrArray *arr,
>> >>
>> >>  DeviceState *qdev_create(BusState *bus, const char *name);
>> >>  DeviceState *qdev_try_create(BusState *bus, const char *name);
>> >> +DeviceState *qdev_new(const char *name);
>> >> +DeviceState *qdev_try_new(const char *name);
>> >>  void qdev_init_nofail(DeviceState *dev);
>> >> +bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp);
>> >> +bool qdev_realize_and_unref(DeviceState *dev, BusState *bus, Error **errp);
>> >> +void qdev_unrealize(DeviceState *dev);
>> >> +
>> >>  void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>> >>                                   int required_for_version);
>> >>  HotplugHandler *qdev_get_bus_hotplug_handler(DeviceState *dev);
>> >> @@ -411,6 +417,9 @@ typedef int (qdev_walkerfn)(DeviceState *dev, void *opaque);
>> >>  void qbus_create_inplace(void *bus, size_t size, const char *typename,
>> >>                           DeviceState *parent, const char *name);
>> >>  BusState *qbus_create(const char *typename, DeviceState *parent, const char *name);
>> >> +bool qbus_realize(BusState *bus, Error **errp);
>> >> +void qbus_unrealize(BusState *bus);
>> >> +
>> >>  /* Returns > 0 if either devfn or busfn skip walk somewhere in cursion,
>> >>   *         < 0 if either devfn or busfn terminate walk somewhere in cursion,
>> >>   *           0 otherwise. */
>> >> diff --git a/hw/core/bus.c b/hw/core/bus.c
>> >> index 08c5eab24a..bf622604a3 100644
>> >> --- a/hw/core/bus.c
>> >> +++ b/hw/core/bus.c
>> >> @@ -169,6 +169,20 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
>> >>      return bus;
>> >>  }
>> >>
>> >> +bool qbus_realize(BusState *bus, Error **errp)
>> >> +{
>> >> +    Error *err = NULL;
>> >> +
>> >> +    object_property_set_bool(OBJECT(bus), true, "realized", &err);
>> >> +    error_propagate(errp, err);
>> >> +    return !err;
>> >> +}
>> >> +
>> >> +void qbus_unrealize(BusState *bus)
>> >> +{
>> >> +    object_property_set_bool(OBJECT(bus), true, "realized", &error_abort);
>> >
>> > Not false?
>> >
>> > Alistair
>>
>> Reasons it's &error_abort:
>
> I meant why is this not setting the bool to false instead of true?

Pasto, I'm blind, tests didn't save me, but you did.  Thanks!

[...]

On 19/05/20 16:54, Markus Armbruster wrote:
> +
> +    object_ref(OBJECT(dev));
> +    object_property_set_bool(OBJECT(dev), true, "realized", &err);
> +    if (err) {
> +        error_propagate_prepend(errp, err,
> +                                "Initialization of device %s failed: ",
> +                                object_get_typename(OBJECT(dev)));
> +    }
> +    object_unref(OBJECT(dev));

Why is the ref/unref pair needed?  Should it be done in the realized
setter instead?

Paolo

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 19/05/20 16:54, Markus Armbruster wrote:
>> +
>> +    object_ref(OBJECT(dev));
>> +    object_property_set_bool(OBJECT(dev), true, "realized", &err);
>> +    if (err) {
>> +        error_propagate_prepend(errp, err,
>> +                                "Initialization of device %s failed: ",
>> +                                object_get_typename(OBJECT(dev)));
>> +    }
>> +    object_unref(OBJECT(dev));
>
> Why is the ref/unref pair needed?  Should it be done in the realized
> setter instead?

Copied from qdev_init_nofail(), where it is necessary (I figured out why
the hard way).  It doesn't seem to be necessary here, though.  Thanks!

On 20/05/20 10:11, Markus Armbruster wrote:
>> On 19/05/20 16:54, Markus Armbruster wrote:
>>> +
>>> +    object_ref(OBJECT(dev));
>>> +    object_property_set_bool(OBJECT(dev), true, "realized", &err);
>>> +    if (err) {
>>> +        error_propagate_prepend(errp, err,
>>> +                                "Initialization of device %s failed: ",
>>> +                                object_get_typename(OBJECT(dev)));
>>> +    }
>>> +    object_unref(OBJECT(dev));
>> Why is the ref/unref pair needed?  Should it be done in the realized
>> setter instead?
> Copied from qdev_init_nofail(), where it is necessary (I figured out why
> the hard way).  It doesn't seem to be necessary here, though.  Thanks!

Why is it necessary there?  It seems a bit iffy.

Paolo

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 20/05/20 10:11, Markus Armbruster wrote:
>>> On 19/05/20 16:54, Markus Armbruster wrote:
>>>> +
>>>> +    object_ref(OBJECT(dev));
>>>> +    object_property_set_bool(OBJECT(dev), true, "realized", &err);
>>>> +    if (err) {
>>>> +        error_propagate_prepend(errp, err,
>>>> +                                "Initialization of device %s failed: ",
>>>> +                                object_get_typename(OBJECT(dev)));
>>>> +    }
>>>> +    object_unref(OBJECT(dev));
>>> Why is the ref/unref pair needed?  Should it be done in the realized
>>> setter instead?
>> Copied from qdev_init_nofail(), where it is necessary (I figured out why
>> the hard way).  It doesn't seem to be necessary here, though.  Thanks!
>
> Why is it necessary there?  It seems a bit iffy.

My exact thoughts a few days back.  One debugging session later, I
understood, and put them right back.  Glad we have tests :)

When object_property_set_bool() fails in qdev_init_nofail(), the
reference count can drop to zero.  Certainly surprised me.  Have a look:

        dev = qdev_create(bus, type_name);
        // @dev is a weak reference, and @bus holds the only strong one
        ...
        qdev_init_nofail(dev);

In qdev_init_nofail():

        // object_ref(OBJECT(dev));
        object_property_set_bool(OBJECT(dev), true, "realized", &err);

This is a fancy way to call device_set_realized().  If something goes
wrong there, we execute

    fail:
        error_propagate(errp, local_err);
        if (unattached_parent) {
            /*
             * Beware, this doesn't just revert
             * object_property_add_child(), it also runs bus_remove()!
             */
            object_unparent(OBJECT(dev));
            unattached_count--;
        }

and bus_remove() drops the reference count to zero.

Back in qdev_init_nofail(), we then use after free:
    
    if (err) {
        error_reportf_err(err, "Initialization of device %s failed: ",
--->                      object_get_typename(OBJECT(dev)));
        exit(1);
    }
    // object_unref(OBJECT(dev));

The ref/unref keeps around @dev long enough for adding @dev's type name
to the error message.

The equivalent new pattern doesn't have this issue:

        dev = qdev_new(type_name);
        // @dev is the only reference
        ...
        qdev_realize_and_unref(dev, bus, errp);

In qdev_realize(), called via qdev_realize_and_unref():

        qdev_set_parent_bus(dev, bus);
        // @bus now holds the second reference

        // object_ref(OBJECT(dev));
        object_property_set_bool(OBJECT(dev), true, "realized", &err);

In device_set_realized(), the reference count drops to one, namely
@dev's reference.  That one goes away only in qdev_realize_and_unref(),
after we added @dev's type name to the error message.

However, a boring drive to the supermarket gave me this scenario:

        dev = qdev_new(type_name);
        // @dev is the only reference
        ...
        object_property_add_child(parent, name, OBJECT(dev));
        // @parent holds the second reference
        object_unref(dev);
        // unusual, but not wrong; @parent holds the only reference now
        ...
        qdev_realize(dev, bus, errp);

Here, the reference count can drop to zero when device_set_realized()
fails, and qdev_realize()'s object_get_typename() is a use after free.

Best to keep the ref/unref, I think.

On 20/05/20 16:42, Markus Armbruster wrote:
> If something goes
> wrong there, we execute
> 
>     fail:
>         error_propagate(errp, local_err);
>         if (unattached_parent) {
>             /*
>              * Beware, this doesn't just revert
>              * object_property_add_child(), it also runs bus_remove()!
>              */
>             object_unparent(OBJECT(dev));
>             unattached_count--;
>         }
> 
> and bus_remove() drops the reference count to zero.

Whoa whoa...  I didn't expect this from a failure to realize.

I think we should move the whole /machine/unattached dance into
qdev_realize, and just assert that a device being realized already has a
parent.  Then the ref/unref _will_ be unnecessary.  In the meanwhile, I
agree to either keep it or move it inside device_set_realized.

Thanks,

Paolo

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 20/05/20 16:42, Markus Armbruster wrote:
>> If something goes
>> wrong there, we execute
>> 
>>     fail:
>>         error_propagate(errp, local_err);
>>         if (unattached_parent) {
>>             /*
>>              * Beware, this doesn't just revert
>>              * object_property_add_child(), it also runs bus_remove()!
>>              */
>>             object_unparent(OBJECT(dev));
>>             unattached_count--;
>>         }
>> 
>> and bus_remove() drops the reference count to zero.
>
> Whoa whoa...  I didn't expect this from a failure to realize.

Me neither.  But by the time I understood what's going on here, my
appetite for big, structural QOM changes was pretty much gone, so I
merely added the "Beware" comment.

> I think we should move the whole /machine/unattached dance into
> qdev_realize, and just assert that a device being realized already has a
> parent.  Then the ref/unref _will_ be unnecessary.  In the meanwhile, I
> agree to either keep it or move it inside device_set_realized.

Could be done on top.  I might try for v2, if I can find the time.

Il lun 25 mag 2020, 08:30 Markus Armbruster <armbru@redhat.com> ha scritto:

> > I think we should move the whole /machine/unattached dance into
> > qdev_realize, and just assert that a device being realized already has a
> > parent.  Then the ref/unref _will_ be unnecessary.  In the meanwhile, I
> > agree to either keep it or move it inside device_set_realized.
>
> Could be done on top.  I might try for v2, if I can find the time.
>

Certainly on top.

Paolo

Markus Armbruster <armbru@redhat.com> writes:

> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> On 20/05/20 10:11, Markus Armbruster wrote:
>>>> On 19/05/20 16:54, Markus Armbruster wrote:
>>>>> +
>>>>> +    object_ref(OBJECT(dev));
>>>>> +    object_property_set_bool(OBJECT(dev), true, "realized", &err);
>>>>> +    if (err) {
>>>>> +        error_propagate_prepend(errp, err,
>>>>> +                                "Initialization of device %s failed: ",
>>>>> +                                object_get_typename(OBJECT(dev)));
>>>>> +    }
>>>>> +    object_unref(OBJECT(dev));
>>>> Why is the ref/unref pair needed?  Should it be done in the realized
>>>> setter instead?
>>> Copied from qdev_init_nofail(), where it is necessary (I figured out why
>>> the hard way).  It doesn't seem to be necessary here, though.  Thanks!
>>
>> Why is it necessary there?  It seems a bit iffy.
>
> My exact thoughts a few days back.  One debugging session later, I
> understood, and put them right back.  Glad we have tests :)
>
> When object_property_set_bool() fails in qdev_init_nofail(), the
> reference count can drop to zero.  Certainly surprised me.  Have a look:
>
>         dev = qdev_create(bus, type_name);
>         // @dev is a weak reference, and @bus holds the only strong one
>         ...
>         qdev_init_nofail(dev);
>
> In qdev_init_nofail():
>
>         // object_ref(OBJECT(dev));
>         object_property_set_bool(OBJECT(dev), true, "realized", &err);
>
> This is a fancy way to call device_set_realized().  If something goes
> wrong there, we execute
>
>     fail:
>         error_propagate(errp, local_err);
>         if (unattached_parent) {
>             /*
>              * Beware, this doesn't just revert
>              * object_property_add_child(), it also runs bus_remove()!
>              */
>             object_unparent(OBJECT(dev));
>             unattached_count--;
>         }
>
> and bus_remove() drops the reference count to zero.
>
> Back in qdev_init_nofail(), we then use after free:
>     
>     if (err) {
>         error_reportf_err(err, "Initialization of device %s failed: ",
> --->                      object_get_typename(OBJECT(dev)));
>         exit(1);
>     }
>     // object_unref(OBJECT(dev));
>
> The ref/unref keeps around @dev long enough for adding @dev's type name
> to the error message.
>
> The equivalent new pattern doesn't have this issue:
>
>         dev = qdev_new(type_name);
>         // @dev is the only reference
>         ...
>         qdev_realize_and_unref(dev, bus, errp);
>
> In qdev_realize(), called via qdev_realize_and_unref():
>
>         qdev_set_parent_bus(dev, bus);
>         // @bus now holds the second reference
>
>         // object_ref(OBJECT(dev));
>         object_property_set_bool(OBJECT(dev), true, "realized", &err);
>
> In device_set_realized(), the reference count drops to one, namely
> @dev's reference.  That one goes away only in qdev_realize_and_unref(),
> after we added @dev's type name to the error message.
>
> However, a boring drive to the supermarket gave me this scenario:
>
>         dev = qdev_new(type_name);
>         // @dev is the only reference
>         ...
>         object_property_add_child(parent, name, OBJECT(dev));
>         // @parent holds the second reference
>         object_unref(dev);
>         // unusual, but not wrong; @parent holds the only reference now
>         ...
>         qdev_realize(dev, bus, errp);
>
> Here, the reference count can drop to zero when device_set_realized()
> fails, and qdev_realize()'s object_get_typename() is a use after free.
>
> Best to keep the ref/unref, I think.

Actually, best to get rid of the "Initialization of device FOO failed: "
prefix, because:

    $ qemu-system-x86_64 -device virtio-blk
    qemu-system-x86_64: -device virtio-blk: Initialization of device virtio-blk-pci failed: Initialization of device virtio-blk-device failed: drive property not set

Ugly as sin.

The prefix exists for cases like this:

    $ qemu-system-x86_64 -vga cirrus -global cirrus-vga.vgamem_mb=99
    qemu-system-x86_64: Initialization of device cirrus-vga failed: Invalid cirrus_vga ram size '99'

Ideally, we'd point to the user configuration that caused the failure,
in this case -global cirrus-vga.vgamem_mb=99.  But that would be work,
so we made do with mentioning the device type.

Prefix pileup is not possible with qdev_init_nofail(), because the error
is immediately fatal there.

With qdev_realize(), realize failure commonly ripples through QOM
composition tree parents all the way to board initialization, and the
prefix gets added at every step.

If we want to keep the prefix, we could keep qdev_init_nofail(), then
figure out when to use it instead of qdev_realize().  That's a lot of
work.  I doubt it's worthwhile now.

I'll drop it.  Speak up if you want me to reconsider.

> This commit creates infrastructure to do this:
> 
>     dev = qdev_new(type_name);
>     ...
>     qdev_realize_and_unref(dev, bus, errp)

Note that this also allows to solve some initialization order issues,
specifically you can easily create devices (then do object operations
like adding/aliasing properties) before the bus is created.

Acked-by: Gerd Hoffmann <kraxel@redhat.com>