Re: [Qemu-devel] [PULL 13/20] qdev: add "hotpluggable" property to Device

[Igor Mammedov]

On Tue, 18 Feb 2014 17:35:16 +0100
Andreas Färber <address@hidden> wrote:

> Am 10.02.2014 17:48, schrieb Michael S. Tsirkin:
> > From: Igor Mammedov <address@hidden>
> > 
> > Currently it's possible to make PCIDevice not hotpluggable
> > by using no_hotplug field of PCIDeviceClass. However it
> > limits this only to PCI devices and prevents from
> > generalizing hotplug code.
> > 
> > So add similar field to DeviceClass so it could be reused
> > with other Devices and would allow to replace PCI specific
> > hotplug callbacks with generic implementation. Following
> > patches will replace PCIDeviceClass.no_hotplug with this
> > new property.
> > 
> > In addition expose field as "hotpluggable" readonly property,
> > to make it possible to read its value via QOM interface.
> > 
> > Make DeviceClass hotpluggable by default as it was assumed
> > before.
> > 
> > Signed-off-by: Igor Mammedov <address@hidden>
> > Reviewed-by: Michael S. Tsirkin <address@hidden>
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > ---
> >  include/hw/qdev-core.h |  3 +++
> >  hw/core/qdev.c         | 29 +++++++++++++++++++++++++++++
> >  2 files changed, 32 insertions(+)
> > 
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index 41ec533..08d329d 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -50,6 +50,8 @@ struct VMStateDescription;
> >   * is changed to %true. Deprecated, new types inheriting directly from
> >   * TYPE_DEVICE should use @realize instead, new leaf types should consult
> >   * their respective parent type.
> > + * @hotpluggable: indicates if #DeviceClass is hotpluggable, available
> > + * as readonly "hotpluggable" property of #DeviceState instance
> >   *
> >   * # Realization #
> >   * Devices are constructed in two stages,
> > @@ -110,6 +112,7 @@ typedef struct DeviceClass {
> >       * TODO remove once we're there
> >       */
> >      bool cannot_instantiate_with_device_add_yet;
> > +    bool hotpluggable;
> >  
> >      /* callbacks */
> >      void (*reset)(DeviceState *dev);
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index c9f0c33..5c864db 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -215,6 +215,12 @@ void qdev_unplug(DeviceState *dev, Error **errp)
> >      }
> >      assert(dc->unplug != NULL);
> >  
> > +    if (!dc->hotpluggable) {
> > +        error_set(errp, QERR_DEVICE_NO_HOTPLUG,
> > +                  object_get_typename(OBJECT(dev)));
> > +        return;
> > +    }
> > +
> >      qdev_hot_removed = true;
> >  
> >      if (dc->unplug(dev) < 0) {
> > @@ -694,6 +700,11 @@ static void device_set_realized(Object *obj, bool 
> > value, Error **err)
> >      DeviceClass *dc = DEVICE_GET_CLASS(dev);
> >      Error *local_err = NULL;
> >  
> > +    if (dev->hotplugged && !dc->hotpluggable) {
> > +        error_set(err, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));
> > +        return;
> > +    }
> > +
> >      if (value && !dev->realized) {
> >          if (!obj->parent && local_err == NULL) {
> >              static int unattached_count;
> > @@ -734,6 +745,14 @@ static void device_set_realized(Object *obj, bool 
> > value, Error **err)
> >      dev->realized = value;
> >  }
> >  
> > +static bool device_get_hotpluggable(Object *obj, Error **err)
> > +{
> > +    DeviceClass *dc = DEVICE_GET_CLASS(obj);
> > +    DeviceState *dev = DEVICE(obj);
> > +
> > +    return dc->hotpluggable && dev->parent_bus->allow_hotplug;
> > +}
> > +
> >  static void device_initfn(Object *obj)
> >  {
> >      DeviceState *dev = DEVICE(obj);
> > @@ -750,6 +769,8 @@ static void device_initfn(Object *obj)
> >  
> >      object_property_add_bool(obj, "realized",
> >                               device_get_realized, device_set_realized, 
> > NULL);
> > +    object_property_add_bool(obj, "hotpluggable",
> > +                             device_get_hotpluggable, NULL, NULL);
> >  
> >      class = object_get_class(OBJECT(dev));
> >      do {
> > @@ -786,6 +807,14 @@ static void device_class_base_init(ObjectClass *class, 
> > void *data)
> >       * so do not propagate them to the subclasses.
> >       */
> >      klass->props = NULL;
> > +
> > +    /* by default all devices were considered as hotpluggable,
> > +     * so with intent to check it in generic qdev_unplug() /
> > +     * device_set_realized() functions make every device
> > +     * hotpluggable. Devices that shouldn't be hotpluggable,
> > +     * should override it in their class_init()
> > +     */
> > +    klass->hotpluggable = true;
> 
> Is this intentionally being added to class_base_init or should this have
> gone into class_init? As seen above, class_base_init has so far been
> used to "erase" stuff set by the parent classes, not to set random defaults.
You are right it should be in class_init(), I'll post fix shortly.

> Regards,
> Andreas
> 
> >  }
> >  
> >  static void device_unparent(Object *obj)
>