[libvirt] [PATCH] caps: Don't default to i686 of KVM on x86_64
My commit 747761a79 (v1.2.15 only) dropped this bit of logic when filling in a default arch in the XML: - /* First try to find one matching host arch */ - for (i = 0; i < caps->nguests; i++) { - if (caps->guests[i]->ostype == ostype) { - for (j = 0; j < caps->guests[i]->arch.ndomains; j++) { - if (caps->guests[i]->arch.domains[j]->type == domain && - caps->guests[i]->arch.id == caps->host.arch) - return caps->guests[i]->arch.id; - } - } - } That attempt to match host.arch is important, otherwise we end up defaulting to i686 on x86_64 host for KVM, which is not intended. Duplicate it in the centralized CapsLookup function. Additionally add some testcases that would have caught this. https://bugzilla.redhat.com/show_bug.cgi?id=1219191 --- src/conf/capabilities.c | 63 +++++++++++++++------- .../qemuxml2argv-default-kvm-host-arch.args | 4 ++ .../qemuxml2argv-default-kvm-host-arch.xml | 11 ++++ tests/qemuxml2argvtest.c | 1 + .../qemuxml2xmlout-default-kvm-host-arch.xml | 21 ++++++++ tests/qemuxml2xmltest.c | 1 + tests/testutilsqemu.c | 12 +++++ 7 files changed, 94 insertions(+), 19 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-default-kvm-host-arch.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-default-kvm-host-arch.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-default-kvm-host-arch.xml diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 922741f..c43bfb3 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -607,25 +607,13 @@ virCapsDomainDataCompare(virCapsGuestPtr guest, return true; } -/** - * virCapabilitiesDomainDataLookup: - * @caps: capabilities to query - * @ostype: guest operating system type, of enum VIR_DOMAIN_OSTYPE - * @arch: Architecture to search for - * @domaintype: domain type to search for, of enum VIR_DOMAIN_VIRT - * @emulator: Emulator path to search for - * @machinetype: Machine type to search for - * - * Search capabilities for the passed values, and if found return - * virCapabilitiesDomainDataLookup filled in with the default values - */ -virCapsDomainDataPtr -virCapabilitiesDomainDataLookup(virCapsPtr caps, - int ostype, - virArch arch, - int domaintype, - const char *emulator, - const char *machinetype) +static virCapsDomainDataPtr +virCapabilitiesDomainDataLookupInternal(virCapsPtr caps, + int ostype, + virArch arch, + int domaintype, + const char *emulator, + const char *machinetype) { virCapsGuestPtr foundguest = NULL; virCapsGuestDomainPtr founddomain = NULL; @@ -730,6 +718,43 @@ virCapabilitiesDomainDataLookup(virCapsPtr caps, return ret; } +/** + * virCapabilitiesDomainDataLookup: + * @caps: capabilities to query + * @ostype: guest operating system type, of enum VIR_DOMAIN_OSTYPE + * @arch: Architecture to search for + * @domaintype: domain type to search for, of enum VIR_DOMAIN_VIRT + * @emulator: Emulator path to search for + * @machinetype: Machine type to search for + * + * Search capabilities for the passed values, and if found return + * virCapabilitiesDomainDataLookup filled in with the default values + */ +virCapsDomainDataPtr +virCapabilitiesDomainDataLookup(virCapsPtr caps, + int ostype, + virArch arch, + int domaintype, + const char *emulator, + const char *machinetype) +{ + virCapsDomainDataPtr ret; + + if (arch == VIR_ARCH_NONE) { + /* Prefer host arch if its available */ + ret = virCapabilitiesDomainDataLookupInternal(caps, ostype, + caps->host.arch, + domaintype, + emulator, machinetype); + if (ret) + return ret; + } + + return virCapabilitiesDomainDataLookupInternal(caps, ostype, + arch, domaintype, + emulator, machinetype); +} + static int virCapabilitiesFormatNUMATopology(virBufferPtr buf, size_t ncells, diff --git a/tests/qemuxml2argvdata/qemuxml2argv-default-kvm-host-arch.args b/tests/qemuxml2argvdata/qemuxml2argv-default-kvm-host-arch.args new file mode 100644 index 0000000..102691f --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-default-kvm-host-arch.args @@ -0,0 +1,4 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/kvm -S -machine pc,accel=kvm -m 4096 -smp 4 -nographic \ +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -net none \ +-serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-default-kvm-host-arch.xml b/tests/qemuxml2argvdata/qemuxml2argv-default-kvm-host-arch.xml new file mode 100644 index 0000000..66dead0 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-default-kvm-host-arch.xml @@ -0,0 +1,11 @@ +<domain type='kvm'> + <name>kvm</name> + <uuid>d091ea82-29e6-2e34-3005-f02617b36e87</uuid> + <memory unit='KiB'>4194304</memory> + <currentMemory unit='KiB'>4194304</currentMemory> + <vcpu placement='static'>4</vcpu> + <os> + <type>hvm</type> + <boot dev='hd'/> + </os> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 97c7fba..b27e3a4 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -599,6 +599,7 @@ mymain(void) DO_TEST_FAILURE("machine-xen-vmport-opt", QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_MACHINE_VMPORT_OPT); DO_TEST("kvm", QEMU_CAPS_MACHINE_OPT); + DO_TEST("default-kvm-host-arch", QEMU_CAPS_MACHINE_OPT); DO_TEST("boot-cdrom", NONE); DO_TEST("boot-network", NONE); DO_TEST("boot-floppy", NONE); diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-default-kvm-host-arch.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-default-kvm-host-arch.xml new file mode 100644 index 0000000..30fa66d --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-default-kvm-host-arch.xml @@ -0,0 +1,21 @@ +<domain type='kvm'> + <name>kvm</name> + <uuid>d091ea82-29e6-2e34-3005-f02617b36e87</uuid> + <memory unit='KiB'>4194304</memory> + <currentMemory unit='KiB'>4194304</currentMemory> + <vcpu placement='static'>4</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/kvm</emulator> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index b611afd..a21a299 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -346,6 +346,7 @@ mymain(void) DO_TEST("minimal"); DO_TEST("machine-core-on"); DO_TEST("machine-core-off"); + DO_TEST_DIFFERENT("default-kvm-host-arch"); DO_TEST("boot-cdrom"); DO_TEST("boot-network"); DO_TEST("boot-floppy"); diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 14743be..d067bca 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -354,6 +354,18 @@ virCapsPtr testQemuCapsInit(void) NULL) == NULL) goto cleanup; + if ((machines = testQemuAllocMachines(&nmachines)) == NULL) + goto cleanup; + + if (virCapabilitiesAddGuestDomain(guest, + VIR_DOMAIN_VIRT_KVM, + "/usr/bin/qemu-kvm", + NULL, + nmachines, + machines) == NULL) + goto cleanup; + machines = NULL; + if ((machines = testQemuAllocNewerMachines(&nmachines)) == NULL) goto cleanup; -- 2.4.0
On Wed, May 06, 2015 at 06:59:46PM -0400, Cole Robinson wrote:
My commit 747761a79 (v1.2.15 only) dropped this bit of logic when filling in a default arch in the XML:
- /* First try to find one matching host arch */ - for (i = 0; i < caps->nguests; i++) { - if (caps->guests[i]->ostype == ostype) { - for (j = 0; j < caps->guests[i]->arch.ndomains; j++) { - if (caps->guests[i]->arch.domains[j]->type == domain && - caps->guests[i]->arch.id == caps->host.arch) - return caps->guests[i]->arch.id; - } - } - }
That attempt to match host.arch is important, otherwise we end up defaulting to i686 on x86_64 host for KVM, which is not intended. Duplicate it in the centralized CapsLookup function.
Additionally add some testcases that would have caught this.
The patch Works for me, so: Tested-by: Richard W.M. Jones <rjones@redhat.com> I would really appreciate this patch to be added urgently to RHEL 7.2 builds, as this bug currently prevents all building and testing of virt-v2v. Rich.
src/conf/capabilities.c | 63 +++++++++++++++------- .../qemuxml2argv-default-kvm-host-arch.args | 4 ++ .../qemuxml2argv-default-kvm-host-arch.xml | 11 ++++ tests/qemuxml2argvtest.c | 1 + .../qemuxml2xmlout-default-kvm-host-arch.xml | 21 ++++++++ tests/qemuxml2xmltest.c | 1 + tests/testutilsqemu.c | 12 +++++ 7 files changed, 94 insertions(+), 19 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-default-kvm-host-arch.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-default-kvm-host-arch.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-default-kvm-host-arch.xml
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 922741f..c43bfb3 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -607,25 +607,13 @@ virCapsDomainDataCompare(virCapsGuestPtr guest, return true; }
-/** - * virCapabilitiesDomainDataLookup: - * @caps: capabilities to query - * @ostype: guest operating system type, of enum VIR_DOMAIN_OSTYPE - * @arch: Architecture to search for - * @domaintype: domain type to search for, of enum VIR_DOMAIN_VIRT - * @emulator: Emulator path to search for - * @machinetype: Machine type to search for - * - * Search capabilities for the passed values, and if found return - * virCapabilitiesDomainDataLookup filled in with the default values - */ -virCapsDomainDataPtr -virCapabilitiesDomainDataLookup(virCapsPtr caps, - int ostype, - virArch arch, - int domaintype, - const char *emulator, - const char *machinetype) +static virCapsDomainDataPtr +virCapabilitiesDomainDataLookupInternal(virCapsPtr caps, + int ostype, + virArch arch, + int domaintype, + const char *emulator, + const char *machinetype) { virCapsGuestPtr foundguest = NULL; virCapsGuestDomainPtr founddomain = NULL; @@ -730,6 +718,43 @@ virCapabilitiesDomainDataLookup(virCapsPtr caps, return ret; }
+/** + * virCapabilitiesDomainDataLookup: + * @caps: capabilities to query + * @ostype: guest operating system type, of enum VIR_DOMAIN_OSTYPE + * @arch: Architecture to search for + * @domaintype: domain type to search for, of enum VIR_DOMAIN_VIRT + * @emulator: Emulator path to search for + * @machinetype: Machine type to search for + * + * Search capabilities for the passed values, and if found return + * virCapabilitiesDomainDataLookup filled in with the default values + */ +virCapsDomainDataPtr +virCapabilitiesDomainDataLookup(virCapsPtr caps, + int ostype, + virArch arch, + int domaintype, + const char *emulator, + const char *machinetype) +{ + virCapsDomainDataPtr ret; + + if (arch == VIR_ARCH_NONE) { + /* Prefer host arch if its available */ + ret = virCapabilitiesDomainDataLookupInternal(caps, ostype, + caps->host.arch, + domaintype, + emulator, machinetype); + if (ret) + return ret; + } + + return virCapabilitiesDomainDataLookupInternal(caps, ostype, + arch, domaintype, + emulator, machinetype); +} + static int virCapabilitiesFormatNUMATopology(virBufferPtr buf, size_t ncells, diff --git a/tests/qemuxml2argvdata/qemuxml2argv-default-kvm-host-arch.args b/tests/qemuxml2argvdata/qemuxml2argv-default-kvm-host-arch.args new file mode 100644 index 0000000..102691f --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-default-kvm-host-arch.args @@ -0,0 +1,4 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/kvm -S -machine pc,accel=kvm -m 4096 -smp 4 -nographic \ +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -net none \ +-serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-default-kvm-host-arch.xml b/tests/qemuxml2argvdata/qemuxml2argv-default-kvm-host-arch.xml new file mode 100644 index 0000000..66dead0 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-default-kvm-host-arch.xml @@ -0,0 +1,11 @@ +<domain type='kvm'> + <name>kvm</name> + <uuid>d091ea82-29e6-2e34-3005-f02617b36e87</uuid> + <memory unit='KiB'>4194304</memory> + <currentMemory unit='KiB'>4194304</currentMemory> + <vcpu placement='static'>4</vcpu> + <os> + <type>hvm</type> + <boot dev='hd'/> + </os> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 97c7fba..b27e3a4 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -599,6 +599,7 @@ mymain(void) DO_TEST_FAILURE("machine-xen-vmport-opt", QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_MACHINE_VMPORT_OPT); DO_TEST("kvm", QEMU_CAPS_MACHINE_OPT); + DO_TEST("default-kvm-host-arch", QEMU_CAPS_MACHINE_OPT); DO_TEST("boot-cdrom", NONE); DO_TEST("boot-network", NONE); DO_TEST("boot-floppy", NONE); diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-default-kvm-host-arch.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-default-kvm-host-arch.xml new file mode 100644 index 0000000..30fa66d --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-default-kvm-host-arch.xml @@ -0,0 +1,21 @@ +<domain type='kvm'> + <name>kvm</name> + <uuid>d091ea82-29e6-2e34-3005-f02617b36e87</uuid> + <memory unit='KiB'>4194304</memory> + <currentMemory unit='KiB'>4194304</currentMemory> + <vcpu placement='static'>4</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/kvm</emulator> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index b611afd..a21a299 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -346,6 +346,7 @@ mymain(void) DO_TEST("minimal"); DO_TEST("machine-core-on"); DO_TEST("machine-core-off"); + DO_TEST_DIFFERENT("default-kvm-host-arch"); DO_TEST("boot-cdrom"); DO_TEST("boot-network"); DO_TEST("boot-floppy"); diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 14743be..d067bca 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -354,6 +354,18 @@ virCapsPtr testQemuCapsInit(void) NULL) == NULL) goto cleanup;
+ if ((machines = testQemuAllocMachines(&nmachines)) == NULL) + goto cleanup; + + if (virCapabilitiesAddGuestDomain(guest, + VIR_DOMAIN_VIRT_KVM, + "/usr/bin/qemu-kvm", + NULL, + nmachines, + machines) == NULL) + goto cleanup; + machines = NULL; + if ((machines = testQemuAllocNewerMachines(&nmachines)) == NULL) goto cleanup;
-- 2.4.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
On Wed, May 06, 2015 at 06:59:46PM -0400, Cole Robinson wrote:
My commit 747761a79 (v1.2.15 only) dropped this bit of logic when filling in a default arch in the XML:
- /* First try to find one matching host arch */ - for (i = 0; i < caps->nguests; i++) { - if (caps->guests[i]->ostype == ostype) { - for (j = 0; j < caps->guests[i]->arch.ndomains; j++) { - if (caps->guests[i]->arch.domains[j]->type == domain && - caps->guests[i]->arch.id == caps->host.arch) - return caps->guests[i]->arch.id; - } - } - }
That attempt to match host.arch is important, otherwise we end up defaulting to i686 on x86_64 host for KVM, which is not intended. Duplicate it in the centralized CapsLookup function.
This isn't really anything todo with KVM - it is a more general requirement. If no architecture is given in the XML, we must always default to the host architecture, whether using QEMU TCG or KVM. It looks like your code handles this fine, but the test case you added is only checking kvm, so might be nice to also validate the QEMU case too. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Richard W.M. Jones