Re: [libvirt] [PATCH 1/1] qemu: fix type of default video device

On 10/8/19 7:16 AM, Pavel Mores wrote: In response to your RFE message earlier, yup this is the right place to do it. I think for starters, breaking the existing logic out into its own function is a nice cleanup. Then every 'video->type = X' can be a 'return', which will make the function flow nicely IMO. A good model to follow is qemuDomainDefaultNetModel Then you can change the log, ideally adding a test case for each one. The test cases will need custom qemu caps lists to hit certain cases, or you can just add a test with RHEL8 qemu caps to catch the 'no cirrus' default case. As for getting into the business of trying to choose an optimal default, I have mixed feelings. Trying to abide opinionated decisions like 'bochs is preferred if firmware=efi' and 'qxl is preferred if type=spice' is a bit of a slippery slope. I think libvirt should just stick with trying to generate any working config, and leave the higher level desires up to management tools. So IMO for this, I think libvirt could just get away with if cirrus supported: use cirrus elif vga supported: use vga else do nothing. domain_conf.c will error that a video default wasn't filled in That will at least inform the user that they need to explicitly specify a video model. If we wanted to extend that matrix to cover qxl and virtio and bochs then that's an option too but I'd rather see some smarter way of dealing with it, like a function to say 'give me all the supported VIR_DOMAIN_VIDEO_TYPEs' and this function just pops one off the top. But IMO that's kinda overkill, just hitting cirrus and vga and maybe virtio for non-x86 virt arches is probably sufficient One comment below If you decide to keep the qxl bit here, you don't need to set this VGAMEM value, it's set in the if block below - Cole - Cole