12:46 p.m.
This is a follow-up series responding to some comments from Jan Tomko. Most
importantly, the fact that the errors are not propagated up to the caller,
they're only logged. To fix this, the function signatures were changed to
return a error status.
The patch series was also re-ordered slightly to improve readability (I hope).
Jonathon Jongsma (5):
qemu: fix documentation for video resolution
conf: remove unnecessary NULL checks
conf: use glib allocation when parsing video props
conf: report errors when parsing video resolution
conf: report errors when parsing video acceleration
docs/formatdomain.html.in | 12 +++--
src/conf/domain_conf.c | 108 ++++++++++++++++++++++----------------
2 files changed, 71 insertions(+), 49 deletions(-)
--
2.21.0
12:46 p.m.
New subject: [libvirt] [PATCH v2 1/5] qemu: fix documentation for video resolution
The video resolution support that was introduced in
7286279797a34b3083d85bc4556432b5e7ad9fff is specified as a <resolution>
sub-element of <model>, not optional attributes of model.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
docs/formatdomain.html.in | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 962766b792..7cc9ada897 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -7077,10 +7077,14 @@ 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. 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.
+ extends secondary bar and makes it addressable as 64bit memory.
+ </p>
+ <p><span class="since">Since 5.9.0</span>, the
<code>model</code>
+ element may also have an optional <code>resolution</code>
sub-element.
+ The <code>resolution</code> element has attributes
<code>x</code> and
+ <code>y</code> to set the minimum resolution for the video device.
This
+ sub-element is valid for model types "vga", "qxl",
"bochs", and
+ "virtio".
</p>
</dd>
--
2.21.0
12:46 p.m.
New subject: [libvirt] [PATCH v2 2/5] conf: remove unnecessary NULL checks
Just above in the function, we return from the function if either x or y
are NULL, so there's no need to re-check whether x or y are NULL.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/conf/domain_conf.c | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9ee9b44a5e..ad1d3ffea6 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15365,20 +15365,16 @@ virDomainVideoResolutionDefParseXML(xmlNodePtr node)
if (VIR_ALLOC(def) < 0)
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;
- }
+ if (virStrToLong_uip(x, NULL, 10, &def->x) < 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("cannot parse video x-resolution '%s'"), x);
+ goto cleanup;
}
- 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;
- }
+ if (virStrToLong_uip(y, NULL, 10, &def->y) < 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("cannot parse video y-resolution '%s'"), y);
+ goto cleanup;
}
cleanup:
--
2.21.0
12:46 p.m.
New subject: [libvirt] [PATCH v2 3/5] conf: use glib allocation when parsing video props
In preparation for some other improvements, switch to using glib
allocation and g_autofree when parsing the 'acceleration' and
'resolution' properties of the video device.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/conf/domain_conf.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index ad1d3ffea6..269a6ec2c3 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15289,7 +15289,7 @@ static virDomainVideoAccelDefPtr
virDomainVideoAccelDefParseXML(xmlNodePtr node)
{
xmlNodePtr cur;
- virDomainVideoAccelDefPtr def;
+ g_autofree virDomainVideoAccelDefPtr def = NULL;
int val;
g_autofree char *accel2d = NULL;
g_autofree char *accel3d = NULL;
@@ -15311,8 +15311,7 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
if (!accel3d && !accel2d && !rendernode)
return NULL;
- if (VIR_ALLOC(def) < 0)
- goto cleanup;
+ def = g_new0(virDomainVideoAccelDef, 1);
if (accel3d) {
if ((val = virTristateBoolTypeFromString(accel3d)) <= 0) {
@@ -15336,14 +15335,14 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
def->rendernode = virFileSanitizePath(rendernode);
cleanup:
- return def;
+ return g_steal_pointer(&def);
}
static virDomainVideoResolutionDefPtr
virDomainVideoResolutionDefParseXML(xmlNodePtr node)
{
xmlNodePtr cur;
- virDomainVideoResolutionDefPtr def;
+ g_autofree virDomainVideoResolutionDefPtr def = NULL;
g_autofree char *x = NULL;
g_autofree char *y = NULL;
@@ -15362,8 +15361,7 @@ virDomainVideoResolutionDefParseXML(xmlNodePtr node)
if (!x || !y)
return NULL;
- if (VIR_ALLOC(def) < 0)
- goto cleanup;
+ def = g_new0(virDomainVideoResolutionDef, 1);
if (virStrToLong_uip(x, NULL, 10, &def->x) < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -15378,7 +15376,7 @@ virDomainVideoResolutionDefParseXML(xmlNodePtr node)
}
cleanup:
- return def;
+ return g_steal_pointer(&def);
}
static virDomainVideoDriverDefPtr
--
2.21.0
12:46 p.m.
New subject: [libvirt] [PATCH v2 4/5] conf: report errors when parsing video resolution
The current code doesn't properly handle errors when parsing a video
device's resolution.
This patch changes the parse function signature to return an error
when we're missing an 'x' or 'y' parameter or when the 'x' or
'y'
parameters are not positive integers. No error is returned when no
'resolution' element is found.
Previously we were returning a NULL structure for the case where 'x' or
'y' were missing, but were returning an under-specified structure for
the other error cases.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/conf/domain_conf.c | 44 ++++++++++++++++++++++++++++--------------
1 file changed, 30 insertions(+), 14 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 269a6ec2c3..b3f32d0b63 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15338,45 +15338,57 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
return g_steal_pointer(&def);
}
-static virDomainVideoResolutionDefPtr
-virDomainVideoResolutionDefParseXML(xmlNodePtr node)
+static int
+virDomainVideoResolutionDefParseXML(xmlNodePtr node,
+ virDomainVideoResolutionDefPtr *res)
{
xmlNodePtr cur;
g_autofree virDomainVideoResolutionDefPtr def = NULL;
g_autofree char *x = NULL;
g_autofree char *y = NULL;
+ *res = 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");
- }
+ if ((cur->type == XML_ELEMENT_NODE) &&
+ virXMLNodeNameEqual(cur, "resolution")) {
+ x = virXMLPropString(cur, "x");
+ y = virXMLPropString(cur, "y");
+ break;
}
cur = cur->next;
}
+ /* resolution not specified */
+ if (cur == NULL)
+ return 0;
+
+ /* resolution specified, but x or y is missing. report error */
if (!x || !y)
- return NULL;
+ return -1;
def = g_new0(virDomainVideoResolutionDef, 1);
if (virStrToLong_uip(x, NULL, 10, &def->x) < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("cannot parse video x-resolution '%s'"), x);
- goto cleanup;
+ return -1;
}
if (virStrToLong_uip(y, NULL, 10, &def->y) < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("cannot parse video y-resolution '%s'"), y);
- goto cleanup;
+ return -1;
}
- cleanup:
- return g_steal_pointer(&def);
+ if (def->x == 0 || def->y == 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("resolution values must be greater than 0"));
+ return -1;
+ }
+
+ *res = g_steal_pointer(&def);
+ return 0;
}
static virDomainVideoDriverDefPtr
@@ -15458,7 +15470,11 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
}
def->accel = virDomainVideoAccelDefParseXML(cur);
- def->res = virDomainVideoResolutionDefParseXML(cur);
+ if (virDomainVideoResolutionDefParseXML(cur, &def->res) < 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ "%s", _("Cannot parse video
resolution"));
+ goto error;
+ }
}
if (virXMLNodeNameEqual(cur, "driver")) {
if (virDomainVirtioOptionsParseXML(cur, &def->virtio) < 0)
--
2.21.0
12:54 p.m.
New subject: [libvirt] [PATCH v2 4/5] conf: report errors when parsing video resolution
I pushed the first three patches. Comments below
On 10/23/19 1:46 PM, Jonathon Jongsma wrote:
The current code doesn't properly handle errors when parsing a
video
device's resolution.
This patch changes the parse function signature to return an error
when we're missing an 'x' or 'y' parameter or when the 'x' or
'y'
parameters are not positive integers. No error is returned when no
'resolution' element is found.
Previously we were returning a NULL structure for the case where 'x' or
'y' were missing, but were returning an under-specified structure for
the other error cases.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/conf/domain_conf.c | 44 ++++++++++++++++++++++++++++--------------
1 file changed, 30 insertions(+), 14 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 269a6ec2c3..b3f32d0b63 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15338,45 +15338,57 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
return g_steal_pointer(&def);
}
-static virDomainVideoResolutionDefPtr
-virDomainVideoResolutionDefParseXML(xmlNodePtr node)
+static int
+virDomainVideoResolutionDefParseXML(xmlNodePtr node,
+ virDomainVideoResolutionDefPtr *res)
{
xmlNodePtr cur;
g_autofree virDomainVideoResolutionDefPtr def = NULL;
g_autofree char *x = NULL;
g_autofree char *y = NULL;
+ *res = 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");
- }
+ if ((cur->type == XML_ELEMENT_NODE) &&
+ virXMLNodeNameEqual(cur, "resolution")) {
+ x = virXMLPropString(cur, "x");
+ y = virXMLPropString(cur, "y");
+ break;
}
cur = cur->next;
}
This loop can be removed if you move the virXMLNodeNameEqual to the
parent function, like how it is handled for parent call of
virDomainVirtioOptionsParseXML
+ /* resolution not specified */
+ if (cur == NULL)
+ return 0;
+
+ /* resolution specified, but x or y is missing. report error */
if (!x || !y)
- return NULL;
+ return -1;
def = g_new0(virDomainVideoResolutionDef, 1);
if (virStrToLong_uip(x, NULL, 10, &def->x) < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("cannot parse video x-resolution '%s'"), x);
- goto cleanup;
+ return -1;
}
if (virStrToLong_uip(y, NULL, 10, &def->y) < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("cannot parse video y-resolution '%s'"), y);
- goto cleanup;
+ return -1;
}
- cleanup:
- return g_steal_pointer(&def);
+ if (def->x == 0 || def->y == 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("resolution values must be greater than 0"));
+ return -1;
+ }
+
+ *res = g_steal_pointer(&def);
+ return 0;
}
This patch is doing two things: converting to the more common error
handling pattern, but also adding this error check. Separating them will
help review.
It's borderline pedantic but this type of error should actually be in
the Validate callback machinery instead. Some drivers will fill in a
virDomainDef manually in code (like virtualbox). If they accidentally
set an x or y value of 0, Validate won't catch it as an error, but the
XML parser will catch it later. Generally the XML parser should only
throw errors about
static virDomainVideoDriverDefPtr
@@ -15458,7 +15470,11 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
}
def->accel = virDomainVideoAccelDefParseXML(cur);
- def->res = virDomainVideoResolutionDefParseXML(cur);
+ if (virDomainVideoResolutionDefParseXML(cur, &def->res) < 0)
{
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ "%s", _("Cannot parse video
resolution"));
+ goto error;
+ }
}
Calling virReporError here will overwrite the error raised by
virDomainVideoResolutionDefParseXML. The error reporting machinery
doesn't have any smarts to merge errors or anything like that, it needs
to be done manually. In this case you can just drop the virReportError
entirely
I'll be quicker about reviewing v3.
Thanks,
Cole
4:41 p.m.
New subject: [libvirt] [PATCH v2 4/5] conf: report errors when parsing video resolution
On Wed, 2019-11-13 at 13:54 -0500, Cole Robinson wrote:
I pushed the first three patches. Comments below
On 10/23/19 1:46 PM, Jonathon Jongsma wrote:
> The current code doesn't properly handle errors when parsing a
> video
> device's resolution.
>
> This patch changes the parse function signature to return an error
> when we're missing an 'x' or 'y' parameter or when the
'x' or 'y'
> parameters are not positive integers. No error is returned when no
> 'resolution' element is found.
>
> Previously we were returning a NULL structure for the case where
> 'x' or
> 'y' were missing, but were returning an under-specified structure
> for
> the other error cases.
>
> Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
> ---
> src/conf/domain_conf.c | 44 ++++++++++++++++++++++++++++--------
> ------
> 1 file changed, 30 insertions(+), 14 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 269a6ec2c3..b3f32d0b63 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -15338,45 +15338,57 @@ virDomainVideoAccelDefParseXML(xmlNodePtr
> node)
> return g_steal_pointer(&def);
> }
>
> -static virDomainVideoResolutionDefPtr
> -virDomainVideoResolutionDefParseXML(xmlNodePtr node)
> +static int
> +virDomainVideoResolutionDefParseXML(xmlNodePtr node,
> + virDomainVideoResolutionDefPtr
> *res)
> {
> xmlNodePtr cur;
> g_autofree virDomainVideoResolutionDefPtr def = NULL;
> g_autofree char *x = NULL;
> g_autofree char *y = NULL;
>
> + *res = 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");
> - }
> + if ((cur->type == XML_ELEMENT_NODE) &&
> + virXMLNodeNameEqual(cur, "resolution")) {
> + x = virXMLPropString(cur, "x");
> + y = virXMLPropString(cur, "y");
> + break;
> }
> cur = cur->next;
> }
>
This loop can be removed if you move the virXMLNodeNameEqual to the
parent function, like how it is handled for parent call of
virDomainVirtioOptionsParseXML
> + /* resolution not specified */
> + if (cur == NULL)
> + return 0;
> +
> + /* resolution specified, but x or y is missing. report error
> */
> if (!x || !y)
> - return NULL;
> + return -1;
>
> def = g_new0(virDomainVideoResolutionDef, 1);
>
> if (virStrToLong_uip(x, NULL, 10, &def->x) < 0) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("cannot parse video x-resolution
'%s'"),
> x);
> - goto cleanup;
> + return -1;
> }
>
> if (virStrToLong_uip(y, NULL, 10, &def->y) < 0) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("cannot parse video y-resolution
'%s'"),
> y);
> - goto cleanup;
> + return -1;
> }
>
> - cleanup:
> - return g_steal_pointer(&def);
> + if (def->x == 0 || def->y == 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("resolution values must be greater than
> 0"));
> + return -1;
> + }
> +
> + *res = g_steal_pointer(&def);
> + return 0;
> }
>
This patch is doing two things: converting to the more common error
handling pattern, but also adding this error check. Separating them
will
help review.
It's borderline pedantic but this type of error should actually be in
the Validate callback machinery instead. Some drivers will fill in a
virDomainDef manually in code (like virtualbox). If they accidentally
set an x or y value of 0, Validate won't catch it as an error, but
the
XML parser will catch it later. Generally the XML parser should only
throw errors about
It seems that this sentence is unfinished?
> static virDomainVideoDriverDefPtr
> @@ -15458,7 +15470,11 @@
> virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
> }
>
> def->accel = virDomainVideoAccelDefParseXML(cur);
> - def->res =
> virDomainVideoResolutionDefParseXML(cur);
> + if (virDomainVideoResolutionDefParseXML(cur, &def-
> >res) < 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + "%s", _("Cannot parse video
> resolution"));
> + goto error;
> + }
> }
Calling virReporError here will overwrite the error raised by
virDomainVideoResolutionDefParseXML. The error reporting machinery
doesn't have any smarts to merge errors or anything like that, it
needs
to be done manually. In this case you can just drop the
virReportError
entirely
I'll be quicker about reviewing v3.
Thanks,
Cole
7:02 a.m.
New subject: [libvirt] [PATCH v2 4/5] conf: report errors when parsing video resolution
On 11/13/19 5:41 PM, Jonathon Jongsma wrote:
On Wed, 2019-11-13 at 13:54 -0500, Cole Robinson wrote:
> I pushed the first three patches. Comments below
>
> On 10/23/19 1:46 PM, Jonathon Jongsma wrote:
>> The current code doesn't properly handle errors when parsing a
>> video
>> device's resolution.
>>
>> This patch changes the parse function signature to return an error
>> when we're missing an 'x' or 'y' parameter or when the
'x' or 'y'
>> parameters are not positive integers. No error is returned when no
>> 'resolution' element is found.
>>
>> Previously we were returning a NULL structure for the case where
>> 'x' or
>> 'y' were missing, but were returning an under-specified structure
>> for
>> the other error cases.
>>
>> Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
>> ---
>> src/conf/domain_conf.c | 44 ++++++++++++++++++++++++++++--------
>> ------
>> 1 file changed, 30 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 269a6ec2c3..b3f32d0b63 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -15338,45 +15338,57 @@ virDomainVideoAccelDefParseXML(xmlNodePtr
>> node)
>> return g_steal_pointer(&def);
>> }
>>
>> -static virDomainVideoResolutionDefPtr
>> -virDomainVideoResolutionDefParseXML(xmlNodePtr node)
>> +static int
>> +virDomainVideoResolutionDefParseXML(xmlNodePtr node,
>> + virDomainVideoResolutionDefPtr
>> *res)
>> {
>> xmlNodePtr cur;
>> g_autofree virDomainVideoResolutionDefPtr def = NULL;
>> g_autofree char *x = NULL;
>> g_autofree char *y = NULL;
>>
>> + *res = 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");
>> - }
>> + if ((cur->type == XML_ELEMENT_NODE) &&
>> + virXMLNodeNameEqual(cur, "resolution")) {
>> + x = virXMLPropString(cur, "x");
>> + y = virXMLPropString(cur, "y");
>> + break;
>> }
>> cur = cur->next;
>> }
>>
>
> This loop can be removed if you move the virXMLNodeNameEqual to the
> parent function, like how it is handled for parent call of
> virDomainVirtioOptionsParseXML
>
>> + /* resolution not specified */
>> + if (cur == NULL)
>> + return 0;
>> +
>> + /* resolution specified, but x or y is missing. report error
>> */
>> if (!x || !y)
>> - return NULL;
>> + return -1;
>>
>> def = g_new0(virDomainVideoResolutionDef, 1);
>>
>> if (virStrToLong_uip(x, NULL, 10, &def->x) < 0) {
>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> _("cannot parse video x-resolution
'%s'"),
>> x);
>> - goto cleanup;
>> + return -1;
>> }
>>
>> if (virStrToLong_uip(y, NULL, 10, &def->y) < 0) {
>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> _("cannot parse video y-resolution
'%s'"),
>> y);
>> - goto cleanup;
>> + return -1;
>> }
>>
>> - cleanup:
>> - return g_steal_pointer(&def);
>> + if (def->x == 0 || def->y == 0) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("resolution values must be greater than
>> 0"));
>> + return -1;
>> + }
>> +
>> + *res = g_steal_pointer(&def);
>> + return 0;
>> }
>>
>
> This patch is doing two things: converting to the more common error
> handling pattern, but also adding this error check. Separating them
> will
> help review.
>
> It's borderline pedantic but this type of error should actually be in
> the Validate callback machinery instead. Some drivers will fill in a
> virDomainDef manually in code (like virtualbox). If they accidentally
> set an x or y value of 0, Validate won't catch it as an error, but
> the
> XML parser will catch it later. Generally the XML parser should only
> throw errors about
It seems that this sentence is unfinished?
I meant to delete that unformed thought because it was going to turn
into more than a sentence and would distract from this patch :)
Generally the XML parser should only raise errors that are specific to
the XML input. Anything that's validating a value after it has already
been parsed is better served in the Validate callbacks. The existing
code is not the best guide for this though, it's just the goal we should
shoot for IMO
- Cole
12:46 p.m.
New subject: [libvirt] [PATCH v2 5/5] conf: report errors when parsing video acceleration
In order to differentiate between a configuration that does not specify
any video acceleration and one that is incorrectly specified, change the
signature of virDomainVideoAccelDefParseXML() to return an error
response.
If any of the values are invalid, report an error rather than returning
a partially-specified accel object. If no 'acceleration' node is found,
do not report an error since it is optional.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/conf/domain_conf.c | 38 +++++++++++++++++++++++---------------
1 file changed, 23 insertions(+), 15 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b3f32d0b63..1c3fc5c4ce 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15285,8 +15285,8 @@ virDomainVideoDefaultType(const virDomainDef *def)
}
}
-static virDomainVideoAccelDefPtr
-virDomainVideoAccelDefParseXML(xmlNodePtr node)
+static int
+virDomainVideoAccelDefParseXML(xmlNodePtr node, virDomainVideoAccelDefPtr *accel)
{
xmlNodePtr cur;
g_autofree virDomainVideoAccelDefPtr def = NULL;
@@ -15295,21 +15295,25 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
g_autofree char *accel3d = NULL;
g_autofree char *rendernode = NULL;
+ *accel = NULL;
cur = node->children;
while (cur != NULL) {
- if (cur->type == XML_ELEMENT_NODE) {
- if (!accel3d && !accel2d &&
- virXMLNodeNameEqual(cur, "acceleration")) {
- accel3d = virXMLPropString(cur, "accel3d");
- accel2d = virXMLPropString(cur, "accel2d");
- rendernode = virXMLPropString(cur, "rendernode");
- }
+ if ((cur->type == XML_ELEMENT_NODE) &&
+ virXMLNodeNameEqual(cur, "acceleration")) {
+ accel3d = virXMLPropString(cur, "accel3d");
+ accel2d = virXMLPropString(cur, "accel2d");
+ rendernode = virXMLPropString(cur, "rendernode");
+ break;
}
cur = cur->next;
}
+ /* no acceleration node was specified */
+ if (cur == NULL)
+ return 0;
+
if (!accel3d && !accel2d && !rendernode)
- return NULL;
+ return -1;
def = g_new0(virDomainVideoAccelDef, 1);
@@ -15317,7 +15321,7 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
if ((val = virTristateBoolTypeFromString(accel3d)) <= 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unknown accel3d value '%s'"), accel3d);
- goto cleanup;
+ return -1;
}
def->accel3d = val;
}
@@ -15326,7 +15330,7 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
if ((val = virTristateBoolTypeFromString(accel2d)) <= 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unknown accel2d value '%s'"), accel2d);
- goto cleanup;
+ return -1;
}
def->accel2d = val;
}
@@ -15334,8 +15338,8 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
if (rendernode)
def->rendernode = virFileSanitizePath(rendernode);
- cleanup:
- return g_steal_pointer(&def);
+ *accel = g_steal_pointer(&def);
+ return 0;
}
static int
@@ -15469,7 +15473,11 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
VIR_FREE(primary);
}
- def->accel = virDomainVideoAccelDefParseXML(cur);
+ if (virDomainVideoAccelDefParseXML(cur, &def->accel) < 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ "%s", _("Cannot parse video
acceleration"));
+ goto error;
+ }
if (virDomainVideoResolutionDefParseXML(cur, &def->res) < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
"%s", _("Cannot parse video
resolution"));
--
2.21.0
12:56 p.m.
New subject: [libvirt] [PATCH v2 5/5] conf: report errors when parsing video acceleration
On 10/23/19 1:46 PM, Jonathon Jongsma wrote:
In order to differentiate between a configuration that does not
specify
any video acceleration and one that is incorrectly specified, change the
signature of virDomainVideoAccelDefParseXML() to return an error
response.
If any of the values are invalid, report an error rather than returning
a partially-specified accel object. If no 'acceleration' node is found,
do not report an error since it is optional.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/conf/domain_conf.c | 38 +++++++++++++++++++++++---------------
1 file changed, 23 insertions(+), 15 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b3f32d0b63..1c3fc5c4ce 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15285,8 +15285,8 @@ virDomainVideoDefaultType(const virDomainDef *def)
}
}
-static virDomainVideoAccelDefPtr
-virDomainVideoAccelDefParseXML(xmlNodePtr node)
+static int
+virDomainVideoAccelDefParseXML(xmlNodePtr node, virDomainVideoAccelDefPtr *accel)
{
xmlNodePtr cur;
g_autofree virDomainVideoAccelDefPtr def = NULL;
@@ -15295,21 +15295,25 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
g_autofree char *accel3d = NULL;
g_autofree char *rendernode = NULL;
+ *accel = NULL;
cur = node->children;
while (cur != NULL) {
- if (cur->type == XML_ELEMENT_NODE) {
- if (!accel3d && !accel2d &&
- virXMLNodeNameEqual(cur, "acceleration")) {
- accel3d = virXMLPropString(cur, "accel3d");
- accel2d = virXMLPropString(cur, "accel2d");
- rendernode = virXMLPropString(cur, "rendernode");
- }
+ if ((cur->type == XML_ELEMENT_NODE) &&
+ virXMLNodeNameEqual(cur, "acceleration")) {
+ accel3d = virXMLPropString(cur, "accel3d");
+ accel2d = virXMLPropString(cur, "accel2d");
+ rendernode = virXMLPropString(cur, "rendernode");
+ break;
}
cur = cur->next;
}
THe virXMLNodeNameEqual check should be moved to the parent, same as
mentioned in patch 4
+ /* no acceleration node was specified */
+ if (cur == NULL)
+ return 0;
+
if (!accel3d && !accel2d && !rendernode)
- return NULL;
+ return -1;
def = g_new0(virDomainVideoAccelDef, 1);
@@ -15317,7 +15321,7 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
if ((val = virTristateBoolTypeFromString(accel3d)) <= 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unknown accel3d value '%s'"), accel3d);
- goto cleanup;
+ return -1;
}
def->accel3d = val;
}
@@ -15326,7 +15330,7 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
if ((val = virTristateBoolTypeFromString(accel2d)) <= 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unknown accel2d value '%s'"), accel2d);
- goto cleanup;
+ return -1;
}
def->accel2d = val;
}
@@ -15334,8 +15338,8 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
if (rendernode)
def->rendernode = virFileSanitizePath(rendernode);
- cleanup:
- return g_steal_pointer(&def);
+ *accel = g_steal_pointer(&def);
+ return 0;
}
static int
@@ -15469,7 +15473,11 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
VIR_FREE(primary);
}
- def->accel = virDomainVideoAccelDefParseXML(cur);
+ if (virDomainVideoAccelDefParseXML(cur, &def->accel) < 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ "%s", _("Cannot parse video
acceleration"));
+ goto error;
+ }
if (virDomainVideoResolutionDefParseXML(cur, &def->res) < 0)
{
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
"%s", _("Cannot parse video
resolution"));
Same error overwriting issue here as in patch 4
- Cole
1834
days inactive
1856
days old
9 comments
2 participants
participants (2)
-
Cole Robinson -
Jonathon Jongsma