Re: [libvirt] [PATCH] qemu: caps: Use unique key for domCaps caching
On Wed, 2019-10-16 at 09:12 -0300, Daniel Henrique Barboza wrote:
On 10/15/19 1:01 PM, Cole Robinson wrote:
> When searching qemuCaps->domCapsCache for existing domCaps data,
> we check for a matching pair of arch+virttype+machine+emulator.
> However
> for the hash table key we only use the machine string. So if the
> cache already contains:
>
> x86_64 + kvm + pc + /usr/bin/qemu-kvm
>
> But a new VM is defined with
>
> x86_64 + qemu + pc + /usr/bin/qemu-kvm
>
> We correctly fail to find matching cached domCaps, but then attempt
> to use a colliding key with virHashAddEntry
>
> Fix this by building a hash key from the 4 values, not just machine
>
> Signed-off-by: Cole Robinson <crobinso(a)redhat.com>
> ---
> qemu_domain.c validation should be affected, but it is covered up
> by another bug, fixed here:
> https://www.redhat.com/archives/libvir-list/2019-October/msg00708.html
>
> src/qemu/qemu_conf.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 08cd784054..64ac8cbdd3 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1396,6 +1396,8 @@
> virQEMUDriverGetDomainCapabilities(virQEMUDriverPtr driver,
> domCaps = virHashSearch(domCapsCache,
> virQEMUDriverSearchDomcaps, &data,
> NULL);
> if (!domCaps) {
> + VIR_AUTOFREE(char *) key = NULL;
> +
"g_autofree char *key = NULL;" instead of VIR_AUTOFREE for extra
karma
points. Everything else LGTM.
Reviewed-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
Looks good to me as well. But in keeping with the theme, we could also
use g_strdup_printf() below instead of virAsprintf(). That is one of
the suggestions made in the recent updates to our hacking documentation
from Daniel's glib patch series.
Reviewed-by: Jonathon Jongsma <jjongsma(a)redhat.com>
> /* hash miss, build new domcaps */
> if (!(domCaps = virDomainCapsNew(data.path, data.machine,
> data.arch,
> data.virttype)))
> @@ -1406,7 +1408,14 @@
> virQEMUDriverGetDomainCapabilities(virQEMUDriverPtr driver,
> cfg->firmwares, cfg-
> >nfirmwares) < 0)
> return NULL;
>
> - if (virHashAddEntry(domCapsCache, machine, domCaps) < 0)
> + if (virAsprintf(&key, "%d:%d:%s:%s",
> + data.arch,
> + data.virttype,
> + NULLSTR(data.machine),
> + NULLSTR(data.path)) < 0)
> + return NULL;
> +
> + if (virHashAddEntry(domCapsCache, key, domCaps) < 0)
> return NULL;
> }
>