[collectd] [PATCH] libvirt plugin: Fix and cleanup refresh_lists()

[Ryota Ozaki]

refresh_lists function skips refreshing network interfaces
if refresing block devices fails. This behavior is not good
in some drivers, e.g., lxc, which does not have block device
entries in its XML.

This patch lets refresh_lists always refresh both elements
if either fails.

To do the fix, the patch separates refresh iterations on the
elements into individual functions to improve readability.
---
 src/libvirt.c |  169 +++++++++++++++++++++++++++++++--------------------------
 1 files changed, 92 insertions(+), 77 deletions(-)

diff --git a/src/libvirt.c b/src/libvirt.c
index 44f3832..c08eb91 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -399,6 +399,90 @@ lv_read (void)
     return 0;
 }
 
+static void
+refresh_block_devices(xmlXPathContextPtr xpath_ctx,
+                      virDomainPtr dom,
+                      const char *name)
+{
+    int i;
+    xmlXPathObjectPtr xpath_obj;
+
+    xpath_obj = xmlXPathEval
+        ((xmlChar *) "/domain/devices/disk/target[@dev]", xpath_ctx);
+    if (xpath_obj == NULL || xpath_obj->type != XPATH_NODESET ||
+        xpath_obj->nodesetval == NULL)
+        return;
+
+    for (i = 0; i < xpath_obj->nodesetval->nodeNr; ++i) {
+        xmlNodePtr node;
+        char *path = NULL;
+
+        node = xpath_obj->nodesetval->nodeTab[i];
+        if (!node) continue;
+        path = (char *) xmlGetProp (node, (xmlChar *) "dev");
+        if (!path) continue;
+
+        if (il_block_devices &&
+            ignore_device_match (il_block_devices, name, path) != 0)
+            goto cleanup;
+
+        add_block_device (dom, path);
+    cleanup:
+        if (path) xmlFree (path);
+    }
+    xmlXPathFreeObject (xpath_obj);
+}
+
+static void
+refresh_network_interfaces(xmlXPathContextPtr xpath_ctx,
+                           virDomainPtr dom,
+                           const char *name)
+{
+    int i;
+    xmlXPathObjectPtr xpath_obj;
+
+    xpath_obj = xmlXPathEval
+        ((xmlChar *) "/domain/devices/interface[target[@dev]]", xpath_ctx);
+    if (xpath_obj == NULL || xpath_obj->type != XPATH_NODESET ||
+        xpath_obj->nodesetval == NULL)
+        return;
+
+    xmlNodeSetPtr xml_interfaces = xpath_obj->nodesetval;
+
+    for (i = 0; i < xml_interfaces->nodeNr; ++i) {
+        char *path = NULL;
+        char *address = NULL;
+        xmlNodePtr xml_interface;
+
+        xml_interface = xml_interfaces->nodeTab[i];
+        if (!xml_interface) continue;
+        xmlNodePtr child = NULL;
+
+        for (child = xml_interface->children; child; child = child->next) {
+            if (child->type != XML_ELEMENT_NODE) continue;
+
+            if (xmlStrEqual(child->name, (const xmlChar *) "target")) {
+                path = (char *) xmlGetProp (child, (const xmlChar *) "dev");
+                if (!path) continue;
+            } else if (xmlStrEqual(child->name, (const xmlChar *) "mac")) {
+                address = (char *) xmlGetProp (child, (const xmlChar *) "address");
+                if (!address) continue;
+            }
+        }
+
+        if (il_interface_devices &&
+            (ignore_device_match (il_interface_devices, name, path) != 0 ||
+             ignore_device_match (il_interface_devices, name, address) != 0))
+            goto cleanup;
+
+        add_interface_device (dom, path, address);
+    cleanup:
+        if (path) xmlFree (path);
+        if (address) xmlFree (address);
+    }
+    xmlXPathFreeObject (xpath_obj);
+}
+
 static int
 refresh_lists (void)
 {
@@ -439,8 +523,6 @@ refresh_lists (void)
             char *xml = NULL;
             xmlDocPtr xml_doc = NULL;
             xmlXPathContextPtr xpath_ctx = NULL;
-            xmlXPathObjectPtr xpath_obj = NULL;
-            int j;
 
             dom = virDomainLookupByID (conn, domids[i]);
             if (dom == NULL) {
@@ -452,104 +534,37 @@ refresh_lists (void)
             name = virDomainGetName (dom);
             if (name == NULL) {
                 VIRT_ERROR (conn, "virDomainGetName");
-                goto cont;
+                goto cleanup;
             }
 
             if (il_domains && ignorelist_match (il_domains, name) != 0)
-                goto cont;
+                goto cleanup;
 
             if (add_domain (dom) < 0) {
                 ERROR ("libvirt plugin: malloc failed.");
-                goto cont;
+                goto cleanup;
             }
 
             /* Get a list of devices for this domain. */
             xml = virDomainGetXMLDesc (dom, 0);
             if (!xml) {
                 VIRT_ERROR (conn, "virDomainGetXMLDesc");
-                goto cont;
+                goto cleanup;
             }
 
             /* Yuck, XML.  Parse out the devices. */
             xml_doc = xmlReadDoc ((xmlChar *) xml, NULL, NULL, XML_PARSE_NONET);
             if (xml_doc == NULL) {
                 VIRT_ERROR (conn, "xmlReadDoc");
-                goto cont;
+                goto cleanup;
             }
 
             xpath_ctx = xmlXPathNewContext (xml_doc);
 
-            /* Block devices. */
-            xpath_obj = xmlXPathEval
-                ((xmlChar *) "/domain/devices/disk/target[@dev]",
-                 xpath_ctx);
-            if (xpath_obj == NULL || xpath_obj->type != XPATH_NODESET ||
-                xpath_obj->nodesetval == NULL)
-                goto cont;
-
-            for (j = 0; j < xpath_obj->nodesetval->nodeNr; ++j) {
-                xmlNodePtr node;
-                char *path = NULL;
-
-                node = xpath_obj->nodesetval->nodeTab[j];
-                if (!node) continue;
-                path = (char *) xmlGetProp (node, (xmlChar *) "dev");
-                if (!path) continue;
-
-                if (il_block_devices &&
-                    ignore_device_match (il_block_devices, name, path) != 0)
-                    goto cont2;
-
-                add_block_device (dom, path);
-            cont2:
-                if (path) xmlFree (path);
-            }
-            xmlXPathFreeObject (xpath_obj);
-
-            /* Network interfaces. */
-            xpath_obj = xmlXPathEval
-                ((xmlChar *) "/domain/devices/interface[target[@dev]]",
-                 xpath_ctx);
-            if (xpath_obj == NULL || xpath_obj->type != XPATH_NODESET ||
-                xpath_obj->nodesetval == NULL)
-                goto cont;
-
-            xmlNodeSetPtr xml_interfaces = xpath_obj->nodesetval;
-
-            for (j = 0; j < xml_interfaces->nodeNr; ++j) {
-                char *path = NULL;
-                char *address = NULL;
-                xmlNodePtr xml_interface;
-
-                xml_interface = xml_interfaces->nodeTab[j];
-                if (!xml_interface) continue;
-                xmlNodePtr child = NULL;
-
-                for (child = xml_interface->children; child; child = child->next) {
-                    if (child->type != XML_ELEMENT_NODE) continue;
-
-                    if (xmlStrEqual(child->name, (const xmlChar *) "target")) {
-                        path = (char *) xmlGetProp (child, (const xmlChar *) "dev");
-                        if (!path) continue;
-                    } else if (xmlStrEqual(child->name, (const xmlChar *) "mac")) {
-                        address = (char *) xmlGetProp (child, (const xmlChar *) "address");
-                        if (!address) continue;
-                    }
-                }
-
-                if (il_interface_devices &&
-                    (ignore_device_match (il_interface_devices, name, path) != 0 ||
-                     ignore_device_match (il_interface_devices, name, address) != 0))
-                    goto cont3;
-
-                add_interface_device (dom, path, address);
-                cont3:
-                    if (path) xmlFree (path);
-                    if (address) xmlFree (address);
-            }
+            refresh_block_devices (xpath_ctx, dom, name);
+            refresh_network_interfaces (xpath_ctx, dom, name);
 
-        cont:
-            if (xpath_obj) xmlXPathFreeObject (xpath_obj);
+        cleanup:
             if (xpath_ctx) xmlXPathFreeContext (xpath_ctx);
             if (xml_doc) xmlFreeDoc (xml_doc);
             sfree (xml);
-- 
1.6.5.2