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@redhat.com> --- 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