[libvirt] [PATCH v4 1/2] conf: Add 'x' and 'y' resolution into video XML definition
Julio Faracco
jcfaracco at gmail.com
Fri Oct 18 01:37:51 UTC 2019
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 at 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 at gmail.com wrote:
> >> From: Julio Faracco <jcfaracco at 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 at 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
More information about the libvir-list
mailing list