[libvirt] [PATCH v3 26/36] network: introduce networkUpdatePortBandwidth
Laine Stump
laine at laine.org
Wed Apr 3 02:06:15 UTC 2019
On 3/19/19 8:46 AM, Daniel P. Berrangé wrote:
> Separate network port bandwidth update code from the domain driver
> network callback implementation.
>
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
> src/network/bridge_driver.c | 115 ++++++++++++++++++++----------------
> 1 file changed, 65 insertions(+), 50 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 3c2fcd16e5..74dcd0c44a 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -5383,77 +5383,56 @@ networkNetworkObjTaint(virNetworkObjPtr obj,
>
>
> static int
> -networkBandwidthUpdate(virDomainNetDefPtr iface,
> - virNetDevBandwidthPtr newBandwidth)
> +networkUpdatePortBandwidth(virNetworkObjPtr obj,
> + virMacAddrPtr mac,
> + unsigned int *class_id,
> + virNetDevBandwidthPtr oldBandwidth,
> + virNetDevBandwidthPtr newBandwidth)
The fact that the virNetworkPortDefPtr isn't in the args made me realize
that this function is changing the bandwidth on the device, but the
bandwidth in the virNetworkPortDefPtr is changed elsewhere. This seems a
bit odd. But again, it is existing behavior, and you don't want to
change everything at once. (seems like that's the way it should work
once there is a public API though - the device and the virNetworkPortDef
should both be updated by the same function).
> {
> virNetworkDriverStatePtr driver = networkGetDriver();
> - virNetworkObjPtr obj = NULL;
> virNetworkDefPtr def;
> 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;
> -
> - if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Expected a interface for a virtual network"));
> - return -1;
> - }
> -
> - if (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 0;
> - }
>
> old_floor = new_floor = 0;
>
> - if (ifaceBand && ifaceBand->in)
> - old_floor = ifaceBand->in->floor;
> + if (oldBandwidth && oldBandwidth->in)
> + old_floor = oldBandwidth->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);
> - if (!obj) {
> - virReportError(VIR_ERR_NO_NETWORK,
> - _("no network with matching name '%s'"),
> - iface->data.network.name);
> - return ret;
> - }
> def = virNetworkObjGetDef(obj);
>
> - if ((plug_ret = networkCheckBandwidth(obj, newBandwidth, ifaceBand,
> - &iface->mac, &new_rate)) < 0) {
> + if ((plug_ret = networkCheckBandwidth(obj, newBandwidth, oldBandwidth,
> + mac, &new_rate)) < 0) {
> /* helper reported error */
> - goto cleanup;
> + return -1;
> }
>
> if (plug_ret > 0) {
> /* no QoS needs to be set; claim success */
> - ret = 0;
> - goto cleanup;
> + return 0;
> }
>
> /* Okay, there are three possible scenarios: */
>
> - if (ifaceBand && ifaceBand->in && ifaceBand->in->floor &&
> + if (oldBandwidth && oldBandwidth->in && oldBandwidth->in->floor &&
> newBandwidth->in && newBandwidth->in->floor) {
> /* Either we just need to update @floor .. */
>
> if (virNetDevBandwidthUpdateRate(def->bridge,
> - iface->data.network.actual->class_id,
> + *class_id,
> def->bandwidth,
> newBandwidth->in->floor) < 0)
> - goto cleanup;
> + return -1;
>
> tmp_floor_sum = virNetworkObjGetFloorSum(obj);
> - tmp_floor_sum -= ifaceBand->in->floor;
> + tmp_floor_sum -= oldBandwidth->in->floor;
> tmp_floor_sum += newBandwidth->in->floor;
> virNetworkObjSetFloorSum(obj, tmp_floor_sum);
> new_rate -= tmp_floor_sum;
> @@ -5463,34 +5442,70 @@ networkBandwidthUpdate(virDomainNetDefPtr iface,
> virNetworkObjSaveStatus(driver->stateDir, obj) < 0) {
> /* Ouch, rollback */
> tmp_floor_sum -= newBandwidth->in->floor;
> - tmp_floor_sum += ifaceBand->in->floor;
> + tmp_floor_sum += oldBandwidth->in->floor;
> virNetworkObjSetFloorSum(obj, tmp_floor_sum);
>
> ignore_value(virNetDevBandwidthUpdateRate(def->bridge,
> - iface->data.network.actual->class_id,
> + *class_id,
> def->bandwidth,
> - ifaceBand->in->floor));
> - goto cleanup;
> + oldBandwidth->in->floor));
> + return -1;
> }
> } else if (newBandwidth->in && newBandwidth->in->floor) {
> /* .. or we need to plug in new .. */
>
> - if (networkPlugBandwidthImpl(obj, &iface->mac, newBandwidth,
> - iface->data.network.actual ?
> - &iface->data.network.actual->class_id : NULL,
> + if (networkPlugBandwidthImpl(obj, mac, newBandwidth,
> + class_id,
> new_rate) < 0)
> - goto cleanup;
> + return -1;
> } else {
> /* .. or unplug old. */
>
> - if (networkUnplugBandwidth(obj, iface->bandwidth,
> - iface->data.network.actual ?
> - &iface->data.network.actual->class_id : NULL) < 0)
> - goto cleanup;
> + if (networkUnplugBandwidth(obj, oldBandwidth, class_id) < 0)
> + return -1;
> }
>
> - ret = 0;
> - cleanup:
> + return 0;
> +}
> +
> +
> +static int
> +networkBandwidthUpdate(virDomainNetDefPtr iface,
> + virNetDevBandwidthPtr newBandwidth)
> +{
> + virNetworkDriverStatePtr driver = networkGetDriver();
> + virNetworkObjPtr obj = NULL;
> + virNetDevBandwidthPtr oldBandwidth = virDomainNetGetActualBandwidth(iface);
> + int ret = -1;
> +
> + if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Expected a interface for a virtual network"));
> + return -1;
> + }
> +
> + if (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 0;
Here's that message again. Shouldn't we have already weeded out any
requests to set/change bandwidth for other network types as invalid (and
logged an error) before we could ever get to this point? (but again,
that's an existing issue, not something new. The comment really bothers
me though :-)
> + }
> +
> + 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 ret;
> + }
> +
> + ret = networkUpdatePortBandwidth(obj,
> + &iface->mac,
> + iface->data.network.actual ?
> + &iface->data.network.actual->class_id : NULL,
> + newBandwidth,
> + oldBandwidth);
> +
> virNetworkObjEndAPI(&obj);
> return ret;
> }
Reviewed-by: Laine Stump <laine at laine.org>
(I unfortunately won't be able to continue with the rest of the series
until Thursday...)
More information about the libvir-list
mailing list