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()