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@xxxxxxxxx wrote: > > From: Julio Faracco <jcfaracco@xxxxxxxxx> > > > > 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@xxxxxxxxx> > > --- > > 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@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list