Re: [PATCH v5 05/18] qdev: unplug blocker for devices

[Jag Raman]

> On Jan 26, 2022, at 4:32 AM, Stefan Hajnoczi <stefanha@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 <stefanha@redhat.com> wrote:
>>> 
>>> On Wed, Jan 19, 2022 at 04:41:54PM -0500, Jagannathan Raman wrote:
>>>> 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 |  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