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

[Pavel Hrdina]

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.

>
>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>> index 81ada48..dae89aa 100644
>> --- a/src/qemu/qemu_capabilities.c
>> +++ b/src/qemu/qemu_capabilities.c
>> @@ -286,6 +286,7 @@ struct _virQEMUCaps {
>>
>>       char *binary;
>>       time_t ctime;
>> +    time_t selfctime;
>>
>>       virBitmapPtr flags;
>>
>> @@ -2689,7 +2690,7 @@ virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char *filename)
>>       virBufferAsprintf(&buf, "<qemuctime>%llu</qemuctime>\n",
>>                         (long long)qemuCaps->ctime);
>>       virBufferAsprintf(&buf, "<selfctime>%llu</selfctime>\n",
>> -                      (long long)virGetSelfLastChanged());
>> +                      (long long)qemuCaps->selfctime);
>>
>>       if (qemuCaps->usedQMP)
>>           virBufferAddLit(&buf, "<usedQMP/>\n");
>> @@ -2743,7 +2744,7 @@ virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char *filename)
>>       VIR_DEBUG("Saved caps '%s' for '%s' with (%lld, %lld)",
>>                 filename, qemuCaps->binary,
>>                 (long long)qemuCaps->ctime,
>> -              (long long)virGetSelfLastChanged());
>> +              (long long)qemuCaps->selfctime);
>>
>>       ret = 0;
>>    cleanup:
>> @@ -2871,12 +2872,12 @@ virQEMUCapsInitCached(virQEMUCapsPtr qemuCaps, const char *cacheDir)
>>
>>       /* Discard if cache is older that QEMU binary */
>>       if (qemuctime != qemuCaps->ctime ||
>> -        selfctime < virGetSelfLastChanged()) {
>> +        selfctime < qemuCaps->selfctime) {
>>           VIR_DEBUG("Outdated cached capabilities '%s' for '%s' "
>>                     "(%lld vs %lld, %lld vs %lld)",
>>                     capsfile, qemuCaps->binary,
>>                     (long long)qemuctime, (long long)qemuCaps->ctime,
>> -                  (long long)selfctime, (long long)virGetSelfLastChanged());
>> +                  (long long)selfctime, (long long)qemuCaps->selfctime);
>>           ignore_value(unlink(capsfile));
>>           virQEMUCapsReset(qemuCaps);
>>           ret = 0;
>> @@ -3371,6 +3372,7 @@ virQEMUCapsPtr virQEMUCapsNewForBinary(const char *binary,
>>           goto error;
>>       }
>>       qemuCaps->ctime = sb.st_ctime;
>> +    qemuCaps->selfctime = virGetSelfLastChanged();
>>
>>       /* Make sure the binary we are about to try exec'ing exists.
>>        * Technically we could catch the exec() failure, but that's
>> @@ -3420,7 +3422,8 @@ bool virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps)
>>       if (stat(qemuCaps->binary, &sb) < 0)
>>           return false;
>>
>> -    return sb.st_ctime == qemuCaps->ctime;
>> +    return sb.st_ctime == qemuCaps->ctime &&
>> +        virGetSelfLastChanged() >= qemuCaps->selfctime;
>>   }
>
> Huh, this doesn't make any sense.
>
> The virQEMUCapsIsValid() method is used to invalidate the cache if
> QEMU binary changes while libvirtd is running. It is not relevant
> whether libvirtd itself has changed here, because changes to the
> libvirtd binary on disk have no effect until libvirtd is restarted.

That's a good point and I've missed that.

>
> When libvirtd is (re)started, the virQEMUCapsInitCached will already
> check the cache selfctime vs its own virGetSelfLastChanged() and
> throw away the cache file if stale.

As I've wrote above, there is still issue with the FIPS that it can be 
enabled without updating libvirtd. And yes you are right that QEMU 
should autodetect it, but it seems they are not planning to do anything 
about that and probably we have to deal with it and somehow make it work.

I'll try to come up with different patch.

Thanks,
Pavel

>
> So I don't see any bug here that needs fixing.
>
> Regards,
> Daniel
>