[libvirt] [PATCH v3 0/4] Fix up some issues from x and y resolution patches
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
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@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
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@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
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@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
Ensure that both x and y are non-zero when resolution is specified for a video device. Signed-off-by: Jonathon Jongsma <jjongsma@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
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@redhat.com> And pushed - Cole
participants (2)
-
Cole Robinson -
Jonathon Jongsma