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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jan 14, 2021 at 05:07:10PM -0600, Jonathon Jongsma wrote:
> On Tue, 5 Jan 2021 17:25:50 +0100
> Erik Skultety <eskultet@xxxxxxxxxx> 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@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;  

There are some trailing whitespaces FWIW.

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

I know, but I would not mind sacrificing a bit of elegance in exchange for
remaining consistent across other drivers in libvirt - IOW persistence is a
property of the XML definition, not the capability itself, regardless of the
fact that for other types of devices we have no use for it - 'false' for
everything else sounds like an acceptable trade-off to me. Unless you have some
other compelling reason to break this consistency in which case I'm open to
reconsider.

Erik




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux