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

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.

> 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.

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.

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