[libvirt] [PATCH v2 4/6] qemu: consolidate video validation
Cole Robinson
crobinso at redhat.com
Tue Oct 15 17:03:20 UTC 2019
On 10/11/19 5:27 PM, Jonathon Jongsma wrote:
> Move video validation logic from qemuProcessStartValidateVideo() to
> qemuDomainDeviceDefValidateVideo() (which is in fact called from the
> aforementioned function).
>
As mentioned in the other response, this patch adds a lot of test suite
breakage. By moving more validation from process start time to XML parse
time, it exposes that many test suite test cases do not have the full
list of capabilities specified that it would need to actually boot the
XML under test.
Most of the example failures are in qemuxml2xml test. Run
VIR_TEST_DEBUG=1 ./tests/qemuxml2xmltest to see each failure and the
associated error message. Most of these cases will just be adding the
associated CIRRUS or VGA qemuCaps to the list in qemuxml2xmltest.c. You
can also look at the same test case in qemuxml2argvtest.c and copy the
caps list from there.
Also this patch should be broken up even more IMO. Some ideas:
* Add a qemuDomainDeviceDefValidateVideoModel from the existing code
qemu_domain.c code, which should be a no-op
* Extend it to handle the vhostuser validation
* Move the video->accel validation to qemuDomainDeviceDefValidateVideo.
it may need to grow an extra check for backend != vhostuser because we
will be de-nesting it
* Move the remaining model validation to
qemuDomainDeviceDefValidateVideoModel
Some of that will also help split up the test suite breakage, rather
than requiring you to adjust them all at once in one patch.
Thanks,
Cole
> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> ---
> src/qemu/qemu_domain.c | 172 +++++++++++++++++++++++++---------------
> src/qemu/qemu_process.c | 62 ---------------
> 2 files changed, 107 insertions(+), 127 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index bc455e7da3..def90a0f7d 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5706,83 +5706,125 @@ qemuDomainDeviceDefValidateHostdev(const virDomainHostdevDef *hostdev,
>
>
> static int
> -qemuDomainDeviceDefValidateVideo(const virDomainVideoDef *video)
> +qemuDomainDeviceDefValidateVideo(const virDomainVideoDef *video,
> + virQEMUCapsPtr qemuCaps)
> {
> - switch ((virDomainVideoType) video->type) {
> - case VIR_DOMAIN_VIDEO_TYPE_NONE:
> - return 0;
> - case VIR_DOMAIN_VIDEO_TYPE_XEN:
> - case VIR_DOMAIN_VIDEO_TYPE_VBOX:
> - case VIR_DOMAIN_VIDEO_TYPE_PARALLELS:
> - case VIR_DOMAIN_VIDEO_TYPE_GOP:
> - case VIR_DOMAIN_VIDEO_TYPE_DEFAULT:
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("video type '%s' is not supported with QEMU"),
> - virDomainVideoTypeToString(video->type));
> - return -1;
> - case VIR_DOMAIN_VIDEO_TYPE_VGA:
> - case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
> - case VIR_DOMAIN_VIDEO_TYPE_VMVGA:
> - case VIR_DOMAIN_VIDEO_TYPE_QXL:
> - case VIR_DOMAIN_VIDEO_TYPE_VIRTIO:
> - case VIR_DOMAIN_VIDEO_TYPE_BOCHS:
> - case VIR_DOMAIN_VIDEO_TYPE_RAMFB:
> - case VIR_DOMAIN_VIDEO_TYPE_LAST:
> - break;
> - }
> -
> - if (!video->primary &&
> - video->type != VIR_DOMAIN_VIDEO_TYPE_QXL &&
> - video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("video type '%s' is only valid as primary "
> - "video device"),
> - virDomainVideoTypeToString(video->type));
> - return -1;
> - }
> -
> - if (video->accel && video->accel->accel2d == VIR_TRISTATE_SWITCH_ON) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("qemu does not support the accel2d setting"));
> - return -1;
> - }
> -
> - if (video->type == VIR_DOMAIN_VIDEO_TYPE_QXL) {
> - if (video->vram > (UINT_MAX / 1024)) {
> - virReportError(VIR_ERR_OVERFLOW,
> - _("value for 'vram' must be less than '%u'"),
> - UINT_MAX / 1024);
> + if (video->backend == VIR_DOMAIN_VIDEO_BACKEND_TYPE_VHOSTUSER) {
> + if (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO &&
> + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VHOST_USER_GPU)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("this QEMU does not support 'vhost-user' video device"));
> + return -1;
> + }
> + } else {
> + switch ((virDomainVideoType) video->type) {
> + case VIR_DOMAIN_VIDEO_TYPE_NONE:
> + return 0;
> + case VIR_DOMAIN_VIDEO_TYPE_XEN:
> + case VIR_DOMAIN_VIDEO_TYPE_VBOX:
> + case VIR_DOMAIN_VIDEO_TYPE_PARALLELS:
> + case VIR_DOMAIN_VIDEO_TYPE_GOP:
> + case VIR_DOMAIN_VIDEO_TYPE_DEFAULT:
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("video type '%s' is not supported with QEMU"),
> + virDomainVideoTypeToString(video->type));
> + return -1;
> + case VIR_DOMAIN_VIDEO_TYPE_VGA:
> + case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
> + case VIR_DOMAIN_VIDEO_TYPE_VMVGA:
> + case VIR_DOMAIN_VIDEO_TYPE_QXL:
> + case VIR_DOMAIN_VIDEO_TYPE_VIRTIO:
> + case VIR_DOMAIN_VIDEO_TYPE_BOCHS:
> + case VIR_DOMAIN_VIDEO_TYPE_RAMFB:
> + case VIR_DOMAIN_VIDEO_TYPE_LAST:
> + break;
> + }
> + 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)) ||
> + (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO &&
> + video->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW &&
> + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU_CCW)) ||
> + (video->type == VIR_DOMAIN_VIDEO_TYPE_BOCHS &&
> + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_BOCHS_DISPLAY)) ||
> + (video->type == VIR_DOMAIN_VIDEO_TYPE_RAMFB &&
> + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_RAMFB))) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("this QEMU does not support '%s' video device"),
> + virDomainVideoTypeToString(video->type));
> return -1;
> }
> - if (video->ram > (UINT_MAX / 1024)) {
> - virReportError(VIR_ERR_OVERFLOW,
> - _("value for 'ram' must be less than '%u'"),
> - UINT_MAX / 1024);
> +
> + if (!video->primary &&
> + video->type != VIR_DOMAIN_VIDEO_TYPE_QXL &&
> + video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("video type '%s' is only valid as primary "
> + "video device"),
> + virDomainVideoTypeToString(video->type));
> return -1;
> }
> - if (video->vgamem) {
> - if (video->vgamem < 1024) {
> +
> + if (video->accel) {
> + if (video->accel->accel3d == VIR_TRISTATE_SWITCH_ON &&
> + (video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO ||
> + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_GPU_VIRGL))) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("%s 3d acceleration is not supported"),
> + virDomainVideoTypeToString(video->type));
> + return -1;
> + }
> + if (video->accel->accel2d == VIR_TRISTATE_SWITCH_ON) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("value for 'vgamem' must be at least 1 MiB "
> - "(1024 KiB)"));
> + _("qemu does not support the accel2d setting"));
> return -1;
> }
> + }
>
> - if (video->vgamem != VIR_ROUND_UP_POWER_OF_TWO(video->vgamem)) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("value for 'vgamem' must be power of two"));
> + if (video->type == VIR_DOMAIN_VIDEO_TYPE_QXL) {
> + if (video->vram > (UINT_MAX / 1024)) {
> + virReportError(VIR_ERR_OVERFLOW,
> + _("value for 'vram' must be less than '%u'"),
> + UINT_MAX / 1024);
> + return -1;
> + }
> + if (video->ram > (UINT_MAX / 1024)) {
> + virReportError(VIR_ERR_OVERFLOW,
> + _("value for 'ram' must be less than '%u'"),
> + UINT_MAX / 1024);
> return -1;
> }
> + if (video->vgamem) {
> + if (video->vgamem < 1024) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("value for 'vgamem' must be at least 1 MiB "
> + "(1024 KiB)"));
> + return -1;
> + }
> +
> + if (video->vgamem != VIR_ROUND_UP_POWER_OF_TWO(video->vgamem)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("value for 'vgamem' must be power of two"));
> + return -1;
> + }
> + }
> }
> - }
>
> - if (video->type == VIR_DOMAIN_VIDEO_TYPE_VGA ||
> - video->type == VIR_DOMAIN_VIDEO_TYPE_VMVGA) {
> - if (video->vram && video->vram < 1024) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - "%s", _("value for 'vram' must be at least "
> - "1 MiB (1024 KiB)"));
> - return -1;
> + if (video->type == VIR_DOMAIN_VIDEO_TYPE_VGA ||
> + video->type == VIR_DOMAIN_VIDEO_TYPE_VMVGA) {
> + if (video->vram && video->vram < 1024) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + "%s", _("value for 'vram' must be at least "
> + "1 MiB (1024 KiB)"));
> + return -1;
> + }
> }
> }
>
> @@ -7247,7 +7289,7 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev,
> break;
>
> case VIR_DOMAIN_DEVICE_VIDEO:
> - ret = qemuDomainDeviceDefValidateVideo(dev->data.video);
> + ret = qemuDomainDeviceDefValidateVideo(dev->data.video, qemuCaps);
> break;
>
> case VIR_DOMAIN_DEVICE_DISK:
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index c14c09da11..df7b7c2d3e 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -5240,65 +5240,6 @@ qemuProcessStartValidateGraphics(virDomainObjPtr vm)
> return 0;
> }
>
> -
> -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->backend == VIR_DOMAIN_VIDEO_BACKEND_TYPE_VHOSTUSER) {
> - if (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO &&
> - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VHOST_USER_GPU)) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("this QEMU does not support 'vhost-user' video device"));
> - return -1;
> - }
> - } else {
> - 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)) ||
> - (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO &&
> - video->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW &&
> - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU_CCW)) ||
> - (video->type == VIR_DOMAIN_VIDEO_TYPE_BOCHS &&
> - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_BOCHS_DISPLAY)) ||
> - (video->type == VIR_DOMAIN_VIDEO_TYPE_RAMFB &&
> - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_RAMFB))) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("this QEMU does not support '%s' video device"),
> - virDomainVideoTypeToString(video->type));
> - return -1;
> - }
> -
> - if (video->accel) {
> - if (video->accel->accel3d == VIR_TRISTATE_SWITCH_ON &&
> - (video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO ||
> - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_GPU_VIRGL))) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> - _("%s 3d acceleration is not supported"),
> - virDomainVideoTypeToString(video->type));
> - return -1;
> - }
> - }
> - }
> - }
> -
> - return 0;
> -}
> -
> -
> static int
> qemuProcessStartValidateIOThreads(virDomainObjPtr vm,
> virQEMUCapsPtr qemuCaps)
> @@ -5483,9 +5424,6 @@ qemuProcessStartValidate(virQEMUDriverPtr driver,
> if (qemuProcessStartValidateGraphics(vm) < 0)
> return -1;
>
> - if (qemuProcessStartValidateVideo(vm, qemuCaps) < 0)
> - return -1;
> -
> if (qemuProcessStartValidateIOThreads(vm, qemuCaps) < 0)
> return -1;
>
>
- Cole
More information about the libvir-list
mailing list