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

[Pavel Hrdina]

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
>