Re: [libvirt PATCH 2/5] qemu: check if 'floor' is supported for given interface and network

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 2/10/20 5:10 PM, Pavel Mores wrote:
> Even if an interface of type 'network', setting 'floor' is only supported
> if the network's forward type is nat, route, open or none.
>
> Signed-off-by: Pavel Mores <pmores@xxxxxxxxxx>
> ---
>   src/network/bridge_driver.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 94212eec77..3b70e52afd 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -5062,9 +5062,26 @@ networkCheckBandwidth(virNetworkObjPtr obj,
>       unsigned long long tmp_floor_sum = virNetworkObjGetFloorSum(obj);
>       unsigned long long tmp_new_rate = 0;
>       char ifmac[VIR_MAC_STRING_BUFLEN];
> +    virNetworkForwardType fwdType;
> +    bool floorSupported;
> +    bool floorRequested;
>   
>       virMacAddrFormat(ifaceMac, ifmac);
>   
> +    fwdType = def->forward.type;
> +    floorSupported = fwdType == VIR_NETWORK_FORWARD_NONE ||
> +        fwdType == VIR_NETWORK_FORWARD_NAT ||
> +        fwdType == VIR_NETWORK_FORWARD_ROUTE ||
> +        fwdType == VIR_NETWORK_FORWARD_OPEN;

What if this was turned into a function? For instance:

static inline bool virNetDevSupportBandwidthFloor(virNetworkForwardType 
type)
{
    switch (type) {
    case VIR_NETWORK_FORWARD_NONE:
    case VIR_NETWORK_FORWARD_NAT:
    case VIR_NETWORK_FORWARD_ROUTE:
    case VIR_NETWORK_FORWARD_OPEN:
        return true;
    case VIR_NETWORK_FORWARD_BRIDGE:
    case VIR_NETWORK_FORWARD_PRIVATE:
    case VIR_NETWORK_FORWARD_VEPA:
    case VIR_NETWORK_FORWARD_PASSTHROUGH:
    case VIR_NETWORK_FORWARD_HOSTDEV:
    case VIR_NETWORK_FORWARD_LAST:
        break;
    }
    return false;
}

which could live next to virNetDevSupportBandwidth(). Then you could 
just call this function instead of having an explicit variable.

> +
> +    floorRequested = ifaceBand && ifaceBand->in && ifaceBand->in->floor != 0;

So this pattern repeats enough times to be turned into a separate 
function IMO. Again, it can be a simple inline function, e.g.:

static inline bool virNetDevBandwidthHasFloor(const virNetDevBandwidth *b)
{
    return b && b->in && b->in->floor != 0;
}

[Side note, out->floor can never be set, as it doesn't make any sense. 
See virNetDevBandwidthParse()]

> +
> +    if (floorRequested && !floorSupported) {
> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                       _("'floor' is only supported for interface type 'network' with forward type 'nat', 'route', 'open' or none"));
> +        return -1;
> +    }
> +
>       if (ifaceBand && ifaceBand->in && ifaceBand->in->floor &&
>           !(netBand && netBand->in)) {
>           virReportError(VIR_ERR_OPERATION_UNSUPPORTED,

Then, I suggest some reordering of patches. Personally, I like cleanup 
patches to go first and only then patches that do something useful, i.e. 
bugfixes, feature implementation.

Michal