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

[Cole Robinson]

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 at redhat.com>
> ---
>   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 at redhat.com>

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