4:54 a.m.
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
4:54 a.m.
New subject: [libvirt] [PATCH v3 1/4] qemu: default video device type selection algoritm moved into its own function
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(a)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
4:54 a.m.
New subject: [libvirt] [PATCH v3 2/4] qemu: prepare existing test for change of the default video device type
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(a)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
4:54 a.m.
New subject: [libvirt] [PATCH v3 3/4] qemu: the actual change of default video devide type selection algorithm
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(a)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
4:54 a.m.
New subject: [libvirt] [PATCH v3 4/4] qemu: added tests of the new default video type selection algorithm
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(a)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
7:54 a.m.
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(a)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
8:40 a.m.
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(a)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
9:04 a.m.
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(a)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
9:42 a.m.
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(a)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
1824
days inactive
1824
days old
8 comments
2 participants
participants (2)
-
Cole Robinson -
Pavel Mores