Re: [libvirt] [PATCH 08/11] qemu_command: cleanup qemuBuildVideoCommandLine
On 09/30/2016 12:02 PM, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_command.c | 69 ++++++++++++++++++++++++++++---------------------
1 file changed, 40 insertions(+), 29 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index aef8c79..15bb381 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4451,43 +4451,54 @@ qemuBuildVideoCommandLine(virCommandPtr cmd,
virQEMUCapsPtr qemuCaps)
{
size_t i;
- int primaryVideoType;
+ char *str = NULL;
- if (!def->videos)
- return 0;
+ for (i = 0; i < def->nvideos; i++) {
+ virDomainVideoDefPtr video = def->videos[i];
- primaryVideoType = def->videos[0]->type;
+ switch (video->type) {
+ case VIR_DOMAIN_VIDEO_TYPE_VGA:
+ case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
+ case VIR_DOMAIN_VIDEO_TYPE_VMVGA:
+ case VIR_DOMAIN_VIDEO_TYPE_VIRTIO:
+ case VIR_DOMAIN_VIDEO_TYPE_QXL:
+ if (video->primary) {
So thinking back to my patch 5 comment - this essentially assumes
def->videos[0] and video->primary are equivalent, right?
+ if (virQEMUCapsGet(qemuCaps,
QEMU_CAPS_DEVICE_VIDEO_PRIMARY)) {
- if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY)) {
- for (i = 0; i < def->nvideos; i++) {
- char *str;
- virCommandAddArg(cmd, "-device");
- if (!(str = qemuBuildDeviceVideoStr(def, def->videos[i],
- qemuCaps)))
- return -1;
+ virCommandAddArg(cmd, "-device");
- virCommandAddArg(cmd, str);
- VIR_FREE(str);
- }
- } else {
- if (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_XEN) {
- /* nothing - vga has no effect on Xen pvfb */
- } else {
- if (qemuBuildVgaVideoCommand(cmd, def, qemuCaps) < 0)
- return -1;
- }
+ if (!(str = qemuBuildDeviceVideoStr(def, def->videos[i],
This would be "video", not the def->videos[i]
+ qemuCaps)))
+ return -1;
+
+ virCommandAddArg(cmd, str);
+ VIR_FREE(str);
+ } else {
+ if (qemuBuildVgaVideoCommand(cmd, def, qemuCaps) < 0)
And from the previous patch passing video here still works
+ return -1;
+ }
+ } else {
+ virCommandAddArg(cmd, "-device");
- for (i = 1; i < def->nvideos; i++) {
- char *str;
+ if (!(str = qemuBuildDeviceVideoStr(def, def->videos[i],
Likewise "video" not the def->videos[i]
+ qemuCaps)))
+ return -1;
- virCommandAddArg(cmd, "-device");
+ virCommandAddArg(cmd, str);
+ VIR_FREE(str);
+ }
+ break;
- if (!(str = qemuBuildDeviceVideoStr(def, def->videos[i],
- qemuCaps)))
- return -1;
+ case VIR_DOMAIN_VIDEO_TYPE_VBOX:
+ case VIR_DOMAIN_VIDEO_TYPE_XEN:
Doesn't putting this here break the previous else check of:
- if (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_XEN) {
- /* nothing - vga has no effect on Xen pvfb */
IOW: Shouldn't the XEN type code check code "skip" def->videos[0] (or
video->primary)?
ACK with the proper usage of video not def->videos[i] and making sure
the _XEN case is handled properly
John
+ case VIR_DOMAIN_VIDEO_TYPE_PARALLELS:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("video type '%s' is not supported with
QEMU"),
+ virDomainVideoTypeToString(video->type));
+ return -1;
- virCommandAddArg(cmd, str);
- VIR_FREE(str);
+ case VIR_DOMAIN_VIDEO_TYPE_LAST:
+ break;
}
}