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

[Daniel P. Berrange]

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
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|