[libvirt PATCH 0/5] qemu: add stricter checks of permissibility of the QoS parameter 'floor'
Aims to fix https://bugzilla.redhat.com/show_bug.cgi?id=1750219 Libvirt previously silently accepted attempts to set 'floor' even for direct bridge interface types where the parameter is not supported. This could happen when manipulating both inactive and active (e.g. via 'virsh domiftune') domain configuration. Pavel Mores (5): qemu: fail on attempt to set 'floor' if interface type is not 'network' qemu: check if 'floor' is supported for given interface and network qemu: call networkPlugBandwidth() for all types of network docs: QoS parameter 'floor' is supported for 'open' networks too qemu: reuse convenience variable introduced in a00b97f27672b3 docs/formatnetwork.html.in | 2 +- src/network/bridge_driver.c | 27 +++++++++++++++++++++------ src/qemu/qemu_driver.c | 13 ++++++++++--- 3 files changed, 32 insertions(+), 10 deletions(-) -- 2.24.1
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@redhat.com> --- 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 2/10/20 5:10 PM, Pavel Mores wrote:
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@redhat.com> --- 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"));
Please break this long line. Michal
On Wed, Feb 12, 2020 at 09:21:05AM +0100, Michal Privoznik wrote:
On 2/10/20 5:10 PM, Pavel Mores wrote:
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@redhat.com> --- 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"));
Please break this long line.
Please do not break this long line as stated in our hacking guide [1]. Pavel [1] <https://libvirt.org/hacking.html#errors>
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; + + floorRequested = ifaceBand && ifaceBand->in && ifaceBand->in->floor != 0; + + 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, -- 2.24.1
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
On Wed, Feb 12, 2020 at 09:20:55AM +0100, Michal Privoznik wrote:
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
In this day and age, there's no need to waste energy by writing the inline keyword - the compiler will do as it pleases anyway (which is why we need the G_GNUC_NO_INLINE marker in the first place). Jano
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; }
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@redhat.com> --- 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
On 2/10/20 5:10 PM, Pavel Mores wrote:
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@redhat.com> --- src/network/bridge_driver.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
I'd mention in the commit message that this is safe to do, because if no QoS is set, then the function is NOP. Or something among those lines. It might help us in the future to pin a point in time when this used to work. Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
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@redhat.com> --- 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><interface type='network'/></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
On 2/10/20 5:10 PM, Pavel Mores wrote:
Relevant code seems to treat forward modes 'route', 'nat', 'open' and none
s/none/'none'/
the same but documentation hasn't reflected that so far.
Signed-off-by: Pavel Mores <pmores@redhat.com> --- docs/formatnetwork.html.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
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@redhat.com> --- 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
On 2/10/20 5:10 PM, Pavel Mores wrote:
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@redhat.com> --- src/network/bridge_driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
libvirt.git $ git describe a00b97f27672b3 fatal: Not a valid object name a00b97f27672b3 ;-)
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"),
As I'm saying in review of 2/5, this won't be needed. Michal
On 2/10/20 5:10 PM, Pavel Mores wrote:
Aims to fix
https://bugzilla.redhat.com/show_bug.cgi?id=1750219
Libvirt previously silently accepted attempts to set 'floor' even for direct bridge interface types where the parameter is not supported. This could happen when manipulating both inactive and active (e.g. via 'virsh domiftune') domain configuration.
Pavel Mores (5): qemu: fail on attempt to set 'floor' if interface type is not 'network' qemu: check if 'floor' is supported for given interface and network qemu: call networkPlugBandwidth() for all types of network docs: QoS parameter 'floor' is supported for 'open' networks too qemu: reuse convenience variable introduced in a00b97f27672b3
docs/formatnetwork.html.in | 2 +- src/network/bridge_driver.c | 27 +++++++++++++++++++++------ src/qemu/qemu_driver.c | 13 ++++++++++--- 3 files changed, 32 insertions(+), 10 deletions(-)
Overall, I think these patches put checks at the right places. But I think the actual code needs to look a bit different. Looking forward to v2. Michal
participants (4)
-
Ján Tomko -
Michal Privoznik -
Pavel Hrdina -
Pavel Mores