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

Re: [libvirt] [PATCH v2] caps: Fix regression defaulting to host arch



On Thu, May 07, 2015 at 11:15:55AM -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.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1219191
> ---
> v2:
>     Tweak subject
>     add test case for type=qemu as well

I didn't specifically test this patch, but I compared it to the
previous patch and all it does is to add / change the test suite.
Since the code is the same as before, and I tested the code in V1:

ACK.

Rich.

>  src/conf/capabilities.c                            | 63 +++++++++++++++-------
>  .../qemuxml2argv-default-kvm-host-arch.args        |  4 ++
>  .../qemuxml2argv-default-kvm-host-arch.xml         | 11 ++++
>  .../qemuxml2argv-default-qemu-host-arch.args       |  4 ++
>  .../qemuxml2argv-default-qemu-host-arch.xml        | 11 ++++
>  tests/qemuxml2argvtest.c                           |  2 +
>  .../qemuxml2xmlout-default-kvm-host-arch.xml       | 21 ++++++++
>  .../qemuxml2xmlout-default-qemu-host-arch.xml      | 21 ++++++++
>  tests/qemuxml2xmltest.c                            |  2 +
>  tests/testutilsqemu.c                              | 12 +++++
>  10 files changed, 132 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/qemuxml2argvdata/qemuxml2argv-default-qemu-host-arch.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-default-qemu-host-arch.xml
>  create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-default-kvm-host-arch.xml
>  create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-default-qemu-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/qemuxml2argvdata/qemuxml2argv-default-qemu-host-arch.args b/tests/qemuxml2argvdata/qemuxml2argv-default-qemu-host-arch.args
> new file mode 100644
> index 0000000..5bd404c
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-default-qemu-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/qemu-system-x86_64 -S -machine pc-0.11,accel=tcg -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-qemu-host-arch.xml b/tests/qemuxml2argvdata/qemuxml2argv-default-qemu-host-arch.xml
> new file mode 100644
> index 0000000..85ddec5
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-default-qemu-host-arch.xml
> @@ -0,0 +1,11 @@
> +<domain type='qemu'>
> +  <name>qemu-host</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..d317c9b 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -599,6 +599,8 @@ 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("default-qemu-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/qemuxml2xmloutdata/qemuxml2xmlout-default-qemu-host-arch.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-default-qemu-host-arch.xml
> new file mode 100644
> index 0000000..3e65b97
> --- /dev/null
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-default-qemu-host-arch.xml
> @@ -0,0 +1,21 @@
> +<domain type='qemu'>
> +  <name>qemu-host</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-0.11'>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/qemu-system-x86_64</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..99f402c 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -346,6 +346,8 @@ mymain(void)
>      DO_TEST("minimal");
>      DO_TEST("machine-core-on");
>      DO_TEST("machine-core-off");
> +    DO_TEST_DIFFERENT("default-kvm-host-arch");
> +    DO_TEST_DIFFERENT("default-qemu-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

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW


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