On a Wednesday in 2020, Laine Stump wrote:
virDomainBlkioDeviceParseXML() has multiple cases of sending the
return from xmlNodeGetContent() directly to virStrToLong_xx() without
checking for NULL. Although it is *very* rare for xmlNodeGetContent()
to return NULL (possibly it only happens in an OOM condition? The
documentation is unclear), it could happen, and the refactor in this
patch manages to eliminate several lines of repeated code while adding
in a (single) check for NULL.
Signed-off-by: Laine Stump <laine@xxxxxxxxxx>
---
src/conf/domain_conf.c | 39 +++++++++++++++------------------------
1 file changed, 15 insertions(+), 24 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1916b51d38..8cde1cd0e8 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1628,73 +1628,64 @@ virDomainBlkioDeviceParseXML(xmlNodePtr root,
virBlkioDevicePtr dev)
{
xmlNodePtr node;
- g_autofree char *c = NULL;
+ g_autofree char *path = NULL;
node = root->children;
while (node) {
- if (node->type == XML_ELEMENT_NODE) {
- if (virXMLNodeNameEqual(node, "path") && !dev->path) {
- dev->path = (char *)xmlNodeGetContent(node);
+ g_autofree char *c = NULL;
+
+ if (node->type == XML_ELEMENT_NODE &&
+ (c = (char *)xmlNodeGetContent(node))) {
Missing ErrorReport if xmlNodeGetContent fails.
Converting this open-coded for loop to an actual for loop would
grant us 'continue' privileges, which would make the checks
nicer and give a possibility of assigning the path directly
to 'path', without the extra steal_pointer.
Alternatively, the minimum diff where I'd consider this patch to be
a strict improvement is:
} else if (node->type == XML_ELEMENT_NODE && !c) {
virReportOOMError();
return -1;
}
With that: Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx>
Jano
Attachment:
signature.asc
Description: PGP signature
