[libvirt] [PATCH v2] qemu: Introduce caching whether /dev/kvm is accessible

[Daniel P. Berrangé]

On Mon, Nov 12, 2018 at 12:43:11PM +0100, Marc Hartmayer wrote:
> On Thu, Nov 01, 2018 at 10:18 AM +0100, "Daniel P. Berrangé" <berrange at redhat.com> wrote:
> > On Thu, Nov 01, 2018 at 09:31:50AM +0100, Martin Kletzander wrote:
> >> On Tue, Oct 30, 2018 at 03:07:59PM +0000, Daniel P. Berrangé wrote:
> >> > On Tue, Oct 30, 2018 at 03:45:36PM +0100, Michal Privoznik wrote:
> >> > > On 10/30/2018 02:46 PM, Michal Privoznik wrote:
> >> > > >
> >> > > > It is kernel problem. In my testing, the moment I call:
> >> > > >
> >> > > >  ioctl(kvm, KVM_CREATE_VM, 0);
> >> > >
> >> > > Okay, I have to retract this claim. 'udevadm monitor' shows some events:
> >> > >
> >> > > KERNEL[3631.129645] change   /devices/virtual/misc/kvm (misc)
> >> > > UDEV  [3631.130816] change   /devices/virtual/misc/kvm (misc)
> >> > >
> >> > > and stopping udevd leaves all three times untouched. So it is udev after
> >> > > all. I just don't know how to find the rule that is causing the issue.
> >> > > Anyway, as for this patch, I think we can merge it in the end, can't we?
> >> >
> >> > Not really. Udev is in use everywhere, so this behaviour makes the
> >> > patch useless in practice, even though it is technically right in
> >> > theory :-(
> >> >
> >>
> >> Does it?  With this behaviour we still do the "expensive" work after any machine
> >> has started.  But for one machine starting it still has the effect of running it
> >> only once.  And we *need* to run it for each machine unless we also cache the
> >> result per (at least) user:group of that machine as every machine can run under
> >> different user:group.
> >
> > Yes, with the current patch it still rechecks it for each VM startup
> 
> But that happens only because of udev (see Michal’s answer).
> 
> > , but
> > I was going to suggest the caching of this is simply put in a global static
> > variable as there's no reason to record this device permissions state in
> > the per VM caps cache.
> 
> I’m not sure I understand you correctly, but this patch doesn’t use the
> VM/QEMU capabilities cache (that would be 'struct _virQEMUCaps') for the
> caching. Instead it uses 'struct _virQEMUCapsCachePriv' and this struct
> is initialized only once when initializing the QEMU driver. This should
> be pretty similar to your proposal with the global static variable.

Oh yes, I missed that.  I think this is fine then, and I don't think we
need to deal with checking against arbitrary user ids


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|