Re: [libvirt PATCH 2/5] qemu: check if 'floor' is supported for given interface and network
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(a)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