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(a)redhat.com&gt; >> --- >> 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; }