[PATCH 3/3] qemu_capabilities: Reword domain caps cache

[Peter Krempa]

On Fri, Jan 24, 2020 at 11:27:18 +0100, Michal Privoznik wrote:
> Since v5.6.0-48-g270583ed98 we try to cache domain capabilities,
> i.e. store filled virDomainCaps in a hash table in virQEMUCaps
> for future use. However, there's a race condition in the way it's
> implemented. We use virQEMUCapsGetDomainCapsCache() to obtain the
> pointer to the hash table, then we search the hash table for
> cached data and if none is found the domcaps is constructed and
> put into the table. Problem is that this is all done without any
> locking, so if there are two threads trying to do the same, one
> will succeed and the other will fail inserting the data into the
> table.
> 
> Also, the API looks a bit fishy - obtaining pointer to the hash
> table is dangerous.
> 
> The solution is to use a mutex that guards the whole operation
> with the hash table. Then, the API can be changes to return
> virDomainCapsPtr directly.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1791790
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_capabilities.c | 118 +++++++++++++++++++++++++++++++++--
>  src/qemu/qemu_capabilities.h |  14 ++++-
>  src/qemu/qemu_conf.c         |  69 +++++---------------
>  3 files changed, 141 insertions(+), 60 deletions(-)

[...]

> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 8040f8ab3a..e0cb8be675 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1337,31 +1337,20 @@ virCapsPtr virQEMUDriverGetCapabilities(virQEMUDriverPtr driver,
>  }
>  
>  
> -struct virQEMUDriverSearchDomcapsData {
> -    const char *path;
> -    const char *machine;
> -    virArch arch;
> -    virDomainVirtType virttype;
> -};
> -
> -
>  static int
> -virQEMUDriverSearchDomcaps(const void *payload,
> -                           const void *name G_GNUC_UNUSED,
> -                           const void *opaque)
> +virQEMUDriverGetDomainCapabilitiesFiller(virDomainCapsPtr domCaps,
> +                                         virQEMUCapsPtr qemuCaps,
> +                                         void *opaque)
>  {
> -    virDomainCapsPtr domCaps = (virDomainCapsPtr) payload;
> -    struct virQEMUDriverSearchDomcapsData *data = (struct virQEMUDriverSearchDomcapsData *) opaque;
> -
> -    if (STREQ_NULLABLE(data->path, domCaps->path) &&
> -        STREQ_NULLABLE(data->machine, domCaps->machine) &&
> -        data->arch == domCaps->arch &&
> -        data->virttype == domCaps->virttype)
> -        return 1;
> +    virQEMUDriverPtr driver = opaque;
> +    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>  
> -    return 0;
> +    return virQEMUCapsFillDomainCaps(qemuCaps, driver->hostarch, domCaps,
> +                                     driver->privileged,
> +                                     cfg->firmwares, cfg->nfirmwares);

This is weird. You have +virQEMUDriverGetDomainCapabilitiesFiller in
qemu_conf.c which internally uses virQEMUCapsFillDomainCaps exported by
qemu_capabilities.c and pass it to virQEMUCapsGetDomainCapsCache which
is defined in qemu_capabilities.c.

Can't we just get rid of the callback?

The rest looks good.