On 08/03/2015 02:39 AM, Michal Privoznik wrote:
So, if a domain vNIC's bandwidth has been successfully set, it's possible that because @floor is set on network's bridge, this part may need updating too. And that's exactly what this function does. While the previous commit introduced a function to check if @floor can be satisfied, this does all the hard work. In general, there may be three, well four possibilities:
1) No change in @floor value (either it remain unset, or its value hasn't changed)
2) The @floor value has changed from a non-zero to a non-zero value
3) New @floor is to be set
4) Old @floor must be cleared out
The difference between 2), 3) and 4) is, that while in 2) the QoS tree on the network's bridge already has a special class for the vNIC, in 3) the class must be created from scratch. In 4) it must be removed. Fortunately, we have helpers for all three interesting cases.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 160 ++++++++++++++++++++++++++++++++++++-------- src/network/bridge_driver.h | 10 +++ 2 files changed, 142 insertions(+), 28 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index fa60ba4..5fad015 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4792,38 +4792,17 @@ networkNextClassID(virNetworkObjPtr net) return ret; }
+ static int -networkPlugBandwidth(virNetworkObjPtr net, - virDomainNetDefPtr iface) +networkPlugBandwidthImpl(virNetworkObjPtr net, + virDomainNetDefPtr iface, + virNetDevBandwidthPtr ifaceBand, + unsigned long long new_rate) { virNetworkDriverStatePtr driver = networkGetDriver(); - int ret = -1; - int plug_ret; - unsigned long long new_rate = 0; ssize_t class_id = 0; - char ifmac[VIR_MAC_STRING_BUFLEN]; - virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface); - - if ((plug_ret = networkCheckBandwidth(net, ifaceBand, NULL, - iface->mac, &new_rate)) < 0) { - /* helper reported error */ - goto cleanup; - } - - if (plug_ret > 0) { - /* no QoS needs to be set; claim success */ - ret = 0; - goto cleanup; - } - - virMacAddrFormat(&iface->mac, ifmac); - if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK || - !iface->data.network.actual) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot set bandwidth on interface '%s' of type %d"), - ifmac, iface->type); - goto cleanup; - } + int plug_ret; + int ret = -1;
/* generate new class_id */ if ((class_id = networkNextClassID(net)) < 0) { @@ -4859,6 +4838,46 @@ networkPlugBandwidth(virNetworkObjPtr net, net->def->bridge);
ret = 0; + cleanup: + return ret; +} + + +static int +networkPlugBandwidth(virNetworkObjPtr net, + virDomainNetDefPtr iface) +{ + int ret = -1; + int plug_ret; + unsigned long long new_rate = 0; + char ifmac[VIR_MAC_STRING_BUFLEN]; + virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface); + + if ((plug_ret = networkCheckBandwidth(net, ifaceBand, NULL, + iface->mac, &new_rate)) < 0) { + /* helper reported error */ + goto cleanup; + } + + if (plug_ret > 0) { + /* no QoS needs to be set; claim success */ + ret = 0; + goto cleanup; + } + + virMacAddrFormat(&iface->mac, ifmac); + if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK || + !iface->data.network.actual) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot set bandwidth on interface '%s' of type %d"), + ifmac, iface->type); + goto cleanup; + } + + if (networkPlugBandwidthImpl(net, iface, ifaceBand, new_rate) < 0) + goto cleanup; + + ret = 0;
cleanup: return ret; @@ -4939,6 +4958,9 @@ networkBandwidthGenericChecks(virDomainNetDefPtr iface, virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface);
^^ 'iface' is used here... but !iface checked below...
unsigned long long old_floor, new_floor;
+ if (!iface && !newBandwidth) + return false; +
Adding this !iface check causes Coverity issues for the following usage of 'iface' (especially if 'newBandwidth' is non NULL)... Checking the two (eventual) callers - networkBandwidthChangeAllowed already has ATTRIBUTE_NONNULL(1) (2) checks and it seems logically at least that networkBandwidthUpdate should have it. Thus I don't think it's necessary (although I may have read things wrong too ;-))
if (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_NETWORK) { /* This is not an interface that's plugged into a network. * We don't care. Thus from our POV bandwidth change is allowed. */ @@ -4985,3 +5007,85 @@ networkBandwidthChangeAllowed(virDomainNetDefPtr iface, virNetworkObjEndAPI(&network); return ret; } + + +int +networkBandwidthUpdate(virDomainNetDefPtr iface, + virNetDevBandwidthPtr newBandwidth) +{ + virNetworkDriverStatePtr driver = networkGetDriver(); + virNetworkObjPtr network = NULL; + virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface);
^^ 'iface' is used unconditionally here So arg1 can have the ATTRIBUTE_NONNULL 'newBandwidth' is used below, so it too can have ATTRIBUTE_NONNULL Which means check in GenericChecks is unnecessary - although to be safe we could add the ATTRIBUTE_NONNULL to the static decl too.
+ unsigned long long new_rate = 0; + int plug_ret; + int ret = -1; + + if (!networkBandwidthGenericChecks(iface, newBandwidth)) + return 0; + + network = virNetworkObjFindByName(driver->networks, iface->data.network.name); + if (!network) { + virReportError(VIR_ERR_NO_NETWORK, + _("no network with matching name '%s'"), + iface->data.network.name); + return ret; + } + + if ((plug_ret = networkCheckBandwidth(network, ifaceBand, NULL, + iface->mac, &new_rate)) < 0) { + /* helper reported error */ + goto cleanup; + } + + if (plug_ret > 0) { + /* no QoS needs to be set; claim success */ + ret = 0; + goto cleanup; + } + + /* Okay, there are three possible scenarios: */ + + if (ifaceBand->in && ifaceBand->in->floor && + newBandwidth->in && newBandwidth->in->floor) { + /* Either we just need to update @floor .. */ + + if (virNetDevBandwidthUpdateRate(network->def->bridge, + iface->data.network.actual->class_id, + network->def->bandwidth, + newBandwidth->in->floor) < 0) + goto cleanup; + + network->floor_sum -= ifaceBand->in->floor; + network->floor_sum += newBandwidth->in->floor; + new_rate -= network->floor_sum; + + if (virNetDevBandwidthUpdateRate(network->def->bridge, 2, + network->def->bandwidth, new_rate) < 0 || + virNetworkSaveStatus(driver->stateDir, network) < 0) { + /* Ouch, rollback */ + network->floor_sum -= newBandwidth->in->floor; + network->floor_sum += ifaceBand->in->floor; + + ignore_value(virNetDevBandwidthUpdateRate(network->def->bridge, + iface->data.network.actual->class_id, + network->def->bandwidth, + ifaceBand->in->floor)); + goto cleanup; + } + } else if (newBandwidth->in && newBandwidth->in->floor) { + /* .. or we need to plug in new .. */ + + if (networkPlugBandwidthImpl(network, iface, newBandwidth, new_rate) < 0) + goto cleanup; + } else { + /* .. or unplug old. */ + + if (networkUnplugBandwidth(network, iface) < 0) + goto cleanup; + } + + ret = 0; + cleanup: + virNetworkObjEndAPI(&network); + return ret; +} diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h index cce9237..3a9f22d 100644 --- a/src/network/bridge_driver.h +++ b/src/network/bridge_driver.h @@ -57,6 +57,9 @@ bool networkBandwidthChangeAllowed(virDomainNetDefPtr iface, virNetDevBandwidthPtr newBandwidth) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+int networkBandwidthUpdate(virDomainNetDefPtr iface, + virNetDevBandwidthPtr newBandwidth); +
I would think based on code ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); would be need to be added. Additionally, networkBandwidthChangeAllowed has them and this is called after that... Beyond that I think things look OK ACK with some adjustments John
# else /* Define no-op replacements that don't drag in any link dependencies. */ # define networkAllocateActualDevice(dom, iface) 0 @@ -85,6 +88,13 @@ networkBandwidthChangeAllowed(virDomainNetDefPtr iface ATTRIBUTE_UNUSED, return true; }
+static inline int +networkBandwidthUpdate(virDomainNetDefPtr iface ATTRIBUTE_UNUSED, + virNetDevBandwidthPtr newBandwidth ATTRIBUTE_UNUSED) +{ + return 0; +} + # endif
typedef char *(*networkDnsmasqLeaseFileNameFunc)(const char *netname);