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@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"
>
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
