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

Re: [libvirt] [PATCH] util: mdev: support persistent devices with mdevctl



On Wed, 2019-08-14 at 16:14 +0200, Boris Fiuczynski wrote:
> On 8/14/19 12:02 AM, Jonathon Jongsma wrote:
> > When a host is rebooted, mediated devices disappear from
> > sysfs.  mdevctl
> > (https://github.com/mdevctl/mdevctl) is a utility for managing and
> > persisting these devices. It maintains a registry of mediated
> > devices
> > and can start and stop them by UUID.
> > 
> > when libvirt attempts to create a new mediated device object, we
> > currently
> > fail if the path does not exist in sysfs. This patch tries a little
> > bit
> > harder by using mdevctl to attempt to activate the device.  The
> > approach
> > is fairly naive -- it just calls 'mdevctl start -u $UUID' without
> > checking whether this UUID is registered with mdevctl or not.
> > 
> > See https://bugzilla.redhat.com/show_bug.cgi?id=1699274
> > 
> > Signed-off-by: Jonathon Jongsma <jjongsma redhat com>
> > ---
> > NOTES:
> > - an argument could be made that we should simply do nothing here.
> > mdevctl does
> >    have support for automatically activating the mediated device
> > when the parent
> >    device becomes available (via udev). So if the administrator set
> > up the mdev
> >    to start automatically, this patch should not even be necessary.
> > That said, I
> >    think this patch could still be useful.
> 
> I would actually like to use this argument since this patch 
> unconditionally starts/creates a persistently defined mdev without
> ever 
> stopping/destroying it and also not looking if it is defined as to
> be 
> automatically started or manually started. If the mdev is specified
> in 
> mdevctl to be started automatically I would rate this as mdevctl is
> in 
> control of this mdevs lifecycle and libvirt should not interfere
> with 
> it. It might be that I am over-interpreting auto and manual as start 
> options in mdevctl but it feels to me that libvirt and mdevctl
> should 
> not run into a management clash of host resources.
> 
> In addition what about a user specifiable tag in the domain xmls
> mdev 
> definition which controls the management behavior of an mdev with 
> mdevctl or another tool?

Thanks for the feedback. I welcome additional opinions on this. If
there's a concensus that the right approach is to do nothing, I can
drop this patch. Or alternately, we could simply point users toward
mdevctl in the error message. For example, something like:


diff --git a/src/util/virmdev.c b/src/util/virmdev.c
index 3d5488cdae..70d990eace 100644
--- a/src/util/virmdev.c
+++ b/src/util/virmdev.c
@@ -149,7 +149,7 @@ virMediatedDeviceNew(const char *uuidstr,
virMediatedDeviceModelType model)
 
     if (!virFileExists(sysfspath)) {
         virReportError(VIR_ERR_DEVICE_MISSING,
-                       _("mediated device '%s' not found"), uuidstr);
+                       _("mediated device '%s' not found. Persistent
devices can be managed with 'mdevctl'."), uuidstr);
         return NULL;
     }
 


> 
> 
> > - As I said above, the approach is pretty naive. I could have
> > checked whether
> >    the device was defined in mdevctl's registry and given a
> > different error
> >    message ("try registering this device with mdevctl") in that
> > scenario. But
> >    I chose to keep it simple.
> > 
> >   src/util/virmdev.c | 22 +++++++++++++++++++---
> >   1 file changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/util/virmdev.c b/src/util/virmdev.c
> > index 3d5488cdae..7a2f0adedc 100644
> > --- a/src/util/virmdev.c
> > +++ b/src/util/virmdev.c
> > @@ -25,6 +25,7 @@
> >   #include "virfile.h"
> >   #include "virstring.h"
> >   #include "viralloc.h"
> > +#include "vircommand.h"
> >   
> >   #define VIR_FROM_THIS VIR_FROM_NONE
> >   
> > @@ -148,9 +149,24 @@ virMediatedDeviceNew(const char *uuidstr,
> > virMediatedDeviceModelType model)
> >           return NULL;
> >   
> >       if (!virFileExists(sysfspath)) {
> > -        virReportError(VIR_ERR_DEVICE_MISSING,
> > -                       _("mediated device '%s' not found"),
> > uuidstr);
> > -        return NULL;
> > +        bool activated = false;
> > +        /* if the mdev device path doesn't exist, we may still be
> > able to start
> > +         * the device using mdevctl
> > +         */
> > +        char *mdevctl = virFindFileInPath("mdevctl");
> > +        if (mdevctl) {
> > +            const char *runargs[] = { mdevctl, "start", "-u",
> > uuidstr, NULL };
> > +            if (virRun(runargs, NULL) == 0) {
> > +                /* check to see if it the device exists now */
> > +                activated = virFileExists(sysfspath);
> > +            }
> > +        }
> > +
> > +        if (!activated) {
> > +            virReportError(VIR_ERR_DEVICE_MISSING,
> > +                           _("mediated device '%s' not found"),
> > uuidstr);
> > +            return NULL;
> > +        }
> >       }
> >   
> >       if (VIR_ALLOC(dev) < 0)
> > 
> 
> 


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