[libvirt] [PATCH] domcaps: Check for architecture more wisely
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'"), -- 2.0.5
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. So rather than extend this specific site with a whitelist that could grow out of date, I'd rather see a CacheLookup function added that takes at least arch + emulator, but probably also machine type and virt type (like domcapabilities takes) and finds the actually requested caps. And separately we should audit all other uses of CacheLookup* to make sure other bugs aren't lingering, or just file a bz for it Thanks, Cole
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 Michal
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 - Cole
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. Thanks, Cole
participants (2)
-
Cole Robinson -
Michal Privoznik