Re: [libvirt PATCH v3 06/21] nodedev: add STOPPED/STARTED lifecycle events

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(a)redhat.com&gt; > --- > 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