From e3be969238c715142642a1429630f2f0bcb5b293 Mon Sep 17 00:00:00 2001
Message-Id: <e3be969238c715142642a1429630f2f0bcb5b293.1383922565.git.jdenemar@redhat.com>
From: Laine Stump <laine@laine.org>
Date: Wed, 6 Nov 2013 04:47:59 -0700
Subject: [PATCH] network: fix connections count in case of allocate failure

This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1020135

If networkAllocateActualDevice() had failed due to a pool of hostdev
or direct devices being depleted, the calling function could still
call networkReleaseActualDevice() as part of its cleanup, and that
function would then unconditionally decrement the connections count
for the network, even though it hadn't been incremented (due to
failure of allocate). This *was* necessary because the .actual member
of the netdef was allocated with a "lazy" algorithm, only being
created if there was a need to store data there (e.g. if a device was
allocated from a pool, or bandwidth was allocated for the device), so
there was no simple way for networkReleaseActualDevice() to tell if
something really had been allocated (i.e. if "connections++" had been
executed).

This patch changes networkAllocateDevice() to *always* allocate an
actual device for any netdef of type='network', even if it isn't
needed for any other reason. This has no ill effects anywhere else in
the code (except for using a small amount of memory), and
networkReleaseActualDevice() can then determine if there was a
previous successful allocate by checking for .actual != NULL (if not,
it skips the "connections--").

(cherry picked from commit b4e0299d4ff059c8707a760b7ec2063ccd57cc21)

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---
 src/network/bridge_driver.c | 41 +++++++++++------------------------------
 1 file changed, 11 insertions(+), 30 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index ea09663..c900478 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -3841,6 +3841,9 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
     }
     netdef = network->def;
 
+    if (VIR_ALLOC(iface->data.network.actual) < 0)
+        goto error;
+
     /* portgroup can be present for any type of network, in particular
      * for bandwidth information, so we need to check for that and
      * fill it in appropriately for all forward types.
@@ -3857,14 +3860,9 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
     else if (portgroup && portgroup->bandwidth)
         bandwidth = portgroup->bandwidth;
 
-    if (bandwidth) {
-        if (!iface->data.network.actual &&
-            VIR_ALLOC(iface->data.network.actual) < 0)
-            goto error;
-        if (virNetDevBandwidthCopy(&iface->data.network.actual->bandwidth,
-                                   bandwidth) < 0)
-            goto error;
-    }
+    if (bandwidth && virNetDevBandwidthCopy(&iface->data.network.actual->bandwidth,
+                                            bandwidth) < 0)
+        goto error;
 
     /* copy appropriate vlan info to actualNet */
     if (iface->vlan.nTags > 0)
@@ -3874,13 +3872,8 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
     else if (netdef->vlan.nTags > 0)
         vlan = &netdef->vlan;
 
-    if (vlan) {
-        if (!iface->data.network.actual &&
-            VIR_ALLOC(iface->data.network.actual) < 0)
-            goto error;
-        if (virNetDevVlanCopy(&iface->data.network.actual->vlan, vlan) < 0)
-           goto error;
-    }
+    if (vlan && virNetDevVlanCopy(&iface->data.network.actual->vlan, vlan) < 0)
+        goto error;
 
     if ((netdef->forward.type == VIR_NETWORK_FORWARD_NONE) ||
         (netdef->forward.type == VIR_NETWORK_FORWARD_NAT) ||
@@ -3889,8 +3882,7 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
          *NETWORK; we just keep the info from the portgroup in
          * iface->data.network.actual
         */
-        if (iface->data.network.actual)
-            iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_NETWORK;
+        iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_NETWORK;
 
         if (networkPlugBandwidth(network, iface) < 0)
             goto error;
@@ -3902,10 +3894,6 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
          * is VIR_DOMAIN_NET_TYPE_BRIDGE
          */
 
-        if (!iface->data.network.actual &&
-            VIR_ALLOC(iface->data.network.actual) < 0)
-            goto error;
-
         iface->data.network.actual->type = actualType = VIR_DOMAIN_NET_TYPE_BRIDGE;
         if (VIR_STRDUP(iface->data.network.actual->data.bridge.brname,
                        netdef->bridge) < 0)
@@ -3938,10 +3926,6 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
 
         virDomainHostdevSubsysPciBackendType backend;
 
-        if (!iface->data.network.actual &&
-            VIR_ALLOC(iface->data.network.actual) < 0)
-            goto error;
-
         iface->data.network.actual->type = actualType = VIR_DOMAIN_NET_TYPE_HOSTDEV;
         if (netdef->forward.npfs > 0 && netdef->forward.nifs <= 0 &&
             networkCreateInterfacePool(netdef) < 0) {
@@ -4025,10 +4009,6 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
          * VIR_DOMAIN_NET_TYPE_DIRECT.
          */
 
-        if (!iface->data.network.actual &&
-            VIR_ALLOC(iface->data.network.actual) < 0)
-            goto error;
-
         /* Set type=direct and appropriate <source mode='xxx'/> */
         iface->data.network.actual->type = actualType = VIR_DOMAIN_NET_TYPE_DIRECT;
         switch (netdef->forward.type) {
@@ -4503,7 +4483,8 @@ networkReleaseActualDevice(virDomainNetDefPtr iface)
    }
 
 success:
-    netdef->connections--;
+    if (iface->data.network.actual)
+        netdef->connections--;
     VIR_DEBUG("Releasing network %s, %d connections",
               netdef->name, netdef->connections);
     ret = 0;
-- 
1.8.4.2