[libvirt] [PATCH 6/8] qemu: Unify cached caps validity checks

[Pavel Hrdina]

On Wed, Nov 02, 2016 at 10:22:35AM +0100, Jiri Denemark wrote:
> Let's keep all run time validation of cached QEMU capabilities in
> virQEMUCapsIsValid and call it whenever we access the cache.
> virQEMUCapsInitCached should keep only the checks which do not make
> sense once the cache is loaded in memory.
> 
> Signed-off-by: Jiri Denemark <jdenemar at redhat.com>
> ---
>  src/qemu/qemu_capabilities.c | 59 ++++++++++++++++++++++++++++++--------------
>  src/qemu/qemu_capabilities.h |  3 ++-
>  2 files changed, 42 insertions(+), 20 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 91e8b30..d1c31ad 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -3516,25 +3516,21 @@ virQEMUCapsInitCached(virCapsPtr caps,
>          VIR_WARN("Failed to load cached caps from '%s' for '%s': %s",
>                   capsfile, qemuCaps->binary, virGetLastErrorMessage());
>          virResetLastError();
> -        ret = 0;
> -        virQEMUCapsReset(qemuCaps);
> -        goto cleanup;
> +        goto discard;
>      }
>  
> +    if (!virQEMUCapsIsValid(qemuCaps, qemuctime))
> +        goto discard;
> +
>      /* Discard cache if QEMU binary or libvirtd changed */
> -    if (qemuctime != qemuCaps->ctime ||
> -        selfctime != virGetSelfLastChanged() ||
> +    if (selfctime != virGetSelfLastChanged() ||
>          selfvers != LIBVIR_VERSION_NUMBER) {
> -        VIR_DEBUG("Outdated cached capabilities '%s' for '%s' "
> -                  "(%lld vs %lld, %lld vs %lld, %lu vs %lu)",
> +        VIR_DEBUG("Dropping cached capabilities '%s' for '%s': "
> +                  "libvirt changed (%lld vs %lld, %lu vs %lu)",

Nice cleanup, the only thing that I would change is to split the DEBUG and move
the "Dropping cached capabilities '%s' for '%s'" and put it ...

>                    capsfile, qemuCaps->binary,
> -                  (long long)qemuCaps->ctime, (long long)qemuctime,
>                    (long long)selfctime, (long long)virGetSelfLastChanged(),
>                    selfvers, (unsigned long)LIBVIR_VERSION_NUMBER);
> -        ignore_value(unlink(capsfile));
> -        virQEMUCapsReset(qemuCaps);
> -        ret = 0;
> -        goto cleanup;
> +        goto discard;
>      }
>  
>      VIR_DEBUG("Loaded '%s' for '%s' ctime %lld usedQMP=%d",
> @@ -3548,6 +3544,12 @@ virQEMUCapsInitCached(virCapsPtr caps,
>      VIR_FREE(capsfile);
>      VIR_FREE(capsdir);
>      return ret;
> +
> + discard:

... here where we actually drop the capability.

> +    ignore_value(unlink(capsfile));
> +    virQEMUCapsReset(qemuCaps);
> +    ret = 0;
> +    goto cleanup;
>  }
>  
>  
> @@ -4111,17 +4113,36 @@ virQEMUCapsNewForBinary(virCapsPtr caps,
>  }
>  
>  
> -bool virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps)
> +bool
> +virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps,
> +                   time_t ctime)
>  {
> -    struct stat sb;
> -
>      if (!qemuCaps->binary)
>          return true;
>  
> -    if (stat(qemuCaps->binary, &sb) < 0)
> -        return false;
> +    if (!ctime) {
> +        struct stat sb;
>  
> -    return sb.st_ctime == qemuCaps->ctime;
> +        if (stat(qemuCaps->binary, &sb) < 0) {
> +            char ebuf[1024];
> +            VIR_DEBUG("Dropping cached capabilities for '%s': "
> +                      "failed to stat QEMU binary: %s",
> +                      qemuCaps->binary,
> +                      virStrerror(errno, ebuf, sizeof(ebuf)));

Same here, remove the "Dropping cached capabilities for '%s': " part and ...

> +            return false;
> +        }
> +        ctime = sb.st_ctime;
> +    }
> +
> +    if (ctime != qemuCaps->ctime) {
> +        VIR_DEBUG("Dropping cached capabilities for '%s': "
> +                  "binary is newer than cache (%lld vs %lld)",
> +                  qemuCaps->binary,
> +                  (long long) ctime, (long long) qemuCaps->ctime);

... same here.  This function doesn't drop the capabilities.

ACK, the suggested change is only for debug logs so I'll leave it up to you,
but it would be cleaner :)

Pavel

> +        return false;
> +    }
> +
> +    return true;
>  }
>  
>  
> @@ -4214,7 +4235,7 @@ virQEMUCapsCacheLookup(virCapsPtr caps,
>      virMutexLock(&cache->lock);
>      ret = virHashLookup(cache->binaries, binary);
>      if (ret &&
> -        !virQEMUCapsIsValid(ret)) {
> +        !virQEMUCapsIsValid(ret, 0)) {
>          VIR_DEBUG("Cached capabilities %p no longer valid for %s",
>                    ret, binary);
>          virHashRemoveEntry(cache->binaries, binary);
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index c77ba13..6c45a67 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -449,7 +449,8 @@ int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr qemuCaps,
>                                     size_t *nmachines,
>                                     virCapsGuestMachinePtr **machines);
>  
> -bool virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps);
> +bool virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps,
> +                        time_t ctime);
>  
>  void virQEMUCapsFilterByMachineType(virQEMUCapsPtr qemuCaps,
>                                      const char *machineType);
> -- 
> 2.10.2
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161103/092896d9/attachment-0001.sig>