1. Patchew
  2. Libvirt
  3. View series

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

Cole Robinson posted 1 patch 4 years, 6 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/d4d77b18bee5121a8e172069fa331fb72836f27e.1571155220.git.crobinso@redhat.com
src/qemu/qemu_conf.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
[libvirt] [PATCH] qemu: caps: Use unique key for domCaps caching
Posted by Cole Robinson 4 years, 6 months ago 
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;
+
         /* 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;
     }
 
-- 
2.23.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: caps: Use unique key for domCaps caching
Posted by Daniel Henrique Barboza 4 years, 6 months ago 

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
Re: [libvirt] [PATCH] qemu: caps: Use unique key for domCaps caching
Posted by Jonathon Jongsma 4 years, 6 months ago 
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
Re: [libvirt] [PATCH] qemu: caps: Use unique key for domCaps caching
Posted by Cole Robinson 4 years, 6 months ago 
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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.