[libvirt] [PATCH v4 1/2] conf: Add 'x' and 'y' resolution into video XML definition
Jonathon Jongsma
jjongsma at redhat.com
Thu Oct 17 19:24:21 UTC 2019
On Thu, 2019-10-17 at 14:12 -0500, Jonathon Jongsma wrote:
> So, you're adding the support for parsing and formatting the new
> resolution parameters in this patch, but you're not actually testing
> them as part of this patch. The new tests that you add here have no
> mention of these resolution parameters. So I think you should include
> the changes to tests/qemuxml2argvdata/video-qxl-resolution.xml and
> tests/qemuxml2xmloutdata/video-qxl-resolution.xml from the next patch
> into this patch so you're testing it in the same commit that you
> introduce it.
>
> However, that leaves a very small patch (basically only generating
> the
> parameters for the qemu command line and testing that) in the second
> patch. So in my mind the two patches could simply be combined. But
> I'll
> defer to other opinions on that.
>
> More comments below.
>
>
> On Thu, 2019-10-17 at 01:30 -0300, jcfaracco at gmail.com wrote:
> > From: Julio Faracco <jcfaracco at gmail.com>
> >
> > This commit adds resolution element with parameters 'x' and 'y'
> > into
> > video
> > XML domain group definition. Both, properties were added into an
> > element
> > called 'resolution' and it was added inside 'model' element. They
> > are
> > set
> > as optional. This element does not follow QEMU properties 'xres'
> > and
> > 'yres' format. Both HTML documentation and schema were changed too.
> > This
> > commit includes a simple test case to cover resolution for QEMU
> > video
> > models. The new XML format for resolution looks like:
> >
> > <model ...>
> > <resolution x='800' y='600'/>
> > </model>
> >
> > Signed-off-by: Julio Faracco <jcfaracco at gmail.com>
> > ---
> > docs/formatdomain.html.in | 5 +-
> > docs/schemas/domaincommon.rng | 10 +++
> > src/conf/domain_conf.c | 63
> > ++++++++++++++++++-
> > src/conf/domain_conf.h | 5 ++
> > src/conf/virconftypes.h | 3 +
> > .../video-qxl-resolution.args | 32 ++++++++++
> > .../qemuxml2argvdata/video-qxl-resolution.xml | 40 ++++++++++++
> > tests/qemuxml2argvtest.c | 4 ++
> > .../video-qxl-resolution.xml | 40 ++++++++++++
> > tests/qemuxml2xmltest.c | 1 +
> > 10 files changed, 201 insertions(+), 2 deletions(-)
> > create mode 100644 tests/qemuxml2argvdata/video-qxl-
> > resolution.args
> > create mode 100644 tests/qemuxml2argvdata/video-qxl-resolution.xml
> > create mode 100644 tests/qemuxml2xmloutdata/video-qxl-
> > resolution.xml
> >
> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > index 500f114f41..962766b792 100644
> > --- a/docs/formatdomain.html.in
> > +++ b/docs/formatdomain.html.in
> > @@ -7077,7 +7077,10 @@ qemu-kvm -net nic,model=? /dev/null
> > <code>vgamem</code> (<span class="since">since
> > 1.2.11</span>) to set
> > the size of VGA framebuffer for fallback mode of QXL
> > device.
> > Attribute <code>vram64</code> (<span class="since">since
> > 1.3.3</span>)
> > - extends secondary bar and makes it addressable as 64bit
> > memory.
> > + extends secondary bar and makes it addressable as 64bit
> > memory. For
> > + resolution settings, there are <code>x</code> and
> > <code>y</code>
> > + (<span class="since">since 5.9.0</span>) optional
> > attributes to set
> > + minimum resolution for model.
>
> I'd personally prefer the wording "optional x and y attributes"
> instead
> of "x and y optional attributes"
>
>
> > </p>
> > </dd>
> >
> > diff --git a/docs/schemas/domaincommon.rng
> > b/docs/schemas/domaincommon.rng
> > index ead5a25068..e06f892da3 100644
> > --- a/docs/schemas/domaincommon.rng
> > +++ b/docs/schemas/domaincommon.rng
> > @@ -3656,6 +3656,16 @@
> > </optional>
> > </element>
> > </optional>
> > + <optional>
> > + <element name="resolution">
> > + <attribute name="x">
> > + <ref name="unsignedInt"/>
> > + </attribute>
> > + <attribute name="y">
> > + <ref name="unsignedInt"/>
> > + </attribute>
> > + </element>
> > + </optional>
> > </element>
> > </optional>
> > <optional>
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 2e6a113de3..88e93f6fb8 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -15345,6 +15345,52 @@ virDomainVideoAccelDefParseXML(xmlNodePtr
> > node)
> > return def;
> > }
> >
> > +static virDomainVideoResolutionDefPtr
> > +virDomainVideoResolutionDefParseXML(xmlNodePtr node)
> > +{
> > + xmlNodePtr cur;
> > + virDomainVideoResolutionDefPtr def;
> > + g_autofree char *x = NULL;
> > + g_autofree char *y = NULL;
> > +
> > + cur = node->children;
> > + while (cur != NULL) {
> > + if (cur->type == XML_ELEMENT_NODE) {
> > + if (!x && !y &&
> > + virXMLNodeNameEqual(cur, "resolution")) {
> > + x = virXMLPropString(cur, "x");
> > + y = virXMLPropString(cur, "y");
> > + }
> > + }
> > + cur = cur->next;
> > + }
> > +
> > + if (!x || !y)
> > + return NULL;
> > +
> > + if (VIR_ALLOC(def) < 0)
>
> Consider using the glib allocation APIs (e.g. g_new0()) in new code.
> This is now the preferred API for memory allocation, and you don't
> need
> to handle failed allocation anymore since glib aborts on failure.
>
> > + goto cleanup;
> > +
> > + if (x) {
> > + if (virStrToLong_uip(x, NULL, 10, &def->x) < 0) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("cannot parse video x-resolution
> > '%s'"), x);
> > + goto cleanup;
>
> Here, you jump to cleanup on error which just returns the incomplete
> 'def' variable. I think you want to actually free 'def' and return
> NULL
> in the case of error. If you mark 'def' as an autofree variable, you
> can just return NULL here and drop the goto.
>
> > + }
> > + }
> > +
> > + if (y) {
> > + if (virStrToLong_uip(y, NULL, 10, &def->y) < 0) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("cannot parse video y-resolution
> > '%s'"), y);
> > + goto cleanup;
>
> same here.
>
> > + }
> > + }
> > +
>
> Validation question: this code accepts 0 as a valid resolution value,
> but the virDomainVideoResolutionDefFormat() function below treats 0
> as
> invalid. If x and y are both zero, the format function results in an
> empty "<resolution/>" element being printed. These functions should
> probably agree on valid values.
Oops, I sent the review just as I noticed another small issue. In this
parse function, you parse the x and the y resolution values separately,
which implies that it's valid to specify only one or the other. In
other words, this function will not report an error for the following
configuration:
<model ...>
<resolution x='800'/>
</model>
However, below in virDomainVideoDefFormat(), you refuse to format the
resolution values unless *both* x and y are non-zero. (similarly, in
the following patch, you only generate the qemu commandline arguments
if both parameters are non-zero in qemuBuildDeviceVideoStr()). If both
x and y are required, you should probably enforce that in this
function.
Jonathon
>
> > + cleanup:
> > + return def;
> > +}
> > +
> > static virDomainVideoDriverDefPtr
> > virDomainVideoDriverDefParseXML(xmlNodePtr node)
> > {
> > @@ -15424,6 +15470,7 @@
> > virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
> > }
> >
> > def->accel = virDomainVideoAccelDefParseXML(cur);
> > + def->res =
> > virDomainVideoResolutionDefParseXML(cur);
>
> It appears that def->res leaks. It should be freed in
> virDomainVideoDefClear()
>
> Thanks,
> Jonathon
>
>
> > }
> > if (virXMLNodeNameEqual(cur, "driver")) {
> > if (virDomainVirtioOptionsParseXML(cur, &def-
> > > virtio) < 0)
> > @@ -26515,6 +26562,18 @@ virDomainVideoAccelDefFormat(virBufferPtr
> > buf,
> > virBufferAddLit(buf, "/>\n");
> > }
> >
> > +static void
> > +virDomainVideoResolutionDefFormat(virBufferPtr buf,
> > + virDomainVideoResolutionDefPtr
> > def)
> > +{
> > + virBufferAddLit(buf, "<resolution");
> > + if (def->x && def->y) {
> > + virBufferAsprintf(buf, " x='%u' y='%u'",
> > + def->x, def->y);
> > + }
> > + virBufferAddLit(buf, "/>\n");
> > +}
> > +
> > static int
> > virDomainVideoDefFormat(virBufferPtr buf,
> > virDomainVideoDefPtr def,
> > @@ -26562,11 +26621,13 @@ virDomainVideoDefFormat(virBufferPtr buf,
> > virBufferAsprintf(buf, " heads='%u'", def->heads);
> > if (def->primary)
> > virBufferAddLit(buf, " primary='yes'");
> > - if (def->accel) {
> > + if (def->accel || def->res) {
> > virBufferAddLit(buf, ">\n");
> > virBufferAdjustIndent(buf, 2);
> > if (def->accel)
> > virDomainVideoAccelDefFormat(buf, def->accel);
> > + if (def->res)
> > + virDomainVideoResolutionDefFormat(buf, def->res);
> > virBufferAdjustIndent(buf, -2);
> > virBufferAddLit(buf, "</model>\n");
> > } else {
> > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> > index edac6250e4..b33e5334f4 100644
> > --- a/src/conf/domain_conf.h
> > +++ b/src/conf/domain_conf.h
> > @@ -1421,6 +1421,10 @@ struct _virDomainVideoAccelDef {
> > char *rendernode;
> > };
> >
> > +struct _virDomainVideoResolutionDef {
> > + unsigned int x;
> > + unsigned int y;
> > +};
> >
> > struct _virDomainVideoDriverDef {
> > virDomainVideoVGAConf vgaconf;
> > @@ -1438,6 +1442,7 @@ struct _virDomainVideoDef {
> > unsigned int heads;
> > bool primary;
> > virDomainVideoAccelDefPtr accel;
> > + virDomainVideoResolutionDefPtr res;
> > virDomainVideoDriverDefPtr driver;
> > virDomainDeviceInfo info;
> > virDomainVirtioOptionsPtr virtio;
> > diff --git a/src/conf/virconftypes.h b/src/conf/virconftypes.h
> > index a15cfb5f9e..462842f324 100644
> > --- a/src/conf/virconftypes.h
> > +++ b/src/conf/virconftypes.h
> > @@ -324,6 +324,9 @@ typedef virDomainVcpuDef *virDomainVcpuDefPtr;
> > typedef struct _virDomainVideoAccelDef virDomainVideoAccelDef;
> > typedef virDomainVideoAccelDef *virDomainVideoAccelDefPtr;
> >
> > +typedef struct _virDomainVideoResolutionDef
> > virDomainVideoResolutionDef;
> > +typedef virDomainVideoResolutionDef
> > *virDomainVideoResolutionDefPtr;
> > +
> > typedef struct _virDomainVideoDef virDomainVideoDef;
> > typedef virDomainVideoDef *virDomainVideoDefPtr;
> >
> > diff --git a/tests/qemuxml2argvdata/video-qxl-resolution.args
> > b/tests/qemuxml2argvdata/video-qxl-resolution.args
> > new file mode 100644
> > index 0000000000..1dbcd660f1
> > --- /dev/null
> > +++ b/tests/qemuxml2argvdata/video-qxl-resolution.args
> > @@ -0,0 +1,32 @@
> > +LC_ALL=C \
> > +PATH=/bin \
> > +HOME=/tmp/lib/domain--1-QEMUGuest1 \
> > +USER=test \
> > +LOGNAME=test \
> > +XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \
> > +XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \
> > +XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \
> > +QEMU_AUDIO_DRV=none \
> > +/usr/bin/qemu-system-i686 \
> > +-name QEMUGuest1 \
> > +-S \
> > +-machine pc,accel=tcg,usb=off,dump-guest-core=off \
> > +-m 214 \
> > +-realtime mlock=off \
> > +-smp 1,sockets=1,cores=1,threads=1 \
> > +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> > +-display none \
> > +-no-user-config \
> > +-nodefaults \
> > +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-
> > QEMUGuest1/monitor.sock,\
> > +server,nowait \
> > +-mon chardev=charmonitor,id=monitor,mode=control \
> > +-rtc base=utc \
> > +-no-shutdown \
> > +-no-acpi \
> > +-usb \
> > +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-
> > ide0-
> > 0-0 \
> > +-device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-
> > 0,bootindex=1 \
> > +-device qxl-
> > vga,id=video0,ram_size=67108864,vram_size=67108864,vgamem_mb=8,\
> > +bus=pci.0,addr=0x2 \
> > +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
> > diff --git a/tests/qemuxml2argvdata/video-qxl-resolution.xml
> > b/tests/qemuxml2argvdata/video-qxl-resolution.xml
> > new file mode 100644
> > index 0000000000..6ba2817002
> > --- /dev/null
> > +++ b/tests/qemuxml2argvdata/video-qxl-resolution.xml
> > @@ -0,0 +1,40 @@
> > +<domain type='qemu'>
> > + <name>QEMUGuest1</name>
> > + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> > + <memory unit='KiB'>219136</memory>
> > + <currentMemory unit='KiB'>219136</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-system-i686</emulator>
> > + <disk type='block' device='disk'>
> > + <driver name='qemu' type='raw'/>
> > + <source dev='/dev/HostVG/QEMUGuest1'/>
> > + <target dev='hda' bus='ide'/>
> > + <address type='drive' controller='0' bus='0' target='0'
> > unit='0'/>
> > + </disk>
> > + <controller type='usb' index='0'>
> > + <address type='pci' domain='0x0000' bus='0x00' slot='0x01'
> > function='0x2'/>
> > + </controller>
> > + <controller type='ide' index='0'>
> > + <address type='pci' domain='0x0000' bus='0x00' slot='0x01'
> > function='0x1'/>
> > + </controller>
> > + <controller type='pci' index='0' model='pci-root'/>
> > + <input type='mouse' bus='ps2'/>
> > + <input type='keyboard' bus='ps2'/>
> > + <video>
> > + <model type='qxl' ram='65536' vram='65536' vgamem='8192'
> > heads='1' primary='yes'/>
> > + <address type='pci' domain='0x0000' bus='0x00' slot='0x02'
> > function='0x0'/>
> > + </video>
> > + <memballoon model='virtio'>
> > + <address type='pci' domain='0x0000' bus='0x00' slot='0x03'
> > function='0x0'/>
> > + </memballoon>
> > + </devices>
> > +</domain>
> > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> > index 5212ce50bd..90edd7a844 100644
> > --- a/tests/qemuxml2argvtest.c
> > +++ b/tests/qemuxml2argvtest.c
> > @@ -2056,6 +2056,10 @@ mymain(void)
> > QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
> > QEMU_CAPS_DEVICE_QXL,
> > QEMU_CAPS_QXL_MAX_OUTPUTS);
> > + DO_TEST("video-qxl-resolution",
> > + QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
> > + QEMU_CAPS_DEVICE_QXL,
> > + QEMU_CAPS_QXL_VGAMEM);
> > DO_TEST("video-virtio-gpu-device",
> > QEMU_CAPS_DEVICE_VIRTIO_GPU,
> > QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
> > diff --git a/tests/qemuxml2xmloutdata/video-qxl-resolution.xml
> > b/tests/qemuxml2xmloutdata/video-qxl-resolution.xml
> > new file mode 100644
> > index 0000000000..6ba2817002
> > --- /dev/null
> > +++ b/tests/qemuxml2xmloutdata/video-qxl-resolution.xml
> > @@ -0,0 +1,40 @@
> > +<domain type='qemu'>
> > + <name>QEMUGuest1</name>
> > + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> > + <memory unit='KiB'>219136</memory>
> > + <currentMemory unit='KiB'>219136</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-system-i686</emulator>
> > + <disk type='block' device='disk'>
> > + <driver name='qemu' type='raw'/>
> > + <source dev='/dev/HostVG/QEMUGuest1'/>
> > + <target dev='hda' bus='ide'/>
> > + <address type='drive' controller='0' bus='0' target='0'
> > unit='0'/>
> > + </disk>
> > + <controller type='usb' index='0'>
> > + <address type='pci' domain='0x0000' bus='0x00' slot='0x01'
> > function='0x2'/>
> > + </controller>
> > + <controller type='ide' index='0'>
> > + <address type='pci' domain='0x0000' bus='0x00' slot='0x01'
> > function='0x1'/>
> > + </controller>
> > + <controller type='pci' index='0' model='pci-root'/>
> > + <input type='mouse' bus='ps2'/>
> > + <input type='keyboard' bus='ps2'/>
> > + <video>
> > + <model type='qxl' ram='65536' vram='65536' vgamem='8192'
> > heads='1' primary='yes'/>
> > + <address type='pci' domain='0x0000' bus='0x00' slot='0x02'
> > function='0x0'/>
> > + </video>
> > + <memballoon model='virtio'>
> > + <address type='pci' domain='0x0000' bus='0x00' slot='0x03'
> > function='0x0'/>
> > + </memballoon>
> > + </devices>
> > +</domain>
> > diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> > index b9364f942f..4c7ba98367 100644
> > --- a/tests/qemuxml2xmltest.c
> > +++ b/tests/qemuxml2xmltest.c
> > @@ -1210,6 +1210,7 @@ mymain(void)
> > QEMU_CAPS_DEVICE_QXL);
> > DO_TEST("video-qxl-heads", NONE);
> > DO_TEST("video-qxl-noheads", NONE);
> > + DO_TEST("video-qxl-resolution", NONE);
> > DO_TEST("video-virtio-gpu-secondary", NONE);
> > DO_TEST("video-virtio-gpu-ccw",
> > QEMU_CAPS_CCW,
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list