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