On Sat, Oct 08, 2016 at 10:02:14AM -0400, John Ferlan wrote:
>
> Nothing else to say?! Is this fixing an existing bug making some other
> patch/fix "more correct".
After sending the patch I've find out that there is a BZ for this and I was
planning to add it to the commit message before pushing. Other than that the
subject explain everything form my POW.
> On 09/30/2016 12:02 PM, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
> > ---
> > docs/formatdomain.html.in | 3 +-
> > src/qemu/qemu_command.c | 32 ++++++++++++-------
> > src/qemu/qemu_domain.c | 3 +-
> > .../qemuxml2argv-video-virtio-gpu-sec.args | 25 +++++++++++++++
> > .../qemuxml2argv-video-virtio-gpu-sec.xml | 36 ++++++++++++++++++++++
> > tests/qemuxml2argvtest.c | 3 ++
> > 6 files changed, 89 insertions(+), 13 deletions(-)
> > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-sec.args
> > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-sec.xml
> >
>
> Adding a new .xml without also adding a qemuxml2xmltest for the same xml
> file? Note that you can "save" space if you make the xml2xml output
> file be the same as the input file and just create a link to it (like a
> number of other more recent changes are doing).
>
>
> Also it seems there are two changes happening here - thus separate patches.
>
> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > index 1266e9d..f08cd01 100644
> > --- a/docs/formatdomain.html.in
> > +++ b/docs/formatdomain.html.in
> > @@ -5641,7 +5641,8 @@ qemu-kvm -net nic,model=? /dev/null
> > video device in domain xml is the primary one, but the optional
> > attribute <code>primary</code> (<span class="since">since 1.0.2</span>)
> > with value 'yes' can be used to mark the primary in cases of multiple
> > - video device. The non-primary must be type of "qxl".
> > + video device. The non-primary must be type of "qxl" or
> > + "virtio" (<span class="since">since 2.4.0</span>).
> > </p>
> > </dd>
> >
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index bb8128e..2314b2df 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -114,6 +114,18 @@ VIR_ENUM_IMPL(qemuDeviceVideo, VIR_DOMAIN_VIDEO_TYPE_LAST,
> > "", /* don't support parallels */
> > "virtio-vga");
> >
> > +VIR_ENUM_DECL(qemuDeviceVideoSec)
> > +
> > +VIR_ENUM_IMPL(qemuDeviceVideoSec, VIR_DOMAIN_VIDEO_TYPE_LAST,
> > + "", /* no secondary device for VGA*/
> > + "", /* no secondary device for cirrus-vga*/
> > + "", /* no secondary device for vmware-svga*/
> > + "", /* no device for xen */
> > + "", /* don't support vbox */
> > + "qxl",
> > + "", /* don't support parallels */
> > + "virtio-gpu-pci");
> > +
>
> I know "Sec" is shorthand for secondary, but I've also seen it used for
> "second"... being verbose isn't bad especially when you read the code
> months later.
Sure, I can change it to ..Secondary.
>
> > VIR_ENUM_DECL(qemuSoundCodec)
> >
> > VIR_ENUM_IMPL(qemuSoundCodec, VIR_DOMAIN_SOUND_CODEC_TYPE_LAST,
> > @@ -4299,18 +4311,16 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
> > virBuffer buf = VIR_BUFFER_INITIALIZER;
> > const char *model;
> >
> > - if (video->primary) {
> > + if (video->primary && qemuDomainSupportVideoVga(video, qemuCaps))
>
> This alters your logic, but the else condition is assuming secondary and
> in fact digging into that table... Shouldn't the logic be:
>
> if (video->primary) {
> if (qemuDomainSupportVideoVga()
> model = qemuDeviceVideoTypeToString(video->type);
> else
> model = "virtio-gpu-pci";
> } else {
> model = qemuDeviceVideoSecTypeToString(video->type);
> }
This is exactly what I was trying to avoid. Having an extra string outside
of that two enums. There are two sets of models for video devices, the first
one is with VGA compatibility support (qemuDeviceVideo) and the second one is
without the VGA compatibility (qemuDeviceVideoSec). The concept of primary vs
secondary video devices is based on the fact that libvirt tries to use video
device with VGA compatibility as primary video device.
The logic is to use the best possible model that is available.
>
> > model = qemuDeviceVideoTypeToString(video->type);
> > - if (!model || STREQ(model, "")) {
> > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > - _("video type %s is not supported with QEMU"),
> > - virDomainVideoTypeToString(video->type));
> > - goto error;
> > - }
> > - if (!qemuDomainSupportVideoVga(video, qemuCaps))
> > - model = "virtio-gpu-pci";
> > - } else {
> > - model = "qxl";
> > + else
> > + model = qemuDeviceVideoSecTypeToString(video->type);
> > +
> > + if (!model || STREQ(model, "")) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("invalid model for video type '%s'"),
> > + virDomainVideoTypeToString(video->type));
> > + goto error;
> > }
> >
> > virBufferAsprintf(&buf, "%s,id=%s", model, video->info.alias);
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 79c945a..8218609 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -2404,7 +2404,8 @@ qemuDomainDefValidateVideo(const virDomainDef *def)
> > video = def->videos[i];
> >
> > if (!video->primary &&
> > - video->type != VIR_DOMAIN_VIDEO_TYPE_QXL) {
> > + video->type != VIR_DOMAIN_VIDEO_TYPE_QXL &&
> > + video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO) {
>
> This would seemingly be a different/separate patch wouldn't it?
This updates the check which devices can be used as secondary so this is related
to this patch.
Thanks for review, I'll send a v2.
Pavel
>
> And I'm not sure which of the remaining checks this belongs with, but
> I'm assuming it's this change.
>
> John
>
> > virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > _("video type '%s' is only valid as primary "
> > "video device"),
> > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-sec.args b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-sec.args
> > new file mode 100644
> > index 0000000..a87c37b
> > --- /dev/null
> > +++ b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-sec.args
> > @@ -0,0 +1,25 @@
> > +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-gpu-pci,id=video0,bus=pci.0,addr=0x2 \
> > +-device virtio-gpu-pci,id=video1,bus=pci.0,addr=0x4 \
> > +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
> > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-sec.xml b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-sec.xml
> > new file mode 100644
> > index 0000000..feba717
> > --- /dev/null
> > +++ b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-sec.xml
> > @@ -0,0 +1,36 @@
> > +<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' primary='yes'/>
> > + </video>
> > + <video>
> > + <model type='virtio' heads='1'/>
> > + </video>
> > + <memballoon model='virtio'/>
> > + </devices>
> > +</domain>
> > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> > index 4cbe57e..d8bcf93 100644
> > --- a/tests/qemuxml2argvtest.c
> > +++ b/tests/qemuxml2argvtest.c
> > @@ -1616,6 +1616,9 @@ mymain(void)
> > QEMU_CAPS_SPICE,
> > QEMU_CAPS_SPICE_GL,
> > QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
> > + DO_TEST("video-virtio-gpu-sec",
> > + QEMU_CAPS_DEVICE_VIRTIO_GPU,
> > + QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
> > DO_TEST("video-virtio-vga",
> > QEMU_CAPS_DEVICE_VIRTIO_GPU,
> > QEMU_CAPS_DEVICE_VIRTIO_VGA,
> >
>
> --
> 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