Re: [PATCH 1/4] qemu: validate bochs-display capability

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 9/13/19 5:20 PM, Jonathon Jongsma wrote:
When the bochs display type was added, the capability was never checked.
Add that check in the same place as the other video device capability
checks.

Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
---
  src/qemu/qemu_process.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 955ba4de4c..b93af966e2 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5278,7 +5278,9 @@ qemuProcessStartValidateVideo(virDomainObjPtr vm,
               !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))) {
+             !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU_CCW)) ||
+            (video->type == VIR_DOMAIN_VIDEO_TYPE_BOCHS &&
+             !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_BOCHS_DISPLAY))) {
              virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                             _("this QEMU does not support '%s' video device"),
                             virDomainVideoTypeToString(video->type));


For 1-3:

Reviewed-by: Cole Robinson <crobinso@xxxxxxxxxx>

I rebased, adjusted the 5.8.0 references and pushed.

Since patch #4 is adding a new XML element, can you rebase and send, but with the new XML in the commit message? You can CC me and I'll review it, but let it sit for a bit incase anyone else has comments on the new XML

Some general comments:

* Not sure if it's a rule around here, but I suggest always sending a cover letter for > 1 patches. Personally for me an upfront summary can help reduce cognitive load of the series, I have an idea of where it is going without having to look at every patch to figure it out.

* Please do for 'ramfb' what fabiano did for bochs-display here: https://www.redhat.com/archives/libvir-list/2019-October/msg00123.html Otherwise apps don't have a good way to determine if ramfb is supported or not.


Sidenote: Better still, you can unify the validate code and domcaps code using an approach like I added for RNG here: https://www.redhat.com/archives/libvir-list/2019-April/msg00457.html

Starting point would be to fold the whole qemuProcessStartValidateVideo to be part of qemuDomainDeviceDefValidateVideo. It's only at ProcessStart time for historical reasons IIRC, back when we didn't have proper infrastructure for the Validate path that wouldn't make VMs disappear on libvirtd restart.

CCing fidencio too who I've talked about this last bit in private, and pmores who is working on nearby code currently. I promise timely reviews for anyone who works on this and CCs me :)

- Cole

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux