From 5ba077dcd0775d5fc5b8d3691c0a68b46e44ff9e Mon Sep 17 00:00:00 2001
From: Jim Fehlig <jfehlig@suse.com>
Date: Thu, 21 Mar 2013 15:44:11 -0600
Subject: [PATCH] Fix parsing of bond interface XML

Noticed that parsing bond interface XML containing the miimon element
fails

  <interface type="bond" name="bond0">
    ...
    <bond mode="active-backup">
      <miimon freq="100" carrier="netif"/>
      ...
    </bond>
  </interface>

This configuration does not contain the optional updelay and downdelay
attributes, but parsing will fail due to returning the result of
virXPathULong (a -1 when the attribute doesn't exist) from
virInterfaceDefParseBond after examining the updelay attribute.

While fixing this bug, cleanup the function to use virXPathInt instead
of virXPathULong, and store the result directly instead of using a tmp
variable.  Using virXPathInt actually fixes a potential silent
truncation bug noted by Eric Blake.

Also, there is no cleanup in the error label.  Remove the label,
returning failure where failure occurs and success if the end of the
function is reached.
---
 src/conf/interface_conf.c | 63 +++++++++++++++++----------------------
 1 file changed, 27 insertions(+), 36 deletions(-)

diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c
index 9301ec0f61..7ca9c8618c 100644
--- a/src/conf/interface_conf.c
+++ b/src/conf/interface_conf.c
@@ -572,81 +572,72 @@ error:
 static int
 virInterfaceDefParseBond(virInterfaceDefPtr def,
                          xmlXPathContextPtr ctxt) {
-    int ret = -1;
-    unsigned long tmp;
+    int res;
 
     def->data.bond.mode = virInterfaceDefParseBondMode(ctxt);
     if (def->data.bond.mode < 0)
-        goto error;
+        return -1;
 
-    ret = virInterfaceDefParseBondItfs(def, ctxt);
-    if (ret != 0)
-       goto error;
+    if (virInterfaceDefParseBondItfs(def, ctxt) != 0)
+        return -1;
 
     if (virXPathNode("./miimon[1]", ctxt) != NULL) {
         def->data.bond.monit = VIR_INTERFACE_BOND_MONIT_MII;
 
-        ret = virXPathULong("string(./miimon/@freq)", ctxt, &tmp);
-        if ((ret == -2) || (ret == -1)) {
+        res = virXPathInt("string(./miimon/@freq)", ctxt,
+                          &def->data.bond.frequency);
+        if ((res == -2) || (res == -1)) {
             virReportError(VIR_ERR_XML_ERROR,
                            "%s", _("bond interface miimon freq missing or invalid"));
-            goto error;
+            return -1;
         }
-        def->data.bond.frequency = (int) tmp;
 
-        ret = virXPathULong("string(./miimon/@downdelay)", ctxt, &tmp);
-        if (ret == -2) {
+        res = virXPathInt("string(./miimon/@downdelay)", ctxt,
+                          &def->data.bond.downdelay);
+        if (res == -2) {
             virReportError(VIR_ERR_XML_ERROR,
                            "%s", _("bond interface miimon downdelay invalid"));
-            goto error;
-        } else if (ret == 0) {
-            def->data.bond.downdelay = (int) tmp;
+            return -1;
         }
 
-        ret = virXPathULong("string(./miimon/@updelay)", ctxt, &tmp);
-        if (ret == -2) {
+        res = virXPathInt("string(./miimon/@updelay)", ctxt,
+                          &def->data.bond.updelay);
+        if (res == -2) {
             virReportError(VIR_ERR_XML_ERROR,
                            "%s", _("bond interface miimon updelay invalid"));
-            goto error;
-        } else if (ret == 0) {
-            def->data.bond.updelay = (int) tmp;
+            return -1;
         }
 
         def->data.bond.carrier = virInterfaceDefParseBondMiiCarrier(ctxt);
-        if (def->data.bond.carrier < 0) {
-            ret = -1;
-            goto error;
-        }
+        if (def->data.bond.carrier < 0)
+            return -1;
 
     } else if (virXPathNode("./arpmon[1]", ctxt) != NULL) {
 
         def->data.bond.monit = VIR_INTERFACE_BOND_MONIT_ARP;
 
-        ret = virXPathULong("string(./arpmon/@interval)", ctxt, &tmp);
-        if ((ret == -2) || (ret == -1)) {
+        res = virXPathInt("string(./arpmon/@interval)", ctxt,
+                          &def->data.bond.interval);
+        if ((res == -2) || (res == -1)) {
             virReportError(VIR_ERR_XML_ERROR,
                            "%s", _("bond interface arpmon interval missing or invalid"));
-            goto error;
+            return -1;
         }
-        def->data.bond.interval = (int) tmp;
 
         def->data.bond.target =
             virXPathString("string(./arpmon/@target)", ctxt);
         if (def->data.bond.target == NULL) {
             virReportError(VIR_ERR_XML_ERROR,
                            "%s", _("bond interface arpmon target missing"));
-            ret = -1;
-            goto error;
+            return -1;
         }
 
         def->data.bond.validate = virInterfaceDefParseBondArpValid(ctxt);
-        if (def->data.bond.validate < 0) {
-            ret = -1;
-            goto error;
-        }
+        if (def->data.bond.validate < 0)
+            return -1;
     }
-error:
-    return ret;
+
+    return 0;
 }
 
 static int
-- 
GitLab