Re: [libvirt] [PATCH 10/11] qemu_command: properly detect which model to use for video device

On 09/30/2016 12:02 PM, Pavel Hrdina wrote: > This improves commit 706b5b6277 in a way that we instead use qemu > capabilities to detect whether we can use virto-vga or not. > > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; > --- > src/qemu/qemu_command.c | 4 +-- > src/qemu/qemu_domain.c | 12 ++++++++ > src/qemu/qemu_domain.h | 3 ++ > .../qemuxml2argv-video-virtio-gpu-device.args | 2 +- > .../qemuxml2argv-video-virtio-gpu-spice-gl.args | 2 +- > .../qemuxml2argv-video-virtio-gpu-virgl.args | 2 +- > .../qemuxml2argv-video-virtio-vga.args | 24 ++++++++++++++++ > .../qemuxml2argv-video-virtio-vga.xml | 33 ++++++++++++++++++++++ > tests/qemuxml2argvtest.c | 4 +++ > 9 files changed, 80 insertions(+), 6 deletions(-) > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-virtio-vga.args > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-virtio-vga.xml > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 15bb381..bb8128e 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -4307,10 +4307,8 @@ qemuBuildDeviceVideoStr(const virDomainDef *def, > virDomainVideoTypeToString(video->type)); > goto error; > } > - if (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO && > - qemuDomainMachineIsVirt(def)) { > + if (!qemuDomainSupportVideoVga(video, qemuCaps)) > model = "virtio-gpu-pci"; > - } > } else { > model = "qxl"; > } > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index 2b7e6d4..79c945a 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -6230,3 +6230,15 @@ qemuDomainCheckMonitor(virQEMUDriverPtr driver, > > return ret; > } > + > + > +bool > +qemuDomainSupportVideoVga(virDomainVideoDefPtr video, > + virQEMUCapsPtr qemuCaps) > +{ > + if (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO && > + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_VGA)) Shall I assume losing the qemuDomainMachineIsVirt check is expected? Perhaps a quick explanation in the commit message would suffice...

As in, commit id 'xxx' use MachineIsVirt, when it should have been checking the cap instead...

There's two things going on in this one patch - not only creating a helper, but you're altering the logic such that you have differences in output (.args) that should have been "that way" when the code was first introduced. In a way it feels like fixing a bug... Following the breadcrumbs is, well mind numbing!

