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

[Cole Robinson]

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 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>
>

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