[libvirt] [PATCH 10/11] qemu: switch QEMU capabilities to use virFileCache

[Jiri Denemark]

On Mon, Jul 10, 2017 at 14:46:49 +0200, Pavel Hrdina wrote:
> The switch contains considerable amount of changes:
> 
>   virQEMUCapsRememberCached() is removed because this is now handled
>   by virFileCacheSave().
> 
>   virQEMUCapsInitCached() is removed because this is now handled by
>   virFileCacheLoad().
> 
>   virQEMUCapsNewForBinary() is split into two functions,
>   virQEMUCapsNewData() which creates new data if there is nothing
>   cached and virQEMUCapsLoadFile() which loads the cached data.
>   This is now handled by virFileCacheNewData().
> 
>   virQEMUCapsCacheValidate() is removed because this is now handled by
>   virFileCacheValidate().
> 
>   virQEMUCapsCacheFree() is removed because it's no longer required.
> 
>   Add virCapsPtr into virQEMUCapsCachePriv because for each call of
>   virFileCacheLookup*() we need to use current virCapsPtr.
> 
> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> ---
>  src/qemu/qemu_capabilities.c | 322 ++++++++++++-------------------------------
>  src/qemu/qemu_capabilities.h |  17 +--
>  src/qemu/qemu_capspriv.h     |   8 +-
>  src/qemu/qemu_conf.h         |   3 +-
>  src/qemu/qemu_driver.c       |   2 +-
>  tests/testutilsqemu.c        |   9 +-
>  tests/testutilsqemu.h        |   3 +-
>  7 files changed, 102 insertions(+), 262 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index e190cfa8b1..c3e09616de 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
...
>  virQEMUCapsPtr
>  virQEMUCapsCacheLookup(virCapsPtr caps,
> -                       virQEMUCapsCachePtr cache,
> +                       virFileCachePtr cache,
>                         const char *binary)
>  {
>      virQEMUCapsPtr ret = NULL;
> +    virQEMUCapsCachePrivPtr priv = virFileCacheGetPriv(cache);
>  
> -    virMutexLock(&cache->lock);
> -
> -    ret = virHashLookup(cache->binaries, binary);
> -    virQEMUCapsCacheValidate(cache, binary, caps, &ret);
> -    virObjectRef(ret);
> -
> -    virMutexUnlock(&cache->lock);
> +    priv->caps = caps;
> +    ret = virFileCacheLookup(cache, binary);
> +    priv->caps = NULL;

This is not safe anymore since the cache is locked only inside
virFileCacheLookup. I think you will need to use a new parameter for
passing the capabilities to virFileCacheLookup.

>  
>      VIR_DEBUG("Returning caps %p for %s", ret, binary);
>      return ret;
...
> @@ -5478,64 +5351,39 @@ virQEMUCapsCompareArch(const void *payload,
>  
>  virQEMUCapsPtr
>  virQEMUCapsCacheLookupByArch(virCapsPtr caps,
> -                             virQEMUCapsCachePtr cache,
> +                             virFileCachePtr cache,
>                               virArch arch)
>  {
>      virQEMUCapsPtr ret = NULL;
> +    virQEMUCapsCachePrivPtr priv = virFileCacheGetPriv(cache);
>      virArch target;
>      struct virQEMUCapsSearchData data = { .arch = arch };
>  
> -    virMutexLock(&cache->lock);
> -    ret = virHashSearch(cache->binaries, virQEMUCapsCompareArch, &data, NULL);
> +    priv->caps = caps;
> +    ret = virFileCacheLookupByFunc(cache, virQEMUCapsCompareArch, &data);
>      if (!ret) {
>          /* If the first attempt at finding capabilities has failed, try
>           * again using the QEMU target as lookup key instead */
>          target = virQEMUCapsFindTarget(virArchFromHost(), data.arch);
>          if (target != data.arch) {
>              data.arch = target;
> -            ret = virHashSearch(cache->binaries, virQEMUCapsCompareArch,
> -                                &data, NULL);
> +            ret = virFileCacheLookupByFunc(cache, virQEMUCapsCompareArch, &data);
>          }
>      }
> +    priv->caps = NULL;

This is unsafe too.

>  
> -    if (ret) {
> -        char *binary;
> -
> -        if (VIR_STRDUP(binary, ret->binary) < 0) {
> -            ret = NULL;
> -        } else {
> -            virQEMUCapsCacheValidate(cache, binary, caps, &ret);
> -            VIR_FREE(binary);
> -        }
> -    } else {
> +    if (!ret) {
>          virReportError(VIR_ERR_INVALID_ARG,
>                         _("unable to find any emulator to serve '%s' "
>                           "architecture"), virArchToString(arch));
>      }
>  
> -    virObjectRef(ret);
> -    virMutexUnlock(&cache->lock);
> -
>      VIR_DEBUG("Returning caps %p for arch %s", ret, virArchToString(arch));
>  
>      return ret;
>  }

Jirka