ACK in principle - still just a better explanation would be good! John > + return false; > + > + return true; > +} > diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h > index 521531b..b968858 100644 > --- a/src/qemu/qemu_domain.h > +++ b/src/qemu/qemu_domain.h > @@ -738,4 +738,7 @@ int qemuDomainCheckMonitor(virQEMUDriverPtr driver, > virDomainObjPtr vm, > qemuDomainAsyncJob asyncJob); > > +bool qemuDomainSupportVideoVga(virDomainVideoDefPtr video, > + virQEMUCapsPtr qemuCaps); > + > #endif /* __QEMU_DOMAIN_H__ */ > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-device.args b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-device.args > index fefa2b6..1107409 100644 > --- a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-device.args > +++ b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-device.args > @@ -20,5 +20,5 @@ QEMU_AUDIO_DRV=none \ > -drive file=/var/lib/libvirt/images/QEMUGuest1,format=qcow2,if=none,\ > id=drive-ide0-0-0,cache=none \ > -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ > --device virtio-vga,id=video0,bus=pci.0,addr=0x2 \ > +-device virtio-gpu-pci,id=video0,bus=pci.0,addr=0x2 \ > -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args > index 8844498..f1ebb62 100644 > --- a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args > +++ b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args > @@ -20,5 +20,5 @@ QEMU_AUDIO_DRV=spice \ > id=drive-ide0-0-0,cache=none \ > -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ > -spice port=0,gl=on \ > --device virtio-vga,id=video0,virgl=on,bus=pci.0,addr=0x2 \ > +-device virtio-gpu-pci,id=video0,virgl=on,bus=pci.0,addr=0x2 \ > -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-virgl.args b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-virgl.args > index 6a55311..0131be8 100644 > --- a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-virgl.args > +++ b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-virgl.args > @@ -20,5 +20,5 @@ QEMU_AUDIO_DRV=none \ > -drive file=/var/lib/libvirt/images/QEMUGuest1,format=qcow2,if=none,\ > id=drive-ide0-0-0,cache=none \ > -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ > --device virtio-vga,id=video0,virgl=on,bus=pci.0,addr=0x2 \ > +-device virtio-gpu-pci,id=video0,virgl=on,bus=pci.0,addr=0x2 \ > -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-vga.args b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-vga.args > new file mode 100644 > index 0000000..fefa2b6 > --- /dev/null > +++ b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-vga.args > @@ -0,0 +1,24 @@ > +LC_ALL=C \ > +PATH=/bin \ > +HOME=/home/test \ > +USER=test \ > +LOGNAME=test \ > +QEMU_AUDIO_DRV=none \ > +/usr/bin/qemu \ > +-name QEMUGuest1 \ > +-S \ > +-M pc \ > +-m 1024 \ > +-smp 1,sockets=1,cores=1,threads=1 \ > +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ > +-nographic \ > +-nodefaults \ > +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ > +-no-acpi \ > +-boot c \ > +-usb \ > +-drive file=/var/lib/libvirt/images/QEMUGuest1,format=qcow2,if=none,\ > +id=drive-ide0-0-0,cache=none \ > +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ > +-device virtio-vga,id=video0,bus=pci.0,addr=0x2 \ > +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-vga.xml b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-vga.xml > new file mode 100644 > index 0000000..e3d28bb > --- /dev/null > +++ b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-vga.xml > @@ -0,0 +1,33 @@ > +<domain type='qemu'> > + <name>QEMUGuest1</name> > + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> > + <memory unit='KiB'>1048576</memory> > + <currentMemory unit='KiB'>1048576</currentMemory> > + <vcpu placement='static'>1</vcpu> > + <os> > + <type arch='i686' machine='pc'>hvm</type> > + <boot dev='hd'/> > + </os> > + <clock offset='utc'/> > + <on_poweroff>destroy</on_poweroff> > + <on_reboot>restart</on_reboot> > + <on_crash>destroy</on_crash> > + <devices> > + <emulator>/usr/bin/qemu</emulator> > + <disk type='file' device='disk'> > + <driver name='qemu' type='qcow2' cache='none'/> > + <source file='/var/lib/libvirt/images/QEMUGuest1'/> > + <target dev='hda' bus='ide'/> > + <address type='drive' controller='0' bus='0' target='0' unit='0'/> > + </disk> > + <controller type='ide' index='0'/> > + <controller type='usb' index='0'/> > + <controller type='pci' index='0' model='pci-root'/> > + <input type='mouse' bus='ps2'/> > + <input type='keyboard' bus='ps2'/> > + <video> > + <model type='virtio' heads='1'/> > + </video> > + <memballoon model='virtio'/> > + </devices> > +</domain> > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c > index 6404865..4cbe57e 100644 > --- a/tests/qemuxml2argvtest.c > +++ b/tests/qemuxml2argvtest.c > @@ -1616,6 +1616,10 @@ mymain(void) > QEMU_CAPS_SPICE, > QEMU_CAPS_SPICE_GL, > QEMU_CAPS_DEVICE_VIDEO_PRIMARY); > + DO_TEST("video-virtio-vga", > + QEMU_CAPS_DEVICE_VIRTIO_GPU, > + QEMU_CAPS_DEVICE_VIRTIO_VGA, > + QEMU_CAPS_DEVICE_VIDEO_PRIMARY); > DO_TEST_PARSE_ERROR("video-invalid", NONE); > > DO_TEST("virtio-rng-default", QEMU_CAPS_DEVICE_VIRTIO_RNG, > -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list