[PATCH 1/5] conf: refactor virDomainBlkioDeviceParseXML to reduce calls to xmlNodeGetContent

[Laine Stump]

virDomainBlkioDeviceParseXML() calls xmlNodeGetContent() multiple
times in a loop, but can easily be refactored to call it once for all
element nodes, and then use the result of that one call in each of the
(mutually exclusive) blocks that previously each had their own call to
xmlNodeGetContent.

This is being done in order to reduce the number of changes needed in
an upcoming patch that will eliminate the lack of checking for NULL on
return from xmlNodeGetContent().

As part of the simplification, the while() loop has been changed into
a for() so that we can use "continue" without bypassing the "node =
node->next".

Signed-off-by: Laine Stump <laine at redhat.com>

Change from V1: turned into for() loop and log error rather than
ignoring NULL. Jano had suggested we might be able to set dev->path
directly instead of using a temporary var, but doing that would
require keeping the error: label and its cleanup of dev->path, rather
than just relying on g_autofree.

Signed-off-by: Laine Stump <laine at redhat.com>
---
 src/conf/domain_conf.c | 128 ++++++++++++++++++++++-------------------
 1 file changed, 70 insertions(+), 58 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7ecd2818b9..ade8c13914 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1635,73 +1635,85 @@ virDomainBlkioDeviceParseXML(xmlNodePtr root,
                              virBlkioDevicePtr dev)
 {
     xmlNodePtr node;
-    g_autofree char *c = NULL;
-
-    node = root->children;
-    while (node) {
-        if (node->type == XML_ELEMENT_NODE) {
-            if (virXMLNodeNameEqual(node, "path") && !dev->path) {
-                dev->path = (char *)xmlNodeGetContent(node);
-            } else if (virXMLNodeNameEqual(node, "weight")) {
-                c = (char *)xmlNodeGetContent(node);
-                if (virStrToLong_ui(c, NULL, 10, &dev->weight) < 0) {
-                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                                   _("could not parse weight %s"),
-                                   c);
-                        goto error;
-                }
-                VIR_FREE(c);
-            } else if (virXMLNodeNameEqual(node, "read_bytes_sec")) {
-                c = (char *)xmlNodeGetContent(node);
-                if (virStrToLong_ull(c, NULL, 10, &dev->rbps) < 0) {
-                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                                   _("could not parse read bytes sec %s"),
-                                   c);
-                    goto error;
-                }
-                VIR_FREE(c);
-            } else if (virXMLNodeNameEqual(node, "write_bytes_sec")) {
-                c = (char *)xmlNodeGetContent(node);
-                if (virStrToLong_ull(c, NULL, 10, &dev->wbps) < 0) {
-                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                                   _("could not parse write bytes sec %s"),
-                                   c);
-                    goto error;
-                }
-                VIR_FREE(c);
-            } else if (virXMLNodeNameEqual(node, "read_iops_sec")) {
-                c = (char *)xmlNodeGetContent(node);
-                if (virStrToLong_ui(c, NULL, 10, &dev->riops) < 0) {
-                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                                   _("could not parse read iops sec %s"),
-                                   c);
-                    goto error;
-                }
-                VIR_FREE(c);
-            } else if (virXMLNodeNameEqual(node, "write_iops_sec")) {
-                c = (char *)xmlNodeGetContent(node);
-                if (virStrToLong_ui(c, NULL, 10, &dev->wiops) < 0) {
-                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                                   _("could not parse write iops sec %s"),
-                                   c);
-                    goto error;
-                }
-                VIR_FREE(c);
+    g_autofree char *path = NULL;
+
+    for (node = root->children; node != NULL; node = node->next) {
+        g_autofree char *c = NULL;
+
+        if (node->type != XML_ELEMENT_NODE)
+            continue;
+
+        c = (char *)xmlNodeGetContent(node);
+
+        if (virXMLNodeNameEqual(node, "path")) {
+            /* To avoid the need for explicit cleanup on failure,
+             * don't set dev->path until we're assured of
+             * success. Until then, store it in an autofree pointer.
+             */
+            if (!path)
+                path = g_steal_pointer(&c);
+            continue;
+        }
+
+        if (virXMLNodeNameEqual(node, "weight")) {
+            if (virStrToLong_ui(c, NULL, 10, &dev->weight) < 0) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                               _("could not parse weight %s"),
+                               c);
+                return -1;
             }
+            continue;
+        }
+
+        if (virXMLNodeNameEqual(node, "read_bytes_sec")) {
+            if (virStrToLong_ull(c, NULL, 10, &dev->rbps) < 0) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                               _("could not parse read bytes sec %s"),
+                               c);
+                return -1;
+            }
+            continue;
+        }
+
+        if (virXMLNodeNameEqual(node, "write_bytes_sec")) {
+            if (virStrToLong_ull(c, NULL, 10, &dev->wbps) < 0) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                               _("could not parse write bytes sec %s"),
+                               c);
+                return -1;
+            }
+            continue;
+        }
+
+        if (virXMLNodeNameEqual(node, "read_iops_sec")) {
+            if (virStrToLong_ui(c, NULL, 10, &dev->riops) < 0) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                               _("could not parse read iops sec %s"),
+                               c);
+                return -1;
+            }
+            continue;
+        }
+
+        if (virXMLNodeNameEqual(node, "write_iops_sec")) {
+            if (virStrToLong_ui(c, NULL, 10, &dev->wiops) < 0) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                               _("could not parse write iops sec %s"),
+                               c);
+                return -1;
+            }
+            continue;
         }
-        node = node->next;
     }
-    if (!dev->path) {
+
+    if (!path) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                        _("missing per-device path"));
         return -1;
     }
 
+    dev->path = g_steal_pointer(&path);
     return 0;
-
- error:
-    VIR_FREE(dev->path);
-    return -1;
 }
 
 
-- 
2.25.4