Re: [PATCH 03/55] qdev: New qdev_new(), qdev_realize(), etc.

[Markus Armbruster <armbru@redhat.com>]

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