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