Re: [libvirt] [PATCH v4 1/2] conf: Add 'x' and 'y' resolution into video XML definition

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(a)gmail.com wrote: > From: Julio Faracco <jcfaracco(a)gmail.com&gt; > > 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(a)gmail.com&gt; > --- > 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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list