qemu: caps: Use unique key for domCaps caching

Message ID d4d77b18bee5121a8e172069fa331fb72836f27e.1571155220.git.crobinso@redhat.com
State Accepted
Commit fbf7c23c2dfae0ff879c98130c3765b4c1ebf34e
Headers show
Series
  • qemu: caps: Use unique key for domCaps caching
Related show

Commit Message

Cole Robinson Oct. 15, 2019, 4:01 p.m.
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@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(-)

-- 
2.23.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

Daniel Henrique Barboza Oct. 16, 2019, 12:12 p.m. | #1
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@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@gmail.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;

>       }

>   


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Jonathon Jongsma Oct. 16, 2019, 4:22 p.m. | #2
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@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@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@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;

> >       }

> >   


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Oct. 17, 2019, 7:03 p.m. | #3
On 10/16/19 12:22 PM, Jonathon Jongsma wrote:
> 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@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@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@redhat.com>

>


I decided to leave the virAsprintf as is, since it sounds like
g_strdup_printf usage is in flux a bit and I figure there will be a mass
conversion at some point.

Thanks,
Cole

> 

>>

>>

>>>           /* 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;

>>>       }

>>>   

> 



- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patch

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;
+
         /* 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;
     }