On Thu, Dec 24, 2020 at 08:14:30AM -0600, Jonathon Jongsma wrote:
> Since a mediated device can be persistently defined by the mdevctl
> backend, we need additional lifecycle events beyond CREATED/DELETED to
> indicate that e.g. the device has been stopped but the device definition
> still exists.
>
> Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> ---
> examples/c/misc/event-test.c | 4 ++++
> include/libvirt/libvirt-nodedev.h | 2 ++
> src/conf/node_device_conf.h | 1 +
> src/node_device/node_device_driver.c | 1 +
> src/node_device/node_device_udev.c | 25 +++++++++++++++++++++++--
> tools/virsh-nodedev.c | 4 +++-
> 6 files changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/examples/c/misc/event-test.c b/examples/c/misc/event-test.c
> index f164e825e1..d6eec648ec 100644
> --- a/examples/c/misc/event-test.c
> +++ b/examples/c/misc/event-test.c
> @@ -381,6 +381,10 @@ nodeDeviceEventToString(int event)
> return "Created";
> case VIR_NODE_DEVICE_EVENT_DELETED:
> return "Deleted";
> + case VIR_NODE_DEVICE_EVENT_STOPPED:
> + return "Stopped";
> + case VIR_NODE_DEVICE_EVENT_STARTED:
> + return "Started";
> case VIR_NODE_DEVICE_EVENT_LAST:
> break;
> }
> diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h
> index d304283871..a473563857 100644
> --- a/include/libvirt/libvirt-nodedev.h
> +++ b/include/libvirt/libvirt-nodedev.h
> @@ -197,6 +197,8 @@ int virConnectNodeDeviceEventDeregisterAny(virConnectPtr conn,
> typedef enum {
> VIR_NODE_DEVICE_EVENT_CREATED = 0,
> VIR_NODE_DEVICE_EVENT_DELETED = 1,
> + VIR_NODE_DEVICE_EVENT_STOPPED = 2,
> + VIR_NODE_DEVICE_EVENT_STARTED = 3,
>
> # ifdef VIR_ENUM_SENTINELS
> VIR_NODE_DEVICE_EVENT_LAST
> diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
> index 3d7872fd6e..bbc28cf2b9 100644
> --- a/src/conf/node_device_conf.h
> +++ b/src/conf/node_device_conf.h
> @@ -157,6 +157,7 @@ struct _virNodeDevCapMdev {
> char *uuid;
> virMediatedDeviceAttrPtr *attributes;
> size_t nattributes;
> + bool persistent;
So, this patch is essentially unchanged since v2. In v2 I suggested ^this
attribute should be bound to the object not the capabability and you haven't
replied to those comments, is there a reason why that is not the case? It's
also consistent with what we do for other objects, like domains and networks.
...
>
> if (!(obj = virNodeDeviceObjListFindBySysfsPath(driver->devs, path))) {
> VIR_DEBUG("Failed to find device to remove that has udev path '%s'",
> @@ -1409,13 +1411,32 @@ udevRemoveOneDeviceSysPath(const char *path)
> }
> def = virNodeDeviceObjGetDef(obj);
>
> + /* If the device is a mediated device that has been 'stopped', it may still
> + * be defined by mdevctl and can therefore be started again. Don't drop it
> + * from the list of node devices */
> + cap = def->caps;
> + while (cap != NULL) {
> + if (cap->data.type == VIR_NODE_DEV_CAP_MDEV) {
> + if (cap->data.mdev.persistent) {
> + VIR_FREE(def->sysfs_path);
> + event_type = VIR_NODE_DEVICE_EVENT_STOPPED;
> + break;
> + }
> + }
> + cap = cap->next;
> + }
> +
...which would simplify ^this while loop to a mere:
if (obj->persitent) {
VIR_FREE(def->sysfs_path);
virNodeDeviceObjSetActive(obj, false);
}
> event = virNodeDeviceEventLifecycleNew(def->name,
> - VIR_NODE_DEVICE_EVENT_DELETED,
> + event_type,
> 0);
>
> VIR_DEBUG("Removing device '%s' with sysfs path '%s'",
> def->name, path);
> - virNodeDeviceObjListRemove(driver->devs, obj);
> +
> + if (event_type == VIR_NODE_DEVICE_EVENT_DELETED)
> + virNodeDeviceObjListRemove(driver->devs, obj);
> + else
> + virNodeDeviceObjSetActive(obj, false);
...and this 'else' branch would be unnecessary then.
> virNodeDeviceObjEndAPI(&obj);
>
> virObjectEventStateQueue(driver->nodeDeviceEventState, event);
Regards,
Erik
