> On Jan 26, 2022, at 4:32 AM, Stefan Hajnoczi <stefa...@redhat.com> wrote:
>
> On Tue, Jan 25, 2022 at 02:43:33PM +0000, Jag Raman wrote:
>>
>>
>>> On Jan 25, 2022, at 5:27 AM, Stefan Hajnoczi <stefa...@redhat.com> wrote:
>>>
>>> On Wed, Jan 19, 2022 at 04:41:54PM -0500, Jagannathan Raman wrote:
>>>> 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 | 5 +++++
>>>> softmmu/qdev-monitor.c | 35 +++++++++++++++++++++++++++++++++++
>>>> 2 files changed, 40 insertions(+)
>>>>
>>>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>>>> index eed2983072..67df5e0081 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 {
>>>> @@ -433,6 +434,10 @@ typedef bool (QDevPutBusFunc)(BusState *bus, Error
>>>> **errp);
>>>> bool qdev_set_bus_cbs(QDevGetBusFunc *get_bus, QDevPutBusFunc *put_bus,
>>>> Error **errp);
>>>>
>>>> +int qdev_add_unplug_blocker(DeviceState *dev, Error *reason, Error
>>>> **errp);
>>>> +void qdev_del_unplug_blocker(DeviceState *dev, Error *reason);
>>>> +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 7306074019..1a169f89a2 100644
>>>> --- a/softmmu/qdev-monitor.c
>>>> +++ b/softmmu/qdev-monitor.c
>>>> @@ -978,10 +978,45 @@ void qmp_device_del(const char *id, Error **errp)
>>>> return;
>>>> }
>>>>
>>>> + if (qdev_unplug_blocked(dev, errp)) {
>>>> + return;
>>>> + }
>>>> +
>>>> qdev_unplug(dev, errp);
>>>> }
>>>> }
>>>>
>>>> +int qdev_add_unplug_blocker(DeviceState *dev, Error *reason, Error **errp)
>>>> +{
>>>> + ERRP_GUARD();
>>>> +
>>>> + if (!migration_is_idle()) {
>>>> + error_setg(errp, "migration is in progress");
>>>> + return -EBUSY;
>>>> + }
>>>
>>> Why can this function not be called during migration?
>>
>> Since ‘unplug_blockers' is a member of the device, I thought it wouldn’t be
>> correct to
>> allow changes to the device's state during migration.
>>
>> I did weigh the following reasons against adding this check:
>> - unplug_blockers is not migrated to the destination anyway, so it doesn’t
>> matter if
>> it changes after migration starts
>
> Yes.
>
>> - whichever code/object that needs to add the blocker could add it at the
>> destination
>> if needed
>
> Yes.
>
>> However, unlike qmp_device_add(), qmp_object_add() doesn’t reject during
>> migration. As such, an object could add a blocker for the device when
>> migration is
>> in progress.
>>
>> Would you prefer to throw a warning, or fully remove this test?
>
> Adding an unplug blocker during migration seems safe to me. I would
> remove this test.
OK, will do.
Thank you!
--
Jag
>
> Stefan
