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

[Markus Armbruster]

Alistair Francis <address@hidden> writes:

> On Tue, May 19, 2020 at 9:26 PM Markus Armbruster <address@hidden> wrote:
>>
>> Alistair Francis <address@hidden> writes:
>>
>> > On Tue, May 19, 2020 at 8:11 AM Markus Armbruster <address@hidden> 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 <address@hidden>
>> >> Cc: Marcel Apfelbaum <address@hidden>
>> >> Cc: Alistair Francis <address@hidden>
>> >> Cc: Gerd Hoffmann <address@hidden>
>> >> Cc: Mark Cave-Ayland <address@hidden>
>> >> Cc: David Gibson <address@hidden>
>> >> Signed-off-by: Markus Armbruster <address@hidden>
>> >> ---
>> >>  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!

[...]