[libvirt] [PATCH] domcaps: Check for architecture more wisely

[Cole Robinson]

On 04/09/2015 09:27 AM, Cole Robinson wrote:
> On 04/09/2015 04:00 AM, Michal Privoznik wrote:
>> On 08.04.2015 17:25, Cole Robinson wrote:
>>> On 04/08/2015 11:12 AM, Michal Privoznik wrote:
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1209948
>>>>
>>>> So we have this bug. The virConnectGetDomainCapabilities() API
>>>> performs a couple of checks before it produces any result. One of
>>>> the checks is if the architecture requested by user can be run by
>>>> the binary (again user provided). However, the check is pretty
>>>> dumb. It merely compares if the default binary architecture
>>>> matches the one provided by user. However, a qemu binary can run
>>>> multiple architectures. For instance: qemu-system-ppc64 can run:
>>>> ppc, ppcle, ppc64, ppc64le and ppcemb. The default is ppc64, so
>>>> if user requested something else, like ppc64le, the check would
>>>> have failed without obvious reason.
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>>> ---
>>>>  src/qemu/qemu_driver.c | 5 ++++-
>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>>> index 921417c..d217481 100644
>>>> --- a/src/qemu/qemu_driver.c
>>>> +++ b/src/qemu/qemu_driver.c
>>>> @@ -18788,7 +18788,10 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn,
>>>>          arch_from_caps = virQEMUCapsGetArch(qemuCaps);
>>>>  
>>>>          if (arch_from_caps != arch &&
>>>> -            (arch_from_caps != VIR_ARCH_X86_64 || arch != VIR_ARCH_I686)) {
>>>> +            !((ARCH_IS_X86(arch) && ARCH_IS_X86(arch_from_caps)) ||
>>>> +              (ARCH_IS_PPC(arch) && ARCH_IS_PPC(arch_from_caps)) ||
>>>> +              (ARCH_IS_ARM(arch) && ARCH_IS_ARM(arch_from_caps)) ||
>>>> +              (ARCH_IS_S390(arch) && ARCH_IS_S390(arch_from_caps)))) {
>>>>              virReportError(VIR_ERR_INVALID_ARG,
>>>>                             _("architecture from emulator '%s' doesn't "
>>>>                               "match given architecture '%s'"),
>>>>
>>>
>>> As I mentioned in the bug, really I think this is more a problem of the
>>> virQEMUCapsCacheLookup* interface. Nowadays we can have the same emulatorbin
>>> map to multiple architectures, and the same architecture provided by multiple
>>> emulators, so CacheLookup* functions that search for only the specified
>>> emulator or only the specified arch are just going to breed more bugs like this.
>>
>> While I understand your concern, qemu is not helping. It is not
>> exporting list of supported architectures for given binary. What I've
>> found is:
>>
>> a) we use 'query-target' command to determine the target architecture
>> for the binary. However, this returns only the default architecture, not
>> a list of all supported architectures.
>>
>> b) There exists 'qemu-system-* -cpu ?', which outputs something like this:
>>
>> x86           athlon  QEMU Virtual CPU version 2.2.92
>> x86        Broadwell  Intel Core Processor (Broadwell)
>>
>> The first item looks like an arch. But it's not much of a help either.
>> For example for qemu-system-{i386,x86_64} I get the very same list of
>> supported CPUs. And architectures too. But if I query those two binaries
>> on QMP for their target they produce different results: 'i386' and 'x86_64'.
>>
>> While I agree your approach is better, there's not much we can do unless
>> qemu is more helping. So I see two options:
>>
>> 1) ask qemu devels
>>
>> 2) merge this meanwhile
> 
> I don't really follow. Even if qemu isn't making life easy for us, AFAICT we
> still have all the info we need tracked in our internal qemu capabilities cache.
> 
> Why can't we add a function like:
> 
> virQEMUCapsCacheLookupFull(virQEMUCapsCachePtr cache,
> 			   const char *binary,
>                            virArch arch)
> 
> Which uses virHashSearch to only return the internal cache object that matches
> the binary + machineType + arch tuple? There should be an entry that matches
> qemu-system-ppc64 + arch=ppc64le   just like there's a separate entry for
> qemu-system-ppc64 + arch=ppc64, correct? Use that in domcapabilities, then the
> IS_ARCH whitelist shouldn't be needed, and we have an internal API that can be
> used to fix these issues if they exist with other CapsCacheLookup callers
> 

I started working on this, should have patches posted by the end of the day.
But they are getting a little invasive for backporting.

So ACK to the original patch. I'll rebase my cleanup patches on top of this
one, and we can backport this simple patch to stable distros.

Thanks,
Cole