[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]