QoS 'floor' setting is documented to be only supported for interfaces of type 'network'. Fail with an error message on attempt to set 'floor' on an interface of any other type. Signed-off-by: Pavel Mores <pmores(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e69d083836..88fa56da42 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11672,9 +11672,16 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, sizeof(*newBandwidth->out)); } - if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK && - virDomainNetBandwidthUpdate(net, newBandwidth) < 0) - goto endjob; + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { + if (virDomainNetBandwidthUpdate(net, newBandwidth) < 0) + goto endjob; + } else { + if (bandwidth->in && bandwidth->in->floor != 0) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("'floor' is only supported for interface type 'network' with forward type 'nat', 'route', 'open' or none")); + goto endjob; + } + } if (virNetDevBandwidthSet(net->ifname, newBandwidth, false, !virDomainNetTypeSharesHostView(net)) < 0) { -- 2.24.1

On Wed, Feb 12, 2020 at 09:21:05AM +0100, Michal Privoznik wrote: Please do not break this long line as stated in our hacking guide [1]. Pavel [1] <https://libvirt.org/hacking.html#errors>

On 2/10/20 5:10 PM, Pavel Mores wrote: 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. 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()] 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

To fix the actual bug, it was necessary to make networkPlugBandwidth() be called also for 'bridge'-type networks implemented using macvtap's 'bridge' mode (previously it was only called for those implemented on top of an existing bridge). However, it seems beneficial to call it for other network types as well, at least because it removes an inconsistency in types of bandwidth configuration changes permissible in inactive and active domain configs. Signed-off-by: Pavel Mores <pmores(a)redhat.com&gt; --- src/network/bridge_driver.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 3b70e52afd..513ae59e68 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4571,8 +4571,6 @@ networkAllocatePort(virNetworkObjPtr obj, return -1; } - if (networkPlugBandwidth(obj, &port->mac, port->bandwidth, &port->class_id) < 0) - return -1; break; case VIR_NETWORK_FORWARD_HOSTDEV: { @@ -4637,8 +4635,6 @@ networkAllocatePort(virNetworkObjPtr obj, } } - if (networkPlugBandwidth(obj, &port->mac, port->bandwidth, &port->class_id) < 0) - return -1; break; } @@ -4736,6 +4732,9 @@ networkAllocatePort(virNetworkObjPtr obj, return -1; } + if (networkPlugBandwidth(obj, &port->mac, port->bandwidth, &port->class_id) < 0) + return -1; + if (virNetworkObjMacMgrAdd(obj, driver->dnsmasqStateDir, port->ownername, &port->mac) < 0) return -1; -- 2.24.1

Relevant code seems to treat forward modes 'route', 'nat', 'open' and none the same but documentation hasn't reflected that so far. Signed-off-by: Pavel Mores <pmores(a)redhat.com&gt; --- docs/formatnetwork.html.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 2448fb09e7..3d807ecab6 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -631,7 +631,7 @@ goes through one point where QoS decisions can take place, hence why this attribute works only for virtual networks for now (that is <code>&lt;interface type='network'/&gt;</code> with a - forward type of route, nat, or no forward at all). Moreover, the + forward type of route, nat, open or no forward at all). Moreover, the virtual network the interface is connected to is required to have at least inbound QoS set (<code>average</code> at least). If using the <code>floor</code> attribute users don't need to specify -- 2.24.1

Since we have the result of a compound condition named now we can use it to simplify another complex condition to make it more readable. Signed-off-by: Pavel Mores <pmores(a)redhat.com&gt; --- src/network/bridge_driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 513ae59e68..584c78cadb 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -5081,8 +5081,7 @@ networkCheckBandwidth(virNetworkObjPtr obj, return -1; } - if (ifaceBand && ifaceBand->in && ifaceBand->in->floor && - !(netBand && netBand->in)) { + if (floorRequested && !(netBand && netBand->in)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("Invalid use of 'floor' on interface with MAC " "address %s - network '%s' has no inbound QoS set"), -- 2.24.1