Re: [PATCH v8 02/17] qdev: unplug blocker for devices

[Jag Raman <jag.raman@oracle.com>]

> On Apr 22, 2022, at 1:18 AM, Markus Armbruster <armbru@redhat.com> wrote:
> 
> Jag Raman <jag.raman@oracle.com> writes:
> 
>>> On Apr 21, 2022, at 10:55 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>> 
>>> Jagannathan Raman <jag.raman@oracle.com> writes:
>>> 
>>>> Add blocker to prevent hot-unplug of devices
>>> 
>>> Why do you need this?  I'm not doubting you do, I just want to read your
>>> reasons here :)
>> 
>> Hi Markus, :)
>> 
>> The x-vfio-user-server depends on an attached PCIDevice. As long as x-vfio-user-server
>> is used, we don’t want the PCIDevice to be unplugged. This blocker prevents an user
>> from removing PCIDevice while the vfio-user server is in use.
> 
> Please work that into your commit message.  Perhaps along the lines of
> 
>    One of the next commits will do <stuff>.  <badness> will happen when
>    the PCI device is unplugged.  Create the means to prevent that.

OK, will do.

Thank you!
--
Jag

> 
>>>> 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>
>> 
>> I recall receiving a “Reviewed-by” from Stefan previously.
>> 
>> I’m very sorry I didn’t add that here. I’ll go over all the patches once again to confirm that
>> the “Reviewed-by” status reflects accurately.
>> 
>>>> ---
>>>> include/hw/qdev-core.h | 29 +++++++++++++++++++++++++++++
>>>> hw/core/qdev.c         | 24 ++++++++++++++++++++++++
>>>> softmmu/qdev-monitor.c |  4 ++++
>>>> 3 files changed, 57 insertions(+)
>>>> 
>>>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>>>> index 92c3d65208..1b9fa25e5c 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,34 @@ void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
>>>> void qdev_machine_creation_done(void);
>>>> bool qdev_machine_modified(void);
>>>> 
>>>> +/*
>>>> + * 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);
>>>> +
>>>> +/*
>>>> + * 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/hw/core/qdev.c b/hw/core/qdev.c
>>>> index 84f3019440..0806d8fcaa 100644
>>>> --- a/hw/core/qdev.c
>>>> +++ b/hw/core/qdev.c
>>>> @@ -468,6 +468,28 @@ char *qdev_get_dev_path(DeviceState *dev)
>>>>    return NULL;
>>>> }
>>>> 
>>>> +void qdev_add_unplug_blocker(DeviceState *dev, Error *reason)
>>>> +{
>>>> +    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;
>>>> +}
>>> 
>>> This cites the most recently added blocker as reason.  Your function
>>> comment covers it: "Returns one of the reasons".  Okay.
>> 
>> I could change the comment to say that it returns the recently added reason.
> 
> Up to you.
>