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

[Jonathon Jongsma]

On Tue, 5 Jan 2021 17:25:50 +0100
Erik Skultety <eskultet at redhat.com> wrote:

> 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 at redhat.com>
> > ---
> >  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.

hmm, I apparently missed that comment. But I guess the reason that I
added it to the mdev caps rather than the object is because it
really only applies to mdev devices. For instance, we don't really have
a mechanism to keep around the definition of e.g. a PCI device that has
been removed from the machine. And I don't really see any use case for
doing so. I suppose we could just set this value to 'false' for all
non-mdev node devices, but that doesn't seem very elegant to me.

> 
> ...
> 
> >  
> >      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