From: Julio Faracco <jcfaracco@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@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.
</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)
+ 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;
+ }
+ }
+
+ 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;
+ }
+ }
+
+ cleanup:
+ return def;
+}
+
static virDomainVideoDriverDefPtr
virDomainVideoDriverDefParseXML(xmlNodePtr node)
{
@@ -15424,6 +15470,7 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
}
def->accel = virDomainVideoAccelDefParseXML(cur);
+ def->res = virDomainVideoResolutionDefParseXML(cur);
}
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,
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@gmail.com wrote:
> From: Julio Faracco <jcfaracco@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@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.
> + 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@gmail.com wrote:
> > From: Julio Faracco <jcfaracco@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@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@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
My apologies Jonathan, I saw your responses after I reviewed and pushed
Julio's patches. I need to remember to check my list mailbox for things
that are sitting in my inbox
On 10/17/19 3:12 PM, 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.
>
I agree with this part. I noticed it too but considering we were at v4
of the patch series I let it slide. But yes, putting the new XML in the
test suite in the first patch will demonstrate that XML parsing and
formatting is working correctly. Then when qemu_command.c changes are
added in patch #2, we see it reflected in the .args output. I prefer
this layout as well
> 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@gmail.com wrote:
>> From: Julio Faracco <jcfaracco@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@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"
>
Yes I agree that sounds more natural.
>
>> </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.
>
>> + 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()
>
Indeed, good catch on all the above. Who volunteers for the follow up
patch? :)
Thanks,
Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
At least, I need to fix the leak problem. ;-)
I can grab other problems too.
I will submit a fix/patch soon.
--
Julio Faracco
Em qui, 17 de out de 2019 às 17:33, Cole Robinson
<crobinso@redhat.com> escreveu:
>
> My apologies Jonathan, I saw your responses after I reviewed and pushed
> Julio's patches. I need to remember to check my list mailbox for things
> that are sitting in my inbox
>
> On 10/17/19 3:12 PM, 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.
> >
>
> I agree with this part. I noticed it too but considering we were at v4
> of the patch series I let it slide. But yes, putting the new XML in the
> test suite in the first patch will demonstrate that XML parsing and
> formatting is working correctly. Then when qemu_command.c changes are
> added in patch #2, we see it reflected in the .args output. I prefer
> this layout as well
>
> > 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@gmail.com wrote:
> >> From: Julio Faracco <jcfaracco@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@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"
> >
>
> Yes I agree that sounds more natural.
>
> >
> >> </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.
> >
> >> + 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()
> >
>
> Indeed, good catch on all the above. Who volunteers for the follow up
> patch? :)
>
> Thanks,
> Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Oct 17, 2019 at 12:37 PM <jcfaracco@gmail.com> wrote:
> From: Julio Faracco <jcfaracco@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>
>
Please mention that it could fix the bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1485793
>
> Signed-off-by: Julio Faracco <jcfaracco@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.
> </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)
> + 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;
> + }
> + }
> +
> + 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;
> + }
> + }
> +
> + cleanup:
> + return def;
> +}
> +
> static virDomainVideoDriverDefPtr
> virDomainVideoDriverDefParseXML(xmlNodePtr node)
> {
> @@ -15424,6 +15470,7 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr
> xmlopt,
> }
>
> def->accel = virDomainVideoAccelDefParseXML(cur);
> + def->res = virDomainVideoResolutionDefParseXML(cur);
> }
> 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,
> --
> 2.20.1
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
Best regards,
-----------------------------------
Han Han
Quality Engineer
Redhat.
Email: hhan@redhat.com
Phone: +861065339333
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.