[libvirt] [PATCH] qemu: caps: Use unique key for domCaps caching

[Jonathon Jongsma]

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 at 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 at 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 at 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;
> >       }
> >