Re: [PATCH v6 03/19] qdev: unplug blocker for devices

[Stefan Hajnoczi]

On Thu, Feb 17, 2022 at 02:48:50AM -0500, Jagannathan Raman wrote:
> Add blocker to prevent hot-unplug of devices
> 
> 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 | 35 +++++++++++++++++++++++++++++++++++
>  softmmu/qdev-monitor.c | 26 ++++++++++++++++++++++++++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 92c3d65208..4b1d77f44a 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,40 @@ void qdev_simple_device_unplug_cb(HotplugHandler 
> *hotplug_dev,
>  void qdev_machine_creation_done(void);
>  bool qdev_machine_modified(void);
>  
> +/**
> + * Device Unplug blocker: prevents a device from being unplugged. It could
      ^^^^^^^^^^^^^^^^^^^^^

This looks strange. gtkdoc will probably treat it as the doc comment for
qdev_add_unplug_blocker(), which is actually defined below. I suggest
not trying to define a new section in the documentation and instead just
focussing on doc comments for qdev_add_unplug_block() and other
functions.

The gtkdoc way of defining sections is covered here but it's almost
never used in QEMU:
https://developer-old.gnome.org/gtk-doc-manual/stable/documenting_sections.html.en

> + * be used to indicate that another object depends on the device.
> + *
> + * 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);

Does the caller have to call qdev_del_unplug_blocker() later?

An assert(!dev->unplug_blockers) would be nice when DeviceState is
destroyed. That way leaks will be caught.

> +
> +/**
> + * 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/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 01f3834db5..69d9cf3f25 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -945,10 +945,36 @@ void qmp_device_del(const char *id, Error **errp)
>              return;
>          }
>  
> +        if (qdev_unplug_blocked(dev, errp)) {
> +            return;
> +        }
> +
>          qdev_unplug(dev, errp);
>      }
>  }
>  
> +void qdev_add_unplug_blocker(DeviceState *dev, Error *reason)

These functions belong in hw/core/qdev.c because they are part of the
DeviceState API, not qdev monitor commands?

> +{
> +    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;
> +}
> +
>  void hmp_device_add(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;
> -- 
> 2.20.1
>

signature.asc
Description: PGP signature