On 09/30/2016 12:02 PM, Pavel Hrdina wrote:
> Runtime validation that depend on qemu capabilities should be moved
> into qemuProcessStartValidateXML.
>
> Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
> ---
> src/qemu/qemu_command.c | 33 +-------------------
> src/qemu/qemu_process.c | 50 ++++++++++++++++++++++++++++++-
> tests/qemuxml2argvtest.c | 78 +++++++++++++++++++++++++++++-------------------
> 3 files changed, 98 insertions(+), 63 deletions(-)
>
Could the qemuxml2argvtest.c to add QEMU_CAPS_DEVICE_CIRRUS_VGA move to
earlier? Perhaps there is a relationship, but it's not clear from the
commit message...
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 761968b..d283c67 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4312,26 +4312,12 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
> model = "virtio-gpu-pci";
> }
> } else {
> - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QXL)) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - "%s", _("only one video card is currently supported"));
> - goto error;
> - }
> -
Just checking - does the removal of this end up as part of the (now)
singular video loop in qemuProcessStartValidateVideo? Same for the
removed check in qemuBuildVideoCommandLine?
It wasn't "clear" the first pass through reading...
> model = "qxl";
> }
>
> virBufferAsprintf(&buf, "%s,id=%s", model, video->info.alias);
>
> if (video->accel && video->accel->accel3d == VIR_TRISTATE_SWITCH_ON) {
> - if (video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO ||
> - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL)) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("%s 3d acceleration is not supported"),
> - virDomainVideoTypeToString(video->type));
> - goto error;
> - }
> -
> virBufferAsprintf(&buf, ",virgl=%s",
> virTristateSwitchTypeToString(video->accel->accel3d));
> }
> @@ -4397,17 +4383,7 @@ qemuBuildVideoCommandLine(virCommandPtr cmd,
>
> primaryVideoType = def->videos[0]->type;
>
> - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY) &&
> - ((primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VGA &&
> - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VGA)) ||
> - (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_CIRRUS &&
> - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_CIRRUS_VGA)) ||
> - (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VMVGA &&
> - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMWARE_SVGA)) ||
> - (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_QXL &&
> - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QXL)) ||
> - (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VIRTIO &&
> - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU)))) {
> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY)) {
> for (i = 0; i < def->nvideos; i++) {
> char *str;
> virCommandAddArg(cmd, "-device");
> @@ -4422,13 +4398,6 @@ qemuBuildVideoCommandLine(virCommandPtr cmd,
> if (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_XEN) {
> /* nothing - vga has no effect on Xen pvfb */
> } else {
> - if ((primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_QXL) &&
> - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QXL)) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("This QEMU does not support QXL graphics adapters"));
> - return -1;
> - }
> -
... Is it safe to assume there can only be one video card here?
> const char *vgastr = qemuVideoTypeToString(primaryVideoType);
> if (!vgastr || STREQ(vgastr, "")) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 6ddcc1f..eb3add9 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -4448,6 +4448,48 @@ qemuProcessStartValidateGraphics(virDomainObjPtr vm)
>
>
> static int
> +qemuProcessStartValidateVideo(virDomainObjPtr vm,
> + virQEMUCapsPtr qemuCaps)
> +{
> + size_t i;
> + virDomainVideoDefPtr video;
> +
> + for (i = 0; i < vm->def->nvideos; i++) {
> + video = vm->def->videos[i];
> +
> + if ((video->type == VIR_DOMAIN_VIDEO_TYPE_VGA &&
> + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VGA)) ||
> + (video->type == VIR_DOMAIN_VIDEO_TYPE_CIRRUS &&
> + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_CIRRUS_VGA)) ||
> + (video->type == VIR_DOMAIN_VIDEO_TYPE_VMVGA &&
> + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMWARE_SVGA)) ||
> + (video->type == VIR_DOMAIN_VIDEO_TYPE_QXL &&
> + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QXL)) ||
> + (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO &&
> + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU))) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("this QEMU does not support '%s' video device"),
> + virDomainVideoTypeToString(video->type));
> + return -1;
> + }
This is a different check to the moved qemuBuildVideoCommandLine code.
1. The video->types were only checked if DEVICE_VIDEO_PRIMARY was
set. IOW: This is adding checks and failure scenario that weren't being
checked previously in the event that cap isn't set.
2. If DEVICE_VIDEO_PRIMARY, then only if the primary (video[0]) was
one of the types would the corresponding cap be checked. This change
will check each video[N] and each cap.
Since qemuDomainDefValidateVideo would have been called before this and
that video[0] == video->primary, then we know video[1]..video[n] can
only be VIDEO_TYPE_QXL (so far), so I think this check can be augmented
to match that.
For sure the non primary video checks need the QEMU_CAPS_DEVICE_QXL
because that is what was removed from qemuBuildDeviceVideoStr.
I think this is close, just needs a tweak. Unless you have it as "innate
knowledge" that videos[0] == video->primary, it can be trying to look at
code that sometimes keys off of video->primary while other code keys off
videos[0] is primary.
> +
> + if (video->accel) {
> + if (video->accel->accel3d == VIR_TRISTATE_SWITCH_ON &&
> + (video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO ||
> + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU_VIRGL))) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("%s 3d acceleration is not supported"),
> + virDomainVideoTypeToString(video->type));
> + return -1;
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> +
> +static int
> qemuProcessStartValidateXML(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> virQEMUCapsPtr qemuCaps,
> @@ -4517,11 +4559,17 @@ qemuProcessStartValidate(virQEMUDriverPtr driver,
> if (qemuProcessStartValidateXML(driver, vm, qemuCaps, caps, flags) < 0)
> return -1;
>
> + if (qemuProcessStartValidateGraphics(vm) < 0)
> + return -1;
This is a separate "bug fix" due to 'df73f1db8' adding code in that
"non-fatal" issues section (whether it's a merge thing or not, I don't
recall). But it should be separate.
> +
> + if (qemuProcessStartValidateVideo(vm, qemuCaps) < 0)
> + return -1;
> +
> VIR_DEBUG("Checking for any possible (non-fatal) issues");
>
> qemuProcessStartWarnShmem(vm);
>
> - return qemuProcessStartValidateGraphics(vm);
> + return 0;
> }
>
As noted earlier, I think the rest of this should be separate.
John
>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 78bfb92..1e910f3 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -748,7 +748,8 @@ mymain(void)
> QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_DRIVE_AIO,
> QEMU_CAPS_CCID_PASSTHRU, QEMU_CAPS_CHARDEV,
> QEMU_CAPS_CHARDEV_SPICEVMC, QEMU_CAPS_SPICE,
> - QEMU_CAPS_HDA_DUPLEX, QEMU_CAPS_USB_HUB);
> + QEMU_CAPS_HDA_DUPLEX, QEMU_CAPS_USB_HUB,
> + QEMU_CAPS_DEVICE_CIRRUS_VGA);
> DO_TEST("eoi-disabled", NONE);
> DO_TEST("eoi-enabled", NONE);
> DO_TEST("pv-spinlock-disabled", NONE);
> @@ -963,32 +964,39 @@ mymain(void)
> QEMU_CAPS_KVM,
> QEMU_CAPS_DRIVE_SERIAL);
>
> - DO_TEST("graphics-vnc", QEMU_CAPS_VNC);
> - DO_TEST("graphics-vnc-socket", QEMU_CAPS_VNC);
> - DO_TEST("graphics-vnc-websocket", QEMU_CAPS_VNC, QEMU_CAPS_VNC_WEBSOCKET);
> - DO_TEST("graphics-vnc-policy", QEMU_CAPS_VNC, QEMU_CAPS_VNC_SHARE_POLICY);
> - DO_TEST("graphics-vnc-no-listen-attr", QEMU_CAPS_VNC);
> - DO_TEST("graphics-vnc-remove-generated-socket", QEMU_CAPS_VNC);
> + DO_TEST("graphics-vnc", QEMU_CAPS_VNC, QEMU_CAPS_DEVICE_CIRRUS_VGA);
> + DO_TEST("graphics-vnc-socket", QEMU_CAPS_VNC, QEMU_CAPS_DEVICE_CIRRUS_VGA);
> + DO_TEST("graphics-vnc-websocket", QEMU_CAPS_VNC, QEMU_CAPS_VNC_WEBSOCKET,
> + QEMU_CAPS_DEVICE_CIRRUS_VGA);
> + DO_TEST("graphics-vnc-policy", QEMU_CAPS_VNC, QEMU_CAPS_VNC_SHARE_POLICY,
> + QEMU_CAPS_DEVICE_CIRRUS_VGA);
> + DO_TEST("graphics-vnc-no-listen-attr", QEMU_CAPS_VNC,
> + QEMU_CAPS_DEVICE_CIRRUS_VGA);
> + DO_TEST("graphics-vnc-remove-generated-socket", QEMU_CAPS_VNC,
> + QEMU_CAPS_DEVICE_CIRRUS_VGA);
> driver.config->vncAutoUnixSocket = true;
> - DO_TEST("graphics-vnc-auto-socket-cfg", QEMU_CAPS_VNC);
> + DO_TEST("graphics-vnc-auto-socket-cfg", QEMU_CAPS_VNC,
> + QEMU_CAPS_DEVICE_CIRRUS_VGA);
> driver.config->vncAutoUnixSocket = false;
> - DO_TEST("graphics-vnc-socket", QEMU_CAPS_VNC);
> - DO_TEST("graphics-vnc-auto-socket", QEMU_CAPS_VNC);
> - DO_TEST("graphics-vnc-none", QEMU_CAPS_VNC);
> + DO_TEST("graphics-vnc-socket", QEMU_CAPS_VNC, QEMU_CAPS_DEVICE_CIRRUS_VGA);
> + DO_TEST("graphics-vnc-auto-socket", QEMU_CAPS_VNC,
> + QEMU_CAPS_DEVICE_CIRRUS_VGA);
> + DO_TEST("graphics-vnc-none", QEMU_CAPS_VNC, QEMU_CAPS_DEVICE_CIRRUS_VGA);
>
> driver.config->vncSASL = 1;
> VIR_FREE(driver.config->vncSASLdir);
> ignore_value(VIR_STRDUP(driver.config->vncSASLdir, "/root/.sasl2"));
> - DO_TEST("graphics-vnc-sasl", QEMU_CAPS_VNC);
> + DO_TEST("graphics-vnc-sasl", QEMU_CAPS_VNC, QEMU_CAPS_DEVICE_CIRRUS_VGA);
> driver.config->vncTLS = 1;
> driver.config->vncTLSx509verify = 1;
> - DO_TEST("graphics-vnc-tls", QEMU_CAPS_VNC);
> + DO_TEST("graphics-vnc-tls", QEMU_CAPS_VNC, QEMU_CAPS_DEVICE_CIRRUS_VGA);
> driver.config->vncSASL = driver.config->vncTLSx509verify = driver.config->vncTLS = 0;
> VIR_FREE(driver.config->vncSASLdir);
> VIR_FREE(driver.config->vncTLSx509certdir);
>
> - DO_TEST("graphics-sdl", QEMU_CAPS_SDL);
> - DO_TEST("graphics-sdl-fullscreen", QEMU_CAPS_SDL);
> + DO_TEST("graphics-sdl", QEMU_CAPS_SDL, QEMU_CAPS_DEVICE_VGA);
> + DO_TEST("graphics-sdl-fullscreen", QEMU_CAPS_SDL,
> + QEMU_CAPS_DEVICE_CIRRUS_VGA);
> DO_TEST("nographics", NONE);
> DO_TEST("nographics-display",
> QEMU_CAPS_DISPLAY);
> @@ -999,7 +1007,7 @@ mymain(void)
> QEMU_CAPS_DEVICE_QXL,
> QEMU_CAPS_SPICE_FILE_XFER_DISABLE);
> DO_TEST("graphics-spice-no-args",
> - QEMU_CAPS_SPICE);
> + QEMU_CAPS_SPICE, QEMU_CAPS_DEVICE_CIRRUS_VGA);
> driver.config->spiceSASL = 1;
> ignore_value(VIR_STRDUP(driver.config->spiceSASLdir, "/root/.sasl2"));
> DO_TEST("graphics-spice-sasl",
> @@ -1011,14 +1019,16 @@ mymain(void)
> QEMU_CAPS_DEVICE_QXL,
> QEMU_CAPS_SPICE,
> QEMU_CAPS_CHARDEV_SPICEVMC,
> - QEMU_CAPS_NODEFCONFIG);
> + QEMU_CAPS_NODEFCONFIG,
> + QEMU_CAPS_DEVICE_CIRRUS_VGA);
> DO_TEST("graphics-spice-compression",
> QEMU_CAPS_SPICE,
> QEMU_CAPS_DEVICE_QXL);
> DO_TEST("graphics-spice-timeout",
> QEMU_CAPS_KVM,
> QEMU_CAPS_SPICE,
> - QEMU_CAPS_DEVICE_QXL);
> + QEMU_CAPS_DEVICE_QXL,
> + QEMU_CAPS_DEVICE_VGA);
> DO_TEST("graphics-spice-qxl-vga",
> QEMU_CAPS_SPICE,
> QEMU_CAPS_DEVICE_QXL);
> @@ -1027,21 +1037,24 @@ mymain(void)
> QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG,
> QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_USB_HUB,
> QEMU_CAPS_ICH9_USB_EHCI1, QEMU_CAPS_USB_REDIR,
> - QEMU_CAPS_CHARDEV_SPICEVMC);
> + QEMU_CAPS_CHARDEV_SPICEVMC, QEMU_CAPS_DEVICE_CIRRUS_VGA);
> DO_TEST("graphics-spice-agent-file-xfer",
> QEMU_CAPS_SPICE,
> QEMU_CAPS_DEVICE_QXL,
> QEMU_CAPS_SPICE_FILE_XFER_DISABLE);
> DO_TEST("graphics-spice-socket",
> QEMU_CAPS_SPICE,
> - QEMU_CAPS_SPICE_UNIX);
> + QEMU_CAPS_SPICE_UNIX,
> + QEMU_CAPS_DEVICE_CIRRUS_VGA);
> DO_TEST("graphics-spice-auto-socket",
> QEMU_CAPS_SPICE,
> - QEMU_CAPS_SPICE_UNIX);
> + QEMU_CAPS_SPICE_UNIX,
> + QEMU_CAPS_DEVICE_CIRRUS_VGA);
> driver.config->spiceAutoUnixSocket = true;
> DO_TEST("graphics-spice-auto-socket-cfg",
> QEMU_CAPS_SPICE,
> - QEMU_CAPS_SPICE_UNIX);
> + QEMU_CAPS_SPICE_UNIX,
> + QEMU_CAPS_DEVICE_CIRRUS_VGA);
> driver.config->spiceAutoUnixSocket = false;
>
> DO_TEST("input-usbmouse", NONE);
> @@ -1184,10 +1197,12 @@ mymain(void)
> QEMU_CAPS_VIRTIO_S390, QEMU_CAPS_SCLP_S390);
> DO_TEST("channel-spicevmc",
> QEMU_CAPS_NODEFCONFIG,
> - QEMU_CAPS_SPICE, QEMU_CAPS_CHARDEV_SPICEVMC);
> + QEMU_CAPS_SPICE, QEMU_CAPS_CHARDEV_SPICEVMC,
> + QEMU_CAPS_DEVICE_CIRRUS_VGA);
> DO_TEST("channel-spicevmc-old",
> QEMU_CAPS_NODEFCONFIG,
> - QEMU_CAPS_SPICE, QEMU_CAPS_DEVICE_SPICEVMC);
> + QEMU_CAPS_SPICE, QEMU_CAPS_DEVICE_SPICEVMC,
> + QEMU_CAPS_DEVICE_CIRRUS_VGA);
> DO_TEST("channel-virtio-default",
> QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG,
> QEMU_CAPS_SPICE, QEMU_CAPS_CHARDEV_SPICEVMC);
> @@ -1561,7 +1576,7 @@ mymain(void)
> QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
> QEMU_CAPS_DEVICE_QXL,
> QEMU_CAPS_DEVICE_PCI_BRIDGE);
> - DO_TEST("video-vga-nodevice", NONE);
> + DO_TEST("video-vga-nodevice", QEMU_CAPS_DEVICE_VGA);
> DO_TEST("video-vga-device", QEMU_CAPS_DEVICE_VGA,
> QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
> DO_TEST("video-vga-device-vgamem", QEMU_CAPS_DEVICE_VGA,
> @@ -1652,11 +1667,14 @@ mymain(void)
> DO_TEST_PARSE_ERROR("pci-slot-invalid", NONE);
> DO_TEST_PARSE_ERROR("pci-function-invalid", NONE);
>
> - DO_TEST("pci-autoadd-addr", QEMU_CAPS_DEVICE_PCI_BRIDGE);
> - DO_TEST("pci-autoadd-idx", QEMU_CAPS_DEVICE_PCI_BRIDGE);
> - DO_TEST("pci-autofill-addr", NONE);
> + DO_TEST("pci-autoadd-addr", QEMU_CAPS_DEVICE_PCI_BRIDGE,
> + QEMU_CAPS_DEVICE_CIRRUS_VGA);
> + DO_TEST("pci-autoadd-idx", QEMU_CAPS_DEVICE_PCI_BRIDGE,
> + QEMU_CAPS_DEVICE_CIRRUS_VGA);
> + DO_TEST("pci-autofill-addr", QEMU_CAPS_DEVICE_CIRRUS_VGA);
> DO_TEST("pci-many",
> - QEMU_CAPS_DEVICE_PCI_BRIDGE);
> + QEMU_CAPS_DEVICE_PCI_BRIDGE,
> + QEMU_CAPS_DEVICE_CIRRUS_VGA);
> DO_TEST("pci-bridge-many-disks",
> QEMU_CAPS_DEVICE_PCI_BRIDGE);
> DO_TEST("pcie-root",
> @@ -2131,7 +2149,7 @@ mymain(void)
>
> DO_TEST("name-escape", QEMU_CAPS_NAME_DEBUG_THREADS,
> QEMU_CAPS_OBJECT_SECRET, QEMU_CAPS_CHARDEV, QEMU_CAPS_VNC,
> - QEMU_CAPS_NAME_GUEST);
> + QEMU_CAPS_NAME_GUEST, QEMU_CAPS_DEVICE_CIRRUS_VGA);
> DO_TEST("debug-threads", QEMU_CAPS_NAME_DEBUG_THREADS);
>
> DO_TEST("master-key", QEMU_CAPS_OBJECT_SECRET);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list