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

[Jag Raman]

> 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
  - whichever code/object that needs to add the blocker could add it at the 
destination
    if needed

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?

--
Jag 

> 
>> +
>> +    dev->unplug_blockers = g_slist_prepend(dev->unplug_blockers, reason);
>> +
>> +    return 0;
>> +}
>> +
>> +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
>>