network: remove the virDomainNetBandwidthChangeAllowed callback

The current qemu driver code for changing bandwidth on a NIC first asks
the network driver if the change is supported, then changes the
bandwidth on the VIF, and then tells the network driver to update the
bandwidth on the bridge.

This is potentially racing if a parallel API call causes the network
driver to allocate bandwidth on the bridge between the check and the
update phases.

Change the code to just try to apply the network bridge update
immediately and rollback at the end if something failed.

Reviewed-by: Laine Stump <laine@laine.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6f704f4e5dcb46d5949f3b19926d5287d9e6b1f0..85b374572c1b8a60875835c5f99930b34feb5ce2 100644 (file)
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -30670,7 +30670,6 @@ virDomainNetDefActualToNetworkPort(virDomainDefPtr dom,
 static virDomainNetAllocateActualDeviceImpl netAllocate;
 static virDomainNetNotifyActualDeviceImpl netNotify;
 static virDomainNetReleaseActualDeviceImpl netRelease;
-static virDomainNetBandwidthChangeAllowedImpl netBandwidthChangeAllowed;
 static virDomainNetBandwidthUpdateImpl netBandwidthUpdate;
 
 
@@ -30678,13 +30677,11 @@ void
 virDomainNetSetDeviceImpl(virDomainNetAllocateActualDeviceImpl allocate,
                           virDomainNetNotifyActualDeviceImpl notify,
                           virDomainNetReleaseActualDeviceImpl release,
-                          virDomainNetBandwidthChangeAllowedImpl bandwidthChangeAllowed,
                           virDomainNetBandwidthUpdateImpl bandwidthUpdate)
 {
     netAllocate = allocate;
     netNotify = notify;
     netRelease = release;
-    netBandwidthChangeAllowed = bandwidthChangeAllowed;
     netBandwidthUpdate = bandwidthUpdate;
 }
 
@@ -30767,18 +30764,6 @@ virDomainNetReleaseActualDevice(virConnectPtr conn,
     return ret;
 }
 
-bool
-virDomainNetBandwidthChangeAllowed(virDomainNetDefPtr iface,
-                                   virNetDevBandwidthPtr newBandwidth)
-{
-    if (!netBandwidthChangeAllowed) {
-        virReportError(VIR_ERR_NO_SUPPORT, "%s",
-                       _("Virtual networking driver is not available"));
-        return -1;
-    }
-
-    return netBandwidthChangeAllowed(iface, newBandwidth);
-}
 
 int
 virDomainNetBandwidthUpdate(virDomainNetDefPtr iface,

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index d88c29fad54cdd122eb6561007f9e3695549a80f..6ba0643cd15d3b317c027aa871bca7fe5579332c 100644 (file)
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3584,10 +3584,6 @@ typedef int
                                        virDomainDefPtr dom,
                                        virDomainNetDefPtr iface);
 
-typedef bool
-(*virDomainNetBandwidthChangeAllowedImpl)(virDomainNetDefPtr iface,
-                                          virNetDevBandwidthPtr newBandwidth);
-
 typedef int
 (*virDomainNetBandwidthUpdateImpl)(virDomainNetDefPtr iface,
                                    virNetDevBandwidthPtr newBandwidth);
@@ -3597,7 +3593,6 @@ void
 virDomainNetSetDeviceImpl(virDomainNetAllocateActualDeviceImpl allocate,
                           virDomainNetNotifyActualDeviceImpl notify,
                           virDomainNetReleaseActualDeviceImpl release,
-                          virDomainNetBandwidthChangeAllowedImpl bandwidthChangeAllowed,
                           virDomainNetBandwidthUpdateImpl bandwidthUpdate);
 
 int
@@ -3618,11 +3613,6 @@ virDomainNetReleaseActualDevice(virConnectPtr conn,
                                 virDomainNetDefPtr iface)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 
-bool
-virDomainNetBandwidthChangeAllowed(virDomainNetDefPtr iface,
-                              virNetDevBandwidthPtr newBandwidth)
-    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-
 int
 virDomainNetBandwidthUpdate(virDomainNetDefPtr iface,
                             virNetDevBandwidthPtr newBandwidth)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 6b030e3de32e4d19e61211a5a49f45fc8f617b20..c4bc60fb24610345b6ff3e7f7e84c2c86b28f1a5 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -456,7 +456,6 @@ virDomainMemorySourceTypeFromString;
 virDomainMemorySourceTypeToString;
 virDomainNetAllocateActualDevice;
 virDomainNetAppendIPAddress;
-virDomainNetBandwidthChangeAllowed;
 virDomainNetBandwidthUpdate;
 virDomainNetDefActualFromNetworkPort;
 virDomainNetDefActualToNetworkPort;

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 95b7a7cce96eabc31a73a6a88ee7c7290690c7ec..fe00a4fd4194439c04f712f50f5d9ca65e7e1303 100644 (file)
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -5349,64 +5349,6 @@ networkNetworkObjTaint(virNetworkObjPtr obj,
 }
 
 
-static bool
-networkBandwidthGenericChecks(virDomainNetDefPtr iface,
-                              virNetDevBandwidthPtr newBandwidth)
-{
-    virNetDevBandwidthPtr ifaceBand;
-    unsigned long long old_floor, new_floor;
-
-    if (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_NETWORK &&
-        (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_BRIDGE ||
-         iface->data.network.actual->data.bridge.brname == NULL)) {
-        /* This is not an interface that's plugged into a network.
-         * We don't care. Thus from our POV bandwidth change is allowed. */
-        return false;
-    }
-
-    ifaceBand = virDomainNetGetActualBandwidth(iface);
-    old_floor = new_floor = 0;
-
-    if (ifaceBand && ifaceBand->in)
-        old_floor = ifaceBand->in->floor;
-    if (newBandwidth && newBandwidth->in)
-        new_floor = newBandwidth->in->floor;
-
-    return new_floor != old_floor;
-}
-
-
-static bool
-networkBandwidthChangeAllowed(virDomainNetDefPtr iface,
-                              virNetDevBandwidthPtr newBandwidth)
-{
-    virNetworkDriverStatePtr driver = networkGetDriver();
-    virNetworkObjPtr obj = NULL;
-    virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface);
-    bool ret = false;
-
-    if (!networkBandwidthGenericChecks(iface, newBandwidth))
-        return true;
-
-    obj = virNetworkObjFindByName(driver->networks, iface->data.network.name);
-    if (!obj) {
-        virReportError(VIR_ERR_NO_NETWORK,
-                       _("no network with matching name '%s'"),
-                       iface->data.network.name);
-        return false;
-    }
-
-    if (networkCheckBandwidth(obj, newBandwidth, ifaceBand, &iface->mac, NULL) < 0)
-        goto cleanup;
-
-    ret = true;
-
- cleanup:
-    virNetworkObjEndAPI(&obj);
-    return ret;
-}
-
-
 static int
 networkBandwidthUpdate(virDomainNetDefPtr iface,
                        virNetDevBandwidthPtr newBandwidth)
