Re: [PATCH v8 02/17] qdev: unplug blocker for devices

[Markus Armbruster]

Jagannathan Raman <jag.raman@oracle.com> writes:

> Add blocker to prevent hot-unplug of devices

Why do you need this?  I'm not doubting you do, I just want to read your
reasons here :)

>
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> ---
>  include/hw/qdev-core.h | 29 +++++++++++++++++++++++++++++
>  hw/core/qdev.c         | 24 ++++++++++++++++++++++++
>  softmmu/qdev-monitor.c |  4 ++++
>  3 files changed, 57 insertions(+)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 92c3d65208..1b9fa25e5c 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -193,6 +193,7 @@ struct DeviceState {
>      int instance_id_alias;
>      int alias_required_for_version;
>      ResettableState reset;
> +    GSList *unplug_blockers;
>  };
>  
>  struct DeviceListener {
> @@ -419,6 +420,34 @@ void qdev_simple_device_unplug_cb(HotplugHandler 
> *hotplug_dev,
>  void qdev_machine_creation_done(void);
>  bool qdev_machine_modified(void);
>  
> +/*
> + * qdev_add_unplug_blocker: Adds an unplug blocker to a device
> + *
> + * @dev: Device to be blocked from unplug
> + * @reason: Reason for blocking
> + */
> +void qdev_add_unplug_blocker(DeviceState *dev, Error *reason);
> +
> +/*
> + * qdev_del_unplug_blocker: Removes an unplug blocker from a device
> + *
> + * @dev: Device to be unblocked
> + * @reason: Pointer to the Error used with qdev_add_unplug_blocker.
> + *          Used as a handle to lookup the blocker for deletion.
> + */
> +void qdev_del_unplug_blocker(DeviceState *dev, Error *reason);
> +
> +/*
> + * qdev_unplug_blocked: Confirms if a device is blocked from unplug
> + *
> + * @dev: Device to be tested
> + * @reason: Returns one of the reasons why the device is blocked,
> + *          if any
> + *
> + * Returns: true if device is blocked from unplug, false otherwise
> + */
> +bool qdev_unplug_blocked(DeviceState *dev, Error **errp);
> +
>  /**
>   * GpioPolarity: Polarity of a GPIO line
>   *
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 84f3019440..0806d8fcaa 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -468,6 +468,28 @@ char *qdev_get_dev_path(DeviceState *dev)
>      return NULL;
>  }
>  
> +void qdev_add_unplug_blocker(DeviceState *dev, Error *reason)
> +{
> +    dev->unplug_blockers = g_slist_prepend(dev->unplug_blockers, reason);
> +}
> +
> +void qdev_del_unplug_blocker(DeviceState *dev, Error *reason)
> +{
> +    dev->unplug_blockers = g_slist_remove(dev->unplug_blockers, reason);
> +}
> +
> +bool qdev_unplug_blocked(DeviceState *dev, Error **errp)
> +{
> +    ERRP_GUARD();
> +
> +    if (dev->unplug_blockers) {
> +        error_propagate(errp, error_copy(dev->unplug_blockers->data));
> +        return true;
> +    }
> +
> +    return false;
> +}

This cites the most recently added blocker as reason.  Your function
comment covers it: "Returns one of the reasons".  Okay.

> +
>  static bool device_get_realized(Object *obj, Error **errp)
>  {
>      DeviceState *dev = DEVICE(obj);
> @@ -704,6 +726,8 @@ static void device_finalize(Object *obj)
>  
>      DeviceState *dev = DEVICE(obj);
>  
> +    g_assert(!dev->unplug_blockers);
> +
>      QLIST_FOREACH_SAFE(ngl, &dev->gpios, node, next) {
>          QLIST_REMOVE(ngl, node);
>          qemu_free_irqs(ngl->in, ngl->num_in);
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 12fe60c467..9cfd59d17c 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -898,6 +898,10 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>      HotplugHandlerClass *hdc;
>      Error *local_err = NULL;
>  
> +    if (qdev_unplug_blocked(dev, errp)) {
> +        return;
> +    }
> +
>      if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) {
>          error_setg(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
>          return;