Re: [libvirt] [PATCH v2 1/2] qemu: fix type of default video device
Sorry for the review delay. If you cc me on v3 I'll review it ASAP
This patch is mixing two things: refactoring, and behavior change. This
makes review more difficult. Please put the refactoring first which will
maintain the old behavior, then add functional changes as additional
patch(es).
On its own, this patch breaks the test suite. Every patch should be self
contained, keeping the test suite working.
On 10/18/19 6:27 AM, Pavel Mores wrote:
If a graphics device is added to XML that has 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1668141
Signed-off-by: Pavel Mores <pmores(a)redhat.com>
---
src/qemu/qemu_domain.c | 39 +++++++++++++++++++++++++++------------
1 file changed, 27 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index b257db44b0..39b2d2da82 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7360,6 +7360,28 @@ qemuDomainDefaultNetModel(const virDomainDef *def,
}
+static int
+qemuDomainDefaultVideoDevice(const virDomainDef *def,
+ virQEMUCapsPtr qemuCaps)
+{
+ 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)) {
Indentation is off here.
+ return VIR_DOMAIN_VIDEO_TYPE_VIRTIO;
+ } else {
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_CIRRUS_VGA)) {
+ return VIR_DOMAIN_VIDEO_TYPE_CIRRUS;
+ } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VGA)) {
+ return VIR_DOMAIN_VIDEO_TYPE_VGA;
+ } else {
+ return VIR_DOMAIN_VIDEO_TYPE_DEFAULT;
+ }
+ }
+}
+
You can remove any 'else' usage after 'return;' here. Every condition
will then be at the same indent level. Then you can drop all the bracket
usage too in accordance with the style guidelines. All this can be done
in the same patch creating the new function IMO.
Second patch can add qemuCaps up through the callstack and make the
behavior change.
The can probably resolve the test breakage by moving current patch #2 to
be first in the series. Then the behavior change patch will demonstrate
the new behavior in changed test suite output, which makes it easier to
review exactly the effect of the changes.
Thanks,
Cole
+
/*
* Clear auto generated unix socket paths:
*
@@ -7821,18 +7843,11 @@ qemuDomainDeviceNetDefPostParse(virDomainNetDefPtr net,
static int
qemuDomainDeviceVideoDefPostParse(virDomainVideoDefPtr video,
- const virDomainDef *def)
+ const virDomainDef *def,
+ virQEMUCapsPtr qemuCaps)
{
- 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, qemuCaps);
if (video->type == VIR_DOMAIN_VIDEO_TYPE_QXL &&
!video->vgamem) {
@@ -7926,7 +7941,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: