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