[libvirt] [PATCH v4 0/2] Add resolution properties for QEMU video devices
From: Julio Faracco <jcfaracco@gmail.com> This serie adds resolution ('x' and 'y') properties into XML definition for QEMU video devices to specify a default resolution. This specific case is not considering those attributes as a QEMU capabilities due to complexity of code versus test complexity versus a real gain. This skeleton would work very well initially. After, it should be possible to include them as a capabilities without changing this serie. v1-v2: Adds suggestions of multiple members. v2-v3: Adds Cole's suggestions. v3-v4: Adds Cole's suggestions again. Julio Faracco (2): conf: Add 'x' and 'y' resolution into video XML definition qemu: Generate 'xres' and 'yres' for QEMU video devices 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 + src/qemu/qemu_command.c | 5 ++ src/qemu/qemu_domain.c | 11 ++++ .../video-qxl-resolution.args | 32 ++++++++++ .../qemuxml2argvdata/video-qxl-resolution.xml | 42 +++++++++++++ tests/qemuxml2argvtest.c | 4 ++ .../video-qxl-resolution.xml | 42 +++++++++++++ tests/qemuxml2xmltest.c | 1 + 12 files changed, 221 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 -- 2.20.1
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
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,
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
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
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
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
From: Julio Faracco <jcfaracco@gmail.com> This commit let QEMU command line define 'xres' and 'yres' properties if XML contains both properties from video model: based on resolution fields 'x' and 'y'. There is a conditional structure inside qemuDomainDeviceDefValidateVideo() that validates if video model supports this feature. This commit includes the necessary changes to cover resolution for 'video-qxl-resolution' test cases too. Signed-off-by: Julio Faracco <jcfaracco@gmail.com> --- src/qemu/qemu_command.c | 5 +++++ src/qemu/qemu_domain.c | 11 +++++++++++ tests/qemuxml2argvdata/video-qxl-resolution.args | 2 +- tests/qemuxml2argvdata/video-qxl-resolution.xml | 4 +++- tests/qemuxml2xmloutdata/video-qxl-resolution.xml | 4 +++- 5 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e98195b1d7..b579b994f3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4598,6 +4598,11 @@ qemuBuildDeviceVideoStr(const virDomainDef *def, virBufferAsprintf(&buf, ",vgamem=%uk", video->vram); } + if (video->res && video->res->x && video->res->y) { + /* QEMU accepts resolution xres and yres. */ + virBufferAsprintf(&buf, ",xres=%u,yres=%u", video->res->x, video->res->y); + } + if (qemuBuildDeviceAddressStr(&buf, def, &video->info, qemuCaps) < 0) return NULL; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 09b6c9a570..a97bf65e7f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5774,6 +5774,17 @@ qemuDomainDeviceDefValidateVideo(const virDomainVideoDef *video) } } + if (video->type != VIR_DOMAIN_VIDEO_TYPE_VGA && + video->type != VIR_DOMAIN_VIDEO_TYPE_QXL && + video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO && + video->type != VIR_DOMAIN_VIDEO_TYPE_BOCHS) { + if (video->res) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("model resolution is not supported")); + return -1; + } + } + if (video->type == VIR_DOMAIN_VIDEO_TYPE_VGA || video->type == VIR_DOMAIN_VIDEO_TYPE_VMVGA) { if (video->vram && video->vram < 1024) { diff --git a/tests/qemuxml2argvdata/video-qxl-resolution.args b/tests/qemuxml2argvdata/video-qxl-resolution.args index 1dbcd660f1..ed2e4422da 100644 --- a/tests/qemuxml2argvdata/video-qxl-resolution.args +++ b/tests/qemuxml2argvdata/video-qxl-resolution.args @@ -28,5 +28,5 @@ server,nowait \ -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 \ +xres=1280,yres=720,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 index 6ba2817002..1bc415f3f8 100644 --- a/tests/qemuxml2argvdata/video-qxl-resolution.xml +++ b/tests/qemuxml2argvdata/video-qxl-resolution.xml @@ -30,7 +30,9 @@ <input type='mouse' bus='ps2'/> <input type='keyboard' bus='ps2'/> <video> - <model type='qxl' ram='65536' vram='65536' vgamem='8192' heads='1' primary='yes'/> + <model type='qxl' ram='65536' vram='65536' vgamem='8192' heads='1' primary='yes'> + <resolution x='1280' y='720'/> + </model> <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> </video> <memballoon model='virtio'> diff --git a/tests/qemuxml2xmloutdata/video-qxl-resolution.xml b/tests/qemuxml2xmloutdata/video-qxl-resolution.xml index 6ba2817002..1bc415f3f8 100644 --- a/tests/qemuxml2xmloutdata/video-qxl-resolution.xml +++ b/tests/qemuxml2xmloutdata/video-qxl-resolution.xml @@ -30,7 +30,9 @@ <input type='mouse' bus='ps2'/> <input type='keyboard' bus='ps2'/> <video> - <model type='qxl' ram='65536' vram='65536' vgamem='8192' heads='1' primary='yes'/> + <model type='qxl' ram='65536' vram='65536' vgamem='8192' heads='1' primary='yes'> + <resolution x='1280' y='720'/> + </model> <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> </video> <memballoon model='virtio'> -- 2.20.1
On 10/17/19 12:30 AM, jcfaracco@gmail.com wrote:
From: Julio Faracco <jcfaracco@gmail.com>
This serie adds resolution ('x' and 'y') properties into XML definition for QEMU video devices to specify a default resolution. This specific case is not considering those attributes as a QEMU capabilities due to complexity of code versus test complexity versus a real gain. This skeleton would work very well initially. After, it should be possible to include them as a capabilities without changing this serie.
v1-v2: Adds suggestions of multiple members. v2-v3: Adds Cole's suggestions. v3-v4: Adds Cole's suggestions again.
Reviewed-by: Cole Robinson <crobinso@redhat.com> and pushed. Thanks for your patience! - Cole
Julio Faracco (2): conf: Add 'x' and 'y' resolution into video XML definition qemu: Generate 'xres' and 'yres' for QEMU video devices
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 + src/qemu/qemu_command.c | 5 ++ src/qemu/qemu_domain.c | 11 ++++ .../video-qxl-resolution.args | 32 ++++++++++ .../qemuxml2argvdata/video-qxl-resolution.xml | 42 +++++++++++++ tests/qemuxml2argvtest.c | 4 ++ .../video-qxl-resolution.xml | 42 +++++++++++++ tests/qemuxml2xmltest.c | 1 + 12 files changed, 221 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
- Cole
I usually don't bother list with ordinary messages, but... Thank you very much for your guidance and patience, Cole! I appreciate it! Em qui, 17 de out de 2019 às 17:25, Cole Robinson <crobinso@redhat.com> escreveu:
On 10/17/19 12:30 AM, jcfaracco@gmail.com wrote:
From: Julio Faracco <jcfaracco@gmail.com>
This serie adds resolution ('x' and 'y') properties into XML definition for QEMU video devices to specify a default resolution. This specific case is not considering those attributes as a QEMU capabilities due to complexity of code versus test complexity versus a real gain. This skeleton would work very well initially. After, it should be possible to include them as a capabilities without changing this serie.
v1-v2: Adds suggestions of multiple members. v2-v3: Adds Cole's suggestions. v3-v4: Adds Cole's suggestions again.
Reviewed-by: Cole Robinson <crobinso@redhat.com>
and pushed. Thanks for your patience!
- Cole
Julio Faracco (2): conf: Add 'x' and 'y' resolution into video XML definition qemu: Generate 'xres' and 'yres' for QEMU video devices
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 + src/qemu/qemu_command.c | 5 ++ src/qemu/qemu_domain.c | 11 ++++ .../video-qxl-resolution.args | 32 ++++++++++ .../qemuxml2argvdata/video-qxl-resolution.xml | 42 +++++++++++++ tests/qemuxml2argvtest.c | 4 ++ .../video-qxl-resolution.xml | 42 +++++++++++++ tests/qemuxml2xmltest.c | 1 + 12 files changed, 221 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
- Cole
participants (5)
-
Cole Robinson -
Han Han -
jcfaracco＠gmail.com -
Jonathon Jongsma -
Julio Faracco