conf: Increase virNetDevBandwidthParse intelligence

There's this function virNetDevBandwidthParse which parses the
bandwidth XML snippet. But it's not clever much. For the
following XML it allocates the virNetDevBandwidth structure even
though it's completely empty:

    <bandwidth>
    </bandwidth>

Later in the code there are some places where we check if
bandwidth was set or not. And since we obtained pointer from the
parsing function we think that it is when in fact it isn't.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b316b4bef980d8e81c3aa4365e68e535c60ae6ca..714ad56e669f472ca4261cd24e22b5822fad629d 100644 (file)
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7325,8 +7325,9 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
 
     bandwidth_node = virXPathNode("./bandwidth", ctxt);
     if (bandwidth_node &&
-        !(actual->bandwidth = virNetDevBandwidthParse(bandwidth_node,
-                                                      actual->type)))
+        virNetDevBandwidthParse(&actual->bandwidth,
+                                bandwidth_node,
+                                actual->type) < 0)
         goto error;
 
     vlanNode = virXPathNode("./vlan", ctxt);
@@ -7583,8 +7584,9 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
                     goto error;
                 }
             } else if (xmlStrEqual(cur->name, BAD_CAST "bandwidth")) {
-                if (!(def->bandwidth = virNetDevBandwidthParse(cur,
-                                                               def->type)))
+                if (virNetDevBandwidthParse(&def->bandwidth,
+                                            cur,
+                                            def->type) < 0)
                     goto error;
             } else if (xmlStrEqual(cur->name, BAD_CAST "vlan")) {
                 if (virNetDevVlanParse(cur, ctxt, &def->vlan) < 0)

diff --git a/src/conf/netdev_bandwidth_conf.c b/src/conf/netdev_bandwidth_conf.c
index 63a39e324c46d91ac6ebeb332fb988a8731d8683..c3e0f9f14af59a87197d4de2bf412d6bbdf4e715 100644 (file)
--- a/src/conf/netdev_bandwidth_conf.c
+++ b/src/conf/netdev_bandwidth_conf.c
@@ -102,6 +102,7 @@ virNetDevBandwidthParseRate(xmlNodePtr node, virNetDevBandwidthRatePtr rate)
 
 /**
  * virNetDevBandwidthParse:
+ * @bandwidth: parsed bandwidth
  * @node: XML node
  * @net_type: one of virDomainNetType
  *
@@ -111,21 +112,23 @@ virNetDevBandwidthParseRate(xmlNodePtr node, virNetDevBandwidthRatePtr rate)
  *
  * Returns !NULL on success, NULL on error.
  */
-virNetDevBandwidthPtr
-virNetDevBandwidthParse(xmlNodePtr node,
+int
+virNetDevBandwidthParse(virNetDevBandwidthPtr *bandwidth,
+                        xmlNodePtr node,
                         int net_type)
 {
+    int ret = -1;
     virNetDevBandwidthPtr def = NULL;
     xmlNodePtr cur;
     xmlNodePtr in = NULL, out = NULL;
 
     if (VIR_ALLOC(def) < 0)
-        return NULL;
+        return ret;
 
     if (!node || !xmlStrEqual(node->name, BAD_CAST "bandwidth")) {
         virReportError(VIR_ERR_INVALID_ARG, "%s",
                        _("invalid argument supplied"));
-        goto error;
+        goto cleanup;
     }
 
     cur = node->children;
@@ -137,7 +140,7 @@ virNetDevBandwidthParse(xmlNodePtr node,
                     virReportError(VIR_ERR_XML_DETAIL, "%s",
                                    _("Only one child <inbound> "
                                      "element allowed"));
-                    goto error;
+                    goto cleanup;
                 }
                 in = cur;
             } else if (xmlStrEqual(cur->name, BAD_CAST "outbound")) {
@@ -145,7 +148,7 @@ virNetDevBandwidthParse(xmlNodePtr node,
                     virReportError(VIR_ERR_XML_DETAIL, "%s",
                                    _("Only one child <outbound> "
                                      "element allowed"));
-                    goto error;
+                    goto cleanup;
                 }
                 out = cur;
             }
@@ -156,11 +159,11 @@ virNetDevBandwidthParse(xmlNodePtr node,
 
     if (in) {
         if (VIR_ALLOC(def->in) < 0)
-            goto error;
+            goto cleanup;
 
         if (virNetDevBandwidthParseRate(in, def->in) < 0) {
             /* helper reported error for us */
-            goto error;
+            goto cleanup;
         }
 
         if (def->in->floor && net_type != VIR_DOMAIN_NET_TYPE_NETWORK) {
@@ -174,32 +177,37 @@ virNetDevBandwidthParse(xmlNodePtr node,
                                _("floor attribute is supported only for "
                                  "interfaces of type network"));
             }
-            goto error;
+            goto cleanup;
         }
     }
 
     if (out) {
         if (VIR_ALLOC(def->out) < 0)
-            goto error;
+            goto cleanup;
 
         if (virNetDevBandwidthParseRate(out, def->out) < 0) {
             /* helper reported error for us */
-            goto error;
+            goto cleanup;
         }
 
         if (def->out->floor) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                            _("'floor' attribute allowed "
                              "only in <inbound> element"));
-            goto error;
+            goto cleanup;
         }
     }
 
