> On Feb 21, 2022, at 10:27 AM, Stefan Hajnoczi <stefa...@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.ufimts...@oracle.com>
>> Signed-off-by: John G Johnson <john.g.john...@oracle.com>
>> Signed-off-by: Jagannathan Raman <jag.ra...@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
>>
