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

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