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

[Jag Raman]

> On Feb 21, 2022, at 10:27 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> 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.

Sorry I assumed that we needed an extra ‘*’ at the beginning of the
comment. I got this idea when checking out block.c and blockdev.c
while working on the migration patch.

I’ll follow the “Comment style” section in “docs/devel/style.rst"

> 
> 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.

Makes sense, will add.

> 
>> +
>> +/**
>> + * 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?

Both hw/core/qdev.c and softmmu/qdev-monitor.c seem to manage the
DeviceState.

softmmu/qdev-monitor.c seems to manage device addition and
removal using qdev_device_add() and qdev_unplug(). I noticed
that some functions in this file change DeviceState. For example,
qdev_device_add() sets DeviceState->opts, qdev_set_id() sets
DeviceState->id. Given the above two reasons, I thought it the
unplug blockers could be better place here.

Since hw/core/qdev.c makes the majority of changes to the
DeviceState, moving the unplug blockers over there makes
sense to me.

Thank you!
--
Jag

> 
>> +{
>> +    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
>>