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

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

 



On Sat, Oct 08, 2016 at 10:02:00AM -0400, John Ferlan wrote:
> 
> 
> 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@xxxxxxxxxx>
> > ---
> >  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...

Yes, because previous patch adds proper detection whether we can use
*virtio-vga* model or not, so we can use that capability instead of
listing architectures (which may change in the future).

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

At that time it was a quick fix to resolve that bug and using the capability
was not possible because that capability did not exist.

> 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!

This basically fixes a bug by improving the logic, I'll try to explain it
better in the commit message.

Pavel

> 
> 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: Digital signature

--
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]