[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] qemu_capabilities: fix issue with discarding old capabilities



On 09/15/2014 11:49 AM, Daniel P. Berrange wrote:
On Mon, Sep 15, 2014 at 11:43:10AM +0200, Pavel Hrdina wrote:
On 09/15/2014 11:24 AM, Daniel P. Berrange wrote:
On Fri, Sep 12, 2014 at 06:42:08PM +0200, Pavel Hrdina wrote:
On 09/12/2014 06:25 PM, Daniel P. Berrange wrote:
On Fri, Sep 12, 2014 at 06:10:44PM +0200, Pavel Hrdina wrote:
There was a bug that if libvirtd binary has been updated than the
capability file wasn't reloaded therefore new capabilities introduced
in libvirt cannot be used because the cached version was loaded.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1135431

That bug is all about FIPS support.

Yes it's about FIPS support but it's already in libvirt. I've tested it
and actually by removing cached file to force detect new capabilities and
after that it worked.

Now I realized that even checking the selfctime during start of libvirtd
isn't sufficient because you can enable the FIPS support for kenrel without
updating the libvirtd binary.

Ah, so the actual bug is that the capabilities we detect have a dependancy
on (libvirtd binary, qemu binary, sysfs/procfs settings). It is pretty
difficult to deal with sysfs/procfs chances & caching here, since there's
no way I know to detect when sysfs/procfs settings change.

Yes, that's the real bug and I also didn't realize that at first.

There is however one more think I'm not sure about. I didn't find any place
where we are discarding old capabilities if the libvirtd binary has been
changed. The only check for that update is in function
"virQEMUCapsInitCached" and its called only from "virQEMUCapsNewForBinary"
and this function is called only if there is no cached caps or the qemu
binary has changed. See the "virQEMUCapsCacheLookup".

So it seems that there is also a bug that we don't check on libvirtd start
if there was an update of that binary.

When libvirtd starts up the cache will be empty. So virQEMUCapsCacheLookup
will always call virQEMUCapsNewForBinary which will call virQEMUCapsInitCached.
So it will always check timestamps on startup.

Well, that's not true. The cache file survive libvirtd stop/start and if there is existing cache file, it will be dropped only if qemu binary has been changed.


I wouldn't want to check the sysfs/procfs settings every time. Perhaps it
would suffice to just do a check on sysfs/procfs when libvirtd starts up,
so we can say that if you change FIPS sysfs settings you must restart
libvirtd ?

I think that would be good enough.

The difficultly though is figuring out whehther the files have changed. For
this, we'd need to record what the original sysfs/procfs values were in the
caps cache, so we then have something to compare.

A completely different way of looking at this problem would be to say that
the virQEMUCapsPtr should *only* reflect settings that are related to the
QEMU binary capabilities. ie sysfs/procs should not be allowed to influence
the capabilities flags. This is kind of what was originally assumed for this
data.

I wonder how many of our capability flags are set based on data that is
not from the QEMU binary ?  If we can elminate that, then the caching problem
will go away.

It seems that the FIPS is the only case that shouldn't be in qemuCaps. I'll create a new patch where I'll move that code directly to qemu_command.

Pavel


Regards,
Daniel



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]