@@ -5417,6 +5359,7 @@ networkBandwidthUpdate(virDomainNetDefPtr iface,
     unsigned long long tmp_floor_sum;
     virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface);
     unsigned long long new_rate = 0;
+    unsigned long long old_floor, new_floor;
     int plug_ret;
     int ret = -1;
 
@@ -5426,7 +5369,22 @@ networkBandwidthUpdate(virDomainNetDefPtr iface,
         return -1;
     }
 
-    if (!networkBandwidthGenericChecks(iface, newBandwidth))
+    if (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_NETWORK &&
+        (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_BRIDGE ||
+         iface->data.network.actual->data.bridge.brname != NULL)) {
+        /* This is not an interface that's plugged into a bridge.
+         * We don't care. Thus from our POV bandwidth change is allowed. */
+        return 0;
+    }
+
+    old_floor = new_floor = 0;
+
+    if (ifaceBand && ifaceBand->in)
+        old_floor = ifaceBand->in->floor;
+    if (newBandwidth && newBandwidth->in)
+        new_floor = newBandwidth->in->floor;
+
+    if (new_floor == old_floor)
         return 0;
 
     obj = virNetworkObjFindByName(driver->networks, iface->data.network.name);
@@ -5572,7 +5530,6 @@ networkRegister(void)
         networkAllocateActualDevice,
         networkNotifyActualDevice,
         networkReleaseActualDevice,
-        networkBandwidthChangeAllowed,
         networkBandwidthUpdate);
 
     return 0;

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f623eaa4220c1eca2114138d4d8768d5d2cf5e59..b6ff0386351197c3170b3c3939a4fc8aa11f85f0 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11625,17 +11625,17 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
         }
 
         if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
-            !virDomainNetBandwidthChangeAllowed(net, newBandwidth))
+            virDomainNetBandwidthUpdate(net, newBandwidth) < 0)
             goto endjob;
 
         if (virNetDevBandwidthSet(net->ifname, newBandwidth, false,
-                                  !virDomainNetTypeSharesHostView(net)) < 0 ||
-            (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
-             virDomainNetBandwidthUpdate(net, newBandwidth) < 0)) {
+                                  !virDomainNetTypeSharesHostView(net)) < 0) {
             ignore_value(virNetDevBandwidthSet(net->ifname,
                                                net->bandwidth,
                                                false,
                                                !virDomainNetTypeSharesHostView(net)));
+            ignore_value(virDomainNetBandwidthUpdate(net,
+                                                     net->bandwidth));
             goto endjob;
         }