Re: [PATCH 4/9] bridge_driver: Introduce networkBandwidthUpdate

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxx>
> ---
>  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);
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]