-    return def;
+    if (!def->in && !def->out)
+        VIR_FREE(def);
+
+    *bandwidth = def;
+    def = NULL;
+    ret = 0;
 
- error:
+ cleanup:
     virNetDevBandwidthFree(def);
-    return NULL;
+    return ret;
 }
 
 static int

diff --git a/src/conf/netdev_bandwidth_conf.h b/src/conf/netdev_bandwidth_conf.h
index 60dacc658bfc55a0c925c4b3da6db5e29905b4b1..6cbf4aecb3df9f6970c6c76d86b6e35b8ddbb5bf 100644 (file)
--- a/src/conf/netdev_bandwidth_conf.h
+++ b/src/conf/netdev_bandwidth_conf.h
@@ -29,9 +29,10 @@
 # include "virxml.h"
 # include "domain_conf.h"
 
-virNetDevBandwidthPtr virNetDevBandwidthParse(xmlNodePtr node,
-                                              int net_type)
-    ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
+int virNetDevBandwidthParse(virNetDevBandwidthPtr *bandwidth,
+                            xmlNodePtr node,
+                            int net_type)
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
 int virNetDevBandwidthFormat(virNetDevBandwidthPtr def,
                              virBufferPtr buf)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index ddb5c077b50099a8928e09c18c3967dbe8f2aaba..26fe18deef175d494525dd5e1d5280c5d87cd0df 100644 (file)
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1649,9 +1649,8 @@ virNetworkPortGroupParseXML(virPortGroupDefPtr def,
 
     bandwidth_node = virXPathNode("./bandwidth", ctxt);
     if (bandwidth_node &&
-        !(def->bandwidth = virNetDevBandwidthParse(bandwidth_node, -1))) {
+        virNetDevBandwidthParse(&def->bandwidth, bandwidth_node, -1) < 0)
         goto cleanup;
-    }
 
     vlanNode = virXPathNode("./vlan", ctxt);
     if (vlanNode && virNetDevVlanParse(vlanNode, ctxt, &def->vlan) < 0)
@@ -2088,8 +2087,8 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
     /* Parse network domain information */
     def->domain = virXPathString("string(./domain[1]/@name)", ctxt);
 
-    if ((bandwidthNode = virXPathNode("./bandwidth", ctxt)) != NULL &&
-        (def->bandwidth = virNetDevBandwidthParse(bandwidthNode, -1)) == NULL)
+    if ((bandwidthNode = virXPathNode("./bandwidth", ctxt)) &&
+        virNetDevBandwidthParse(&def->bandwidth, bandwidthNode, -1) < 0)
         goto error;
 
     vlanNode = virXPathNode("./vlan", ctxt);

diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c
index cd24442ab2abe5a62b5549769d620f67c4885ee3..3b4645533fc9d62cbf0fd85566e45dbab141103e 100644 (file)
--- a/tests/virnetdevbandwidthtest.c
+++ b/tests/virnetdevbandwidthtest.c
@@ -43,6 +43,7 @@ struct testSetStruct {
 
 #define PARSE(xml, var)                                                 \
     do {                                                                \
+        int rc;                                                         \
         xmlDocPtr doc;                                                  \
         xmlXPathContextPtr ctxt = NULL;                                 \
                                                                         \
@@ -54,11 +55,12 @@ struct testSetStruct {
                                           &ctxt)))                      \
             goto cleanup;                                               \
                                                                         \
-        (var) = virNetDevBandwidthParse(ctxt->node,                     \
-                                        VIR_DOMAIN_NET_TYPE_NETWORK);   \
+        rc = virNetDevBandwidthParse(&(var),                            \
+                                     ctxt->node,                        \
+                                     VIR_DOMAIN_NET_TYPE_NETWORK);      \
         xmlFreeDoc(doc);                                                \
         xmlXPathFreeContext(ctxt);                                      \
-        if (!(var))                                                     \
+        if (rc < 0)                                                     \
             goto cleanup;                                               \
     } while (0)
 
@@ -127,9 +129,7 @@ mymain(void)
 
     DO_TEST_SET(NULL, NULL);
 
-    DO_TEST_SET(("<bandwidth/>"),
-                (TC " qdisc del dev eth0 root\n"
-                 TC " qdisc del dev eth0 ingress\n"));
+    DO_TEST_SET("<bandwidth/>", NULL);
 
     DO_TEST_SET(("<bandwidth>"
                  "  <inbound average='1024'/>"