[libvirt] [PATCH v3 0/4] qemu: fix type of default video device
This new version mostly integrates Cole's comments about the second version. Refactoring and behaviour change are now separate commits. Tests succeed for every individual patch in the series. Pavel Mores (4): qemu: default video device type selection algoritm moved into its own function qemu: prepare existing test for change of the default video device type qemu: the actual change of default video devide type selection algorithm qemu: added tests of the new default video type selection algorithm src/qemu/qemu_domain.c | 37 ++++++++++------ .../default-video-type-aarch64.xml | 16 +++++++ .../default-video-type-ppc64.xml | 16 +++++++ .../default-video-type-riscv64.xml | 16 +++++++ .../default-video-type-s390x.xml | 16 +++++++ .../default-video-type-x86_64-caps-test-0.xml | 17 ++++++++ .../default-video-type-x86_64-caps-test-1.xml | 17 ++++++++ tests/qemuxml2argvtest.c | 1 + ...ault-video-type-aarch64.aarch64-latest.xml | 42 +++++++++++++++++++ .../default-video-type-ppc64.ppc64-latest.xml | 31 ++++++++++++++ ...ault-video-type-riscv64.riscv64-latest.xml | 39 +++++++++++++++++ .../default-video-type-s390x.s390x-latest.xml | 32 ++++++++++++++ .../default-video-type-x86_64-caps-test-0.xml | 31 ++++++++++++++ .../default-video-type-x86_64-caps-test-1.xml | 31 ++++++++++++++ tests/qemuxml2xmltest.c | 10 ++++- 15 files changed, 339 insertions(+), 13 deletions(-) create mode 100644 tests/qemuxml2argvdata/default-video-type-aarch64.xml create mode 100644 tests/qemuxml2argvdata/default-video-type-ppc64.xml create mode 100644 tests/qemuxml2argvdata/default-video-type-riscv64.xml create mode 100644 tests/qemuxml2argvdata/default-video-type-s390x.xml create mode 100644 tests/qemuxml2argvdata/default-video-type-x86_64-caps-test-0.xml create mode 100644 tests/qemuxml2argvdata/default-video-type-x86_64-caps-test-1.xml create mode 100644 tests/qemuxml2xmloutdata/default-video-type-aarch64.aarch64-latest.xml create mode 100644 tests/qemuxml2xmloutdata/default-video-type-ppc64.ppc64-latest.xml create mode 100644 tests/qemuxml2xmloutdata/default-video-type-riscv64.riscv64-latest.xml create mode 100644 tests/qemuxml2xmloutdata/default-video-type-s390x.s390x-latest.xml create mode 100644 tests/qemuxml2xmloutdata/default-video-type-x86_64-caps-test-0.xml create mode 100644 tests/qemuxml2xmloutdata/default-video-type-x86_64-caps-test-1.xml -- 2.21.0
The default video device type selection algorithm we're about to deploy will increase the amount of code dedicated to the task by amount enough to warrant factoring the whole thing into its own function so as not to pollute the caller qemuDomainDeviceVideoDefPostParse(). Do it now so that the actual algorithm change later on is in a clean commit by itself and easy to review. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_domain.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d56dfa9f00..26da41f9ee 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7876,20 +7876,26 @@ qemuDomainDeviceNetDefPostParse(virDomainNetDefPtr net, } +static int +qemuDomainDefaultVideoDevice(const virDomainDef *def) +{ + if (ARCH_IS_PPC64(def->os.arch)) + return VIR_DOMAIN_VIDEO_TYPE_VGA; + else if (qemuDomainIsARMVirt(def) || + qemuDomainIsRISCVVirt(def) || + ARCH_IS_S390(def->os.arch)) + return VIR_DOMAIN_VIDEO_TYPE_VIRTIO; + else + return VIR_DOMAIN_VIDEO_TYPE_CIRRUS; +} + + static int qemuDomainDeviceVideoDefPostParse(virDomainVideoDefPtr video, const virDomainDef *def) { - if (video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT) { - if (ARCH_IS_PPC64(def->os.arch)) - video->type = VIR_DOMAIN_VIDEO_TYPE_VGA; - else if (qemuDomainIsARMVirt(def) || - qemuDomainIsRISCVVirt(def) || - ARCH_IS_S390(def->os.arch)) - video->type = VIR_DOMAIN_VIDEO_TYPE_VIRTIO; - else - video->type = VIR_DOMAIN_VIDEO_TYPE_CIRRUS; - } + if (video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT) + video->type = qemuDomainDefaultVideoDevice(def); if (video->type == VIR_DOMAIN_VIDEO_TYPE_QXL && !video->vgamem) { -- 2.21.0
The test relied implicitly on default video device being cirrus. As we're about to change that the test would start failing. To avoid this, just make the test's requirement explicit. Signed-off-by: Pavel Mores <pmores@redhat.com> --- tests/qemuxml2xmltest.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index ed53ddc8c6..2c61d7db8a 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -372,7 +372,8 @@ mymain(void) DO_TEST("graphics-egl-headless-rendernode", QEMU_CAPS_DEVICE_CIRRUS_VGA, QEMU_CAPS_EGL_HEADLESS, - QEMU_CAPS_EGL_HEADLESS_RENDERNODE); + QEMU_CAPS_EGL_HEADLESS_RENDERNODE, + QEMU_CAPS_DEVICE_CIRRUS_VGA); DO_TEST("input-usbmouse", NONE); DO_TEST("input-usbtablet", NONE); -- 2.21.0
If a graphics device was added to XML that had no video device, libvirt automatically added a video device which was always of type 'cirrus' on x86_64, even if the underlying qemu didn't support cirrus. This patch refines a bit the decision about the type of the video device. Based on QEMU capabilities, cirrus is still preferred but only added if QEMU supports it, otherwise VGA is used if supported by QEMU. There is now no fallback as libvirt only aspires to generate a basic working config and leaves anything more specific up to higher-level management tools. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_domain.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 26da41f9ee..18b6f1ea1c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7877,25 +7877,32 @@ qemuDomainDeviceNetDefPostParse(virDomainNetDefPtr net, static int -qemuDomainDefaultVideoDevice(const virDomainDef *def) +qemuDomainDefaultVideoDevice(const virDomainDef *def, + virQEMUCapsPtr qemuCaps) { - if (ARCH_IS_PPC64(def->os.arch)) + if (ARCH_IS_PPC64(def->os.arch)) { return VIR_DOMAIN_VIDEO_TYPE_VGA; - else if (qemuDomainIsARMVirt(def) || - qemuDomainIsRISCVVirt(def) || - ARCH_IS_S390(def->os.arch)) + } else if (qemuDomainIsARMVirt(def) || + qemuDomainIsRISCVVirt(def) || + ARCH_IS_S390(def->os.arch)) { return VIR_DOMAIN_VIDEO_TYPE_VIRTIO; - else - return VIR_DOMAIN_VIDEO_TYPE_CIRRUS; + } else { + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_CIRRUS_VGA)) + return VIR_DOMAIN_VIDEO_TYPE_CIRRUS; + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VGA)) + return VIR_DOMAIN_VIDEO_TYPE_VGA; + return VIR_DOMAIN_VIDEO_TYPE_DEFAULT; + } } static int qemuDomainDeviceVideoDefPostParse(virDomainVideoDefPtr video, - const virDomainDef *def) + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) { if (video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT) - video->type = qemuDomainDefaultVideoDevice(def); + video->type = qemuDomainDefaultVideoDevice(def, qemuCaps); if (video->type == VIR_DOMAIN_VIDEO_TYPE_QXL && !video->vgamem) { @@ -7989,7 +7996,7 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, break; case VIR_DOMAIN_DEVICE_VIDEO: - ret = qemuDomainDeviceVideoDefPostParse(dev->data.video, def); + ret = qemuDomainDeviceVideoDefPostParse(dev->data.video, def, qemuCaps); break; case VIR_DOMAIN_DEVICE_PANIC: -- 2.21.0
The test case for x86_64 and neither cirrus nor vga capability is of the xml2argv type because it actually fails to parse the XML at all [*] which is something that xml2xml tests don't seem to handle. xml2argv test fails to produce a qemu argv for this case which xml2argv tests can handle. [*] This is a consequence of the decision not to have a fallback if the obvious choices (cirrus and vga) aren't viable due to missing QEMU caps. Signed-off-by: Pavel Mores <pmores@redhat.com> --- .../default-video-type-aarch64.xml | 16 +++++++ .../default-video-type-ppc64.xml | 16 +++++++ .../default-video-type-riscv64.xml | 16 +++++++ .../default-video-type-s390x.xml | 16 +++++++ .../default-video-type-x86_64-caps-test-0.xml | 17 ++++++++ .../default-video-type-x86_64-caps-test-1.xml | 17 ++++++++ tests/qemuxml2argvtest.c | 1 + ...ault-video-type-aarch64.aarch64-latest.xml | 42 +++++++++++++++++++ .../default-video-type-ppc64.ppc64-latest.xml | 31 ++++++++++++++ ...ault-video-type-riscv64.riscv64-latest.xml | 39 +++++++++++++++++ .../default-video-type-s390x.s390x-latest.xml | 32 ++++++++++++++ .../default-video-type-x86_64-caps-test-0.xml | 31 ++++++++++++++ .../default-video-type-x86_64-caps-test-1.xml | 31 ++++++++++++++ tests/qemuxml2xmltest.c | 7 ++++ 14 files changed, 312 insertions(+) create mode 100644 tests/qemuxml2argvdata/default-video-type-aarch64.xml create mode 100644 tests/qemuxml2argvdata/default-video-type-ppc64.xml create mode 100644 tests/qemuxml2argvdata/default-video-type-riscv64.xml create mode 100644 tests/qemuxml2argvdata/default-video-type-s390x.xml create mode 100644 tests/qemuxml2argvdata/default-video-type-x86_64-caps-test-0.xml create mode 100644 tests/qemuxml2argvdata/default-video-type-x86_64-caps-test-1.xml create mode 100644 tests/qemuxml2xmloutdata/default-video-type-aarch64.aarch64-latest.xml create mode 100644 tests/qemuxml2xmloutdata/default-video-type-ppc64.ppc64-latest.xml create mode 100644 tests/qemuxml2xmloutdata/default-video-type-riscv64.riscv64-latest.xml create mode 100644 tests/qemuxml2xmloutdata/default-video-type-s390x.s390x-latest.xml create mode 100644 tests/qemuxml2xmloutdata/default-video-type-x86_64-caps-test-0.xml create mode 100644 tests/qemuxml2xmloutdata/default-video-type-x86_64-caps-test-1.xml diff --git a/tests/qemuxml2argvdata/default-video-type-aarch64.xml b/tests/qemuxml2argvdata/default-video-type-aarch64.xml new file mode 100644 index 0000000000..03326d3c9b --- /dev/null +++ b/tests/qemuxml2argvdata/default-video-type-aarch64.xml @@ -0,0 +1,16 @@ +<domain type='kvm'> + <name>default-video-type-aarch64-test</name> + <uuid>6ba410c5-1e5c-4d57-bee7-2228e7ffa32f</uuid> + <memory unit='KiB'>1048576</memory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='aarch64' machine='virt'>hvm</type> + </os> + <cpu mode='host-passthrough'/> + <devices> + <emulator>/usr/bin/qemu-system-aarch64</emulator> + <controller type='usb' index='0' model='none'/> + <memballoon model='none'/> + <graphics type='spice'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/default-video-type-ppc64.xml b/tests/qemuxml2argvdata/default-video-type-ppc64.xml new file mode 100644 index 0000000000..739e07fc19 --- /dev/null +++ b/tests/qemuxml2argvdata/default-video-type-ppc64.xml @@ -0,0 +1,16 @@ +<domain type='kvm'> + <name>default-video-type-ppc64-test</name> + <uuid>6ba410c5-1e5c-4d57-bee7-2228e7ffa32f</uuid> + <memory unit='KiB'>1048576</memory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='virt'>hvm</type> + </os> + <cpu mode='host-passthrough'/> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <controller type='usb' index='0' model='none'/> + <memballoon model='none'/> + <graphics type='spice'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/default-video-type-riscv64.xml b/tests/qemuxml2argvdata/default-video-type-riscv64.xml new file mode 100644 index 0000000000..55f6fa9391 --- /dev/null +++ b/tests/qemuxml2argvdata/default-video-type-riscv64.xml @@ -0,0 +1,16 @@ +<domain type='qemu'> + <name>default-video-type-riscv64-test</name> + <uuid>6ba410c5-1e5c-4d57-bee7-2228e7ffa32f</uuid> + <memory unit='KiB'>1048576</memory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='riscv64' machine='virt'>hvm</type> + </os> + <cpu mode='host-passthrough'/> + <devices> + <emulator>/usr/bin/qemu-system-riscv64</emulator> + <controller type='usb' index='0' model='none'/> + <memballoon model='none'/> + <graphics type='spice'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/default-video-type-s390x.xml b/tests/qemuxml2argvdata/default-video-type-s390x.xml new file mode 100644 index 0000000000..9eda06a3a1 --- /dev/null +++ b/tests/qemuxml2argvdata/default-video-type-s390x.xml @@ -0,0 +1,16 @@ +<domain type='kvm'> + <name>default-video-type-s390x-test</name> + <uuid>6ba410c5-1e5c-4d57-bee7-2228e7ffa32f</uuid> + <memory unit='KiB'>1048576</memory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='s390x' machine='virt'>hvm</type> + </os> + <cpu mode='host-passthrough'/> + <devices> + <emulator>/usr/bin/qemu-system-s390x</emulator> + <controller type='usb' index='0' model='none'/> + <memballoon model='none'/> + <graphics type='spice'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/default-video-type-x86_64-caps-test-0.xml b/tests/qemuxml2argvdata/default-video-type-x86_64-caps-test-0.xml new file mode 100644 index 0000000000..2c753fe227 --- /dev/null +++ b/tests/qemuxml2argvdata/default-video-type-x86_64-caps-test-0.xml @@ -0,0 +1,17 @@ +<domain type='kvm'> + <name>default-video-type-x86_64-test</name> + <uuid>6ba410c5-1e5c-4d57-bee7-2228e7ffa32f</uuid> + <memory unit='KiB'>1048576</memory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='virt'>hvm</type> + </os> + <cpu mode='host-passthrough'/> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='none'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='none'/> + <graphics type='spice'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/default-video-type-x86_64-caps-test-1.xml b/tests/qemuxml2argvdata/default-video-type-x86_64-caps-test-1.xml new file mode 100644 index 0000000000..2c753fe227 --- /dev/null +++ b/tests/qemuxml2argvdata/default-video-type-x86_64-caps-test-1.xml @@ -0,0 +1,17 @@ +<domain type='kvm'> + <name>default-video-type-x86_64-test</name> + <uuid>6ba410c5-1e5c-4d57-bee7-2228e7ffa32f</uuid> + <memory unit='KiB'>1048576</memory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='virt'>hvm</type> + </os> + <cpu mode='host-passthrough'/> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='none'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='none'/> + <graphics type='spice'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 07e711840d..a2791d0460 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2059,6 +2059,7 @@ mymain(void) DO_TEST("video-none-device", QEMU_CAPS_VNC); DO_TEST_PARSE_ERROR("video-invalid-multiple-devices", NONE); + DO_TEST_PARSE_ERROR("default-video-type-x86_64-caps-test-0", NONE); DO_TEST("virtio-rng-default", QEMU_CAPS_DEVICE_VIRTIO_RNG, diff --git a/tests/qemuxml2xmloutdata/default-video-type-aarch64.aarch64-latest.xml b/tests/qemuxml2xmloutdata/default-video-type-aarch64.aarch64-latest.xml new file mode 100644 index 0000000000..4b660b8d70 --- /dev/null +++ b/tests/qemuxml2xmloutdata/default-video-type-aarch64.aarch64-latest.xml @@ -0,0 +1,42 @@ +<domain type='kvm'> + <name>default-video-type-aarch64-test</name> + <uuid>6ba410c5-1e5c-4d57-bee7-2228e7ffa32f</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='aarch64' machine='virt'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <gic version='3'/> + </features> + <cpu mode='host-passthrough' check='none'/> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-aarch64</emulator> + <controller type='usb' index='0' model='none'/> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='1' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' multifunction='on'/> + </controller> + <controller type='pci' index='2' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='2' port='0x9'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <graphics type='spice'> + <listen type='none'/> + </graphics> + <video> + <model type='virtio' heads='1' primary='yes'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/default-video-type-ppc64.ppc64-latest.xml b/tests/qemuxml2xmloutdata/default-video-type-ppc64.ppc64-latest.xml new file mode 100644 index 0000000000..590a73b456 --- /dev/null +++ b/tests/qemuxml2xmloutdata/default-video-type-ppc64.ppc64-latest.xml @@ -0,0 +1,31 @@ +<domain type='kvm'> + <name>default-video-type-ppc64-test</name> + <uuid>6ba410c5-1e5c-4d57-bee7-2228e7ffa32f</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='virt'>hvm</type> + <boot dev='hd'/> + </os> + <cpu mode='host-passthrough' check='none'/> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <controller type='usb' index='0' model='none'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='keyboard' bus='usb'/> + <input type='mouse' bus='usb'/> + <graphics type='spice'> + <listen type='none'/> + </graphics> + <video> + <model type='vga' vram='16384' heads='1' primary='yes'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/default-video-type-riscv64.riscv64-latest.xml b/tests/qemuxml2xmloutdata/default-video-type-riscv64.riscv64-latest.xml new file mode 100644 index 0000000000..ebb3bfe980 --- /dev/null +++ b/tests/qemuxml2xmloutdata/default-video-type-riscv64.riscv64-latest.xml @@ -0,0 +1,39 @@ +<domain type='qemu'> + <name>default-video-type-riscv64-test</name> + <uuid>6ba410c5-1e5c-4d57-bee7-2228e7ffa32f</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='riscv64' machine='virt'>hvm</type> + <boot dev='hd'/> + </os> + <cpu mode='host-passthrough' check='none'/> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-riscv64</emulator> + <controller type='usb' index='0' model='none'/> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='1' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0' multifunction='on'/> + </controller> + <controller type='pci' index='2' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='2' port='0x9'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <graphics type='spice'> + <listen type='none'/> + </graphics> + <video> + <model type='virtio' heads='1' primary='yes'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/default-video-type-s390x.s390x-latest.xml b/tests/qemuxml2xmloutdata/default-video-type-s390x.s390x-latest.xml new file mode 100644 index 0000000000..21718db1ca --- /dev/null +++ b/tests/qemuxml2xmloutdata/default-video-type-s390x.s390x-latest.xml @@ -0,0 +1,32 @@ +<domain type='kvm'> + <name>default-video-type-s390x-test</name> + <uuid>6ba410c5-1e5c-4d57-bee7-2228e7ffa32f</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='s390x' machine='virt'>hvm</type> + <boot dev='hd'/> + </os> + <cpu mode='host-passthrough' check='none'/> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-s390x</emulator> + <controller type='usb' index='0' model='none'/> + <controller type='pci' index='0' model='pci-root'/> + <graphics type='spice'> + <listen type='none'/> + </graphics> + <video> + <model type='virtio' heads='1' primary='yes'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'> + <zpci uid='0x0001' fid='0x00000000'/> + </address> + </video> + <memballoon model='none'/> + <panic model='s390'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/default-video-type-x86_64-caps-test-0.xml b/tests/qemuxml2xmloutdata/default-video-type-x86_64-caps-test-0.xml new file mode 100644 index 0000000000..645019c230 --- /dev/null +++ b/tests/qemuxml2xmloutdata/default-video-type-x86_64-caps-test-0.xml @@ -0,0 +1,31 @@ +<domain type='kvm'> + <name>default-video-type-x86_64-test</name> + <uuid>6ba410c5-1e5c-4d57-bee7-2228e7ffa32f</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='virt'>hvm</type> + <boot dev='hd'/> + </os> + <cpu mode='host-passthrough' check='none'/> + <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' model='none'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='spice'> + <listen type='none'/> + </graphics> + <video> + <model type='vga' vram='16384' heads='1' primary='yes'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/default-video-type-x86_64-caps-test-1.xml b/tests/qemuxml2xmloutdata/default-video-type-x86_64-caps-test-1.xml new file mode 100644 index 0000000000..f763b6902e --- /dev/null +++ b/tests/qemuxml2xmloutdata/default-video-type-x86_64-caps-test-1.xml @@ -0,0 +1,31 @@ +<domain type='kvm'> + <name>default-video-type-x86_64-test</name> + <uuid>6ba410c5-1e5c-4d57-bee7-2228e7ffa32f</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='virt'>hvm</type> + <boot dev='hd'/> + </os> + <cpu mode='host-passthrough' check='none'/> + <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' model='none'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='spice'> + <listen type='none'/> + </graphics> + <video> + <model type='cirrus' vram='16384' heads='1' primary='yes'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </video> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 2c61d7db8a..8b43f35f06 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -355,6 +355,13 @@ mymain(void) DO_TEST("graphics-vnc-egl-headless", QEMU_CAPS_DEVICE_CIRRUS_VGA, QEMU_CAPS_EGL_HEADLESS); + DO_TEST_CAPS_ARCH_LATEST("default-video-type-aarch64", "aarch64"); + DO_TEST_CAPS_ARCH_LATEST("default-video-type-ppc64", "ppc64"); + DO_TEST_CAPS_ARCH_LATEST("default-video-type-riscv64", "riscv64"); + DO_TEST_CAPS_ARCH_LATEST("default-video-type-s390x", "s390x"); + DO_TEST("default-video-type-x86_64-caps-test-0", QEMU_CAPS_DEVICE_VGA); + DO_TEST("default-video-type-x86_64-caps-test-1", QEMU_CAPS_DEVICE_CIRRUS_VGA); + DO_TEST("graphics-sdl", QEMU_CAPS_DEVICE_VGA); DO_TEST("graphics-sdl-fullscreen", QEMU_CAPS_DEVICE_CIRRUS_VGA); DO_TEST("graphics-spice", QEMU_CAPS_DEVICE_QXL); -- 2.21.0
On 11/25/19 5:54 AM, Pavel Mores wrote:
This new version mostly integrates Cole's comments about the second version. Refactoring and behaviour change are now separate commits. Tests succeed for every individual patch in the series.
Pavel Mores (4): qemu: default video device type selection algoritm moved into its own function qemu: prepare existing test for change of the default video device type qemu: the actual change of default video devide type selection algorithm qemu: added tests of the new default video type selection algorithm
Reviewed-by: Cole Robinson <crobinso@redhat.com> and pushed now. patch #3 still had some style issues I pointed out in the previous review: using 'else' when it isn't necessary, because every block returns. Better to use the style of qemuDomainDefaultNetModel which saves having to do any nested logic. I'll be happy to review a follow up cleanup patch for that Minor bit: I would have squashed patch #2 + #3 together, maybe only separating them if there was lots of test suite churn needed, but I'm not sure if others agree. Thanks, Cole
On Mon, Nov 25, 2019 at 08:54:43AM -0500, Cole Robinson wrote:
On 11/25/19 5:54 AM, Pavel Mores wrote:
This new version mostly integrates Cole's comments about the second version. Refactoring and behaviour change are now separate commits. Tests succeed for every individual patch in the series.
Pavel Mores (4): qemu: default video device type selection algoritm moved into its own function qemu: prepare existing test for change of the default video device type qemu: the actual change of default video devide type selection algorithm qemu: added tests of the new default video type selection algorithm
Reviewed-by: Cole Robinson <crobinso@redhat.com>
and pushed now.
patch #3 still had some style issues I pointed out in the previous review: using 'else' when it isn't necessary, because every block returns. Better to use the style of qemuDomainDefaultNetModel which saves having to do any nested logic. I'll be happy to review a follow up cleanup patch for that
Oh, now I see I changed my own additions but overlooked the style of pre-existing code. Sorry about that - I guess I don't have an eye for this as personally I'd probably slightly prefer the else-based style.
Minor bit: I would have squashed patch #2 + #3 together, maybe only separating them if there was lots of test suite churn needed, but I'm not sure if others agree.
Thanks, Cole
On 11/25/19 9:40 AM, Pavel Mores wrote:
On Mon, Nov 25, 2019 at 08:54:43AM -0500, Cole Robinson wrote:
On 11/25/19 5:54 AM, Pavel Mores wrote:
This new version mostly integrates Cole's comments about the second version. Refactoring and behaviour change are now separate commits. Tests succeed for every individual patch in the series.
Pavel Mores (4): qemu: default video device type selection algoritm moved into its own function qemu: prepare existing test for change of the default video device type qemu: the actual change of default video devide type selection algorithm qemu: added tests of the new default video type selection algorithm
Reviewed-by: Cole Robinson <crobinso@redhat.com>
and pushed now.
patch #3 still had some style issues I pointed out in the previous review: using 'else' when it isn't necessary, because every block returns. Better to use the style of qemuDomainDefaultNetModel which saves having to do any nested logic. I'll be happy to review a follow up cleanup patch for that
Oh, now I see I changed my own additions but overlooked the style of pre-existing code. Sorry about that - I guess I don't have an eye for this as personally I'd probably slightly prefer the else-based style.
The main issue with the code as implemented is that the third block is indented under an 'else' when it doesn't need to be. If we followed that pattern to the extreme the code would look like if (condition1) { return foo; } else { if (condition2) { return bar; } else { if (condition3) return baz; } } instead of if (condition1) return foo; if (condition2) return bar; if (condition3) return baz; Thanks, Cole
On Mon, Nov 25, 2019 at 10:04:58AM -0500, Cole Robinson wrote:
On 11/25/19 9:40 AM, Pavel Mores wrote:
On Mon, Nov 25, 2019 at 08:54:43AM -0500, Cole Robinson wrote:
On 11/25/19 5:54 AM, Pavel Mores wrote:
This new version mostly integrates Cole's comments about the second version. Refactoring and behaviour change are now separate commits. Tests succeed for every individual patch in the series.
Pavel Mores (4): qemu: default video device type selection algoritm moved into its own function qemu: prepare existing test for change of the default video device type qemu: the actual change of default video devide type selection algorithm qemu: added tests of the new default video type selection algorithm
Reviewed-by: Cole Robinson <crobinso@redhat.com>
and pushed now.
patch #3 still had some style issues I pointed out in the previous review: using 'else' when it isn't necessary, because every block returns. Better to use the style of qemuDomainDefaultNetModel which saves having to do any nested logic. I'll be happy to review a follow up cleanup patch for that
Oh, now I see I changed my own additions but overlooked the style of pre-existing code. Sorry about that - I guess I don't have an eye for this as personally I'd probably slightly prefer the else-based style.
The main issue with the code as implemented is that the third block is indented under an 'else' when it doesn't need to be. If we followed that pattern to the extreme the code would look like
if (condition1) { return foo; } else { if (condition2) { return bar; } else { if (condition3) return baz; } }
instead of
if (condition1) return foo; if (condition2) return bar; if (condition3) return baz;
I agree that the way the code is now, the flat style is more readable. I just feel that the additional structure provided by nesting could come handy if the algorithm has be changed again in a way that made it impossible to have just a return in every branch. If we can reasonably assume that that's not going to happen, and if the original author doesn't object (cc'ing him), I'm happy to make the change. Thanks, pvl
participants (2)
-
Cole Robinson -
Pavel Mores