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

[Andreas Färber]

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;

This is breaking my extended qom-test. You add this property to each
device, irrespective of whether it has a non-NULL parent_bus, which
results in a SIGSEGV when qom-get'ting it. Will post a fix.

Next time please ping for review of such core changes.

Andreas

> +}
> +
>  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;
>  }
>  
>  static void device_unparent(Object *obj)
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg