3:59 p.m.
This is the third version of this patch series after a review from Cole. The
main changes to this version of the patch series are:
- several patches have been pushed upstream (and therefore dropped from this
series
- The iteration over child nodes was moved up to the caller due to a
suggestion from Cole (much better, thanks)
- Because the parse functions are now only called when the appropriate XML
element is present, the parse functions no longer need to return
"successful" NULL for the case where the element is not specified in the
XML. Therefore we do not need to change the function signatures and we can
simply treat a NULL return value as an error.
- the resolution validation is moved to virDomainVideoDefValidate().
Jonathon Jongsma (4):
conf: iterate video model children in parent function
conf: report errors when parsing video resolution
conf: report errors when parsing video acceleration
conf: validate video resolution
src/conf/domain_conf.c | 75 ++++++++++++++++++++++--------------------
1 file changed, 40 insertions(+), 35 deletions(-)
--
2.21.0
3:59 p.m.
New subject: [libvirt] [PATCH v3 1/4] conf: iterate video model children in parent function
Previously, we were passing the video "model" node to the
"acceleration"
and "resolution" parsing functions and requiring them to iterate over
the children to discover and parse the appropriate node. It makes more
sense to move this responsibility up to the parent function and just
pass these functions the node that needs to be parsed.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/conf/domain_conf.c | 45 +++++++++++++++++-------------------------
1 file changed, 18 insertions(+), 27 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 561e25ff6e..5c86729258 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15259,25 +15259,15 @@ virDomainVideoDefaultType(const virDomainDef *def)
static virDomainVideoAccelDefPtr
virDomainVideoAccelDefParseXML(xmlNodePtr node)
{
- xmlNodePtr cur;
g_autofree virDomainVideoAccelDefPtr def = NULL;
int val;
g_autofree char *accel2d = NULL;
g_autofree char *accel3d = NULL;
g_autofree char *rendernode = 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");
- }
- }
- cur = cur->next;
- }
+ accel3d = virXMLPropString(node, "accel3d");
+ accel2d = virXMLPropString(node, "accel2d");
+ rendernode = virXMLPropString(node, "rendernode");
if (!accel3d && !accel2d && !rendernode)
return NULL;
@@ -15312,22 +15302,12 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
static virDomainVideoResolutionDefPtr
virDomainVideoResolutionDefParseXML(xmlNodePtr node)
{
- xmlNodePtr cur;
g_autofree virDomainVideoResolutionDefPtr def = NULL;
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;
- }
+ x = virXMLPropString(node, "x");
+ y = virXMLPropString(node, "y");
if (!x || !y)
return NULL;
@@ -15415,6 +15395,7 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
if (cur->type == XML_ELEMENT_NODE) {
if (!type && !vram && !ram && !heads &&
virXMLNodeNameEqual(cur, "model")) {
+ xmlNodePtr child;
type = virXMLPropString(cur, "type");
ram = virXMLPropString(cur, "ram");
vram = virXMLPropString(cur, "vram");
@@ -15427,8 +15408,18 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
VIR_FREE(primary);
}
- def->accel = virDomainVideoAccelDefParseXML(cur);
- def->res = virDomainVideoResolutionDefParseXML(cur);
+ child = cur->children;
+ while (child != NULL) {
+ if (child->type == XML_ELEMENT_NODE) {
+ if (def->accel == NULL &&
+ virXMLNodeNameEqual(child, "acceleration"))
+ def->accel = virDomainVideoAccelDefParseXML(child);
+ if (def->res == NULL &&
+ virXMLNodeNameEqual(child, "resolution"))
+ def->res = virDomainVideoResolutionDefParseXML(child);
+ }
+ child = child->next;
+ }
}
if (virXMLNodeNameEqual(cur, "driver")) {
if (virDomainVirtioOptionsParseXML(cur, &def->virtio) < 0)
--
2.21.0
3:59 p.m.
New subject: [libvirt] [PATCH v3 2/4] conf: report errors when parsing video resolution
The current code doesn't properly handle errors when parsing a video
device's resolution. We were returning a NULL structure for the case
where 'x' or 'y' were missing. But for the other error cases, we were
logging an error (virReportError()), but still returning an
under-specified structure. That under-specified structure was used by
the calling function rather than properly reporting an error.
This patch changes the parse function to return NULL on any parsing
error and changes the calling function to report an error when NULL is
returned.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/conf/domain_conf.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5c86729258..b0599c7318 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15309,24 +15309,26 @@ virDomainVideoResolutionDefParseXML(xmlNodePtr node)
x = virXMLPropString(node, "x");
y = virXMLPropString(node, "y");
- if (!x || !y)
+ if (!x || !y) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("missing values for resolution"));
return NULL;
+ }
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 NULL;
}
if (virStrToLong_uip(y, NULL, 10, &def->y) < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("cannot parse video y-resolution '%s'"), y);
- goto cleanup;
+ return NULL;
}
- cleanup:
return g_steal_pointer(&def);
}
@@ -15415,8 +15417,10 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
virXMLNodeNameEqual(child, "acceleration"))
def->accel = virDomainVideoAccelDefParseXML(child);
if (def->res == NULL &&
- virXMLNodeNameEqual(child, "resolution"))
- def->res = virDomainVideoResolutionDefParseXML(child);
+ virXMLNodeNameEqual(child, "resolution")) {
+ if ((def->res =
virDomainVideoResolutionDefParseXML(child)) == NULL)
+ goto error;
+ }
}
child = child->next;
}
--
2.21.0
3:59 p.m.
New subject: [libvirt] [PATCH v3 3/4] conf: report errors when parsing video acceleration
Since this function is now only called when an 'acceleration' element is
present in the xml, any failure to parse the element will be considered
an error.
Previously, we detected some types of errors, but we would only log an
error (virReportError()), but still return a partially-specified accel
object to the caller. This patch returns NULL for all parsing errors and
reports that error back up to the caller.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/conf/domain_conf.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b0599c7318..c0f20c928f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15269,8 +15269,11 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
accel2d = virXMLPropString(node, "accel2d");
rendernode = virXMLPropString(node, "rendernode");
- if (!accel3d && !accel2d && !rendernode)
+ if (!accel3d && !accel2d && !rendernode) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("missing values for acceleration"));
return NULL;
+ }
def = g_new0(virDomainVideoAccelDef, 1);
@@ -15278,7 +15281,7 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
if ((val = virTristateBoolTypeFromString(accel3d)) <= 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unknown accel3d value '%s'"), accel3d);
- goto cleanup;
+ return NULL;
}
def->accel3d = val;
}
@@ -15287,7 +15290,7 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
if ((val = virTristateBoolTypeFromString(accel2d)) <= 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unknown accel2d value '%s'"), accel2d);
- goto cleanup;
+ return NULL;
}
def->accel2d = val;
}
@@ -15295,7 +15298,6 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
if (rendernode)
def->rendernode = virFileSanitizePath(rendernode);
- cleanup:
return g_steal_pointer(&def);
}
@@ -15414,8 +15416,10 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
while (child != NULL) {
if (child->type == XML_ELEMENT_NODE) {
if (def->accel == NULL &&
- virXMLNodeNameEqual(child, "acceleration"))
- def->accel = virDomainVideoAccelDefParseXML(child);
+ virXMLNodeNameEqual(child, "acceleration")) {
+ if ((def->accel = virDomainVideoAccelDefParseXML(child))
== NULL)
+ goto error;
+ }
if (def->res == NULL &&
virXMLNodeNameEqual(child, "resolution")) {
if ((def->res =
virDomainVideoResolutionDefParseXML(child)) == NULL)
--
2.21.0
3:59 p.m.
New subject: [libvirt] [PATCH v3 4/4] conf: validate video resolution
Ensure that both x and y are non-zero when resolution is specified for a
video device.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/conf/domain_conf.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c0f20c928f..54d6ae297e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6319,6 +6319,12 @@ virDomainVideoDefValidate(const virDomainVideoDef *video,
return -1;
}
+ if (video->res && (video->res->x == 0 || video->res->y == 0))
{
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("video resolution values must be greater than 0"));
+ return -1;
+ }
+
return 0;
}
--
2.21.0
12:54 p.m.
On 11/14/19 4:59 PM, Jonathon Jongsma wrote:
This is the third version of this patch series after a review from
Cole. The
main changes to this version of the patch series are:
- several patches have been pushed upstream (and therefore dropped from this
series
- The iteration over child nodes was moved up to the caller due to a
suggestion from Cole (much better, thanks)
- Because the parse functions are now only called when the appropriate XML
element is present, the parse functions no longer need to return
"successful" NULL for the case where the element is not specified in the
XML. Therefore we do not need to change the function signatures and we can
simply treat a NULL return value as an error.
- the resolution validation is moved to virDomainVideoDefValidate().
Jonathon Jongsma (4):
conf: iterate video model children in parent function
conf: report errors when parsing video resolution
conf: report errors when parsing video acceleration
conf: validate video resolution
src/conf/domain_conf.c | 75 ++++++++++++++++++++++--------------------
1 file changed, 40 insertions(+), 35 deletions(-)
Reviewed-by: Cole Robinson <crobinso(a)redhat.com>
And pushed
- Cole
1864
days inactive
1865
days old
5 comments
2 participants
participants (2)
-
Cole Robinson -
Jonathon Jongsma