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 <jjong...@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)
> >
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
