[libvirt PATCH v2 0/6] qemu: add stricter checks of permissibility of the QoS parameter 'floor'
v2 is mostly just integrating requests from Michal's review. The initial two commits introduce new utility functions to be used in the following two commits. The final two commits have no substantial changes since v1. The only exception are long lines caused by error messages which stay unbroken in v2 as per libvirt's contributor's guidelines (as was also pointed out during review). Pavel Mores (6): qemu: test if bandwidth has 'floor' factored out to separate function qemu: add function to test if network supports setting 'floor' 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 docs/formatnetwork.html.in | 2 +- src/conf/netdev_bandwidth_conf.h | 27 +++++++++++++++++++++++++++ src/network/bridge_driver.c | 27 +++++++++++++++++++-------- src/qemu/qemu_driver.c | 18 +++++++++++++++--- 4 files changed, 62 insertions(+), 12 deletions(-) -- 2.24.1
This compound condition will be useful in several places so it makes sense to give it a name for better readability. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/conf/netdev_bandwidth_conf.h | 7 +++++++ src/network/bridge_driver.c | 8 ++++---- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/conf/netdev_bandwidth_conf.h b/src/conf/netdev_bandwidth_conf.h index 0004e48a4a..5b5ed52566 100644 --- a/src/conf/netdev_bandwidth_conf.h +++ b/src/conf/netdev_bandwidth_conf.h @@ -23,6 +23,7 @@ #include "virbuffer.h" #include "virxml.h" #include "domain_conf.h" +#include "network_conf.h" int virNetDevBandwidthParse(virNetDevBandwidthPtr *bandwidth, unsigned int *class_id, @@ -57,3 +58,9 @@ static inline bool virNetDevSupportBandwidth(virDomainNetType type) } return false; } + + +static inline bool virNetDevBandwidthHasFloor(const virNetDevBandwidth *b) +{ + return b && b->in && b->in->floor != 0; +} diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 94212eec77..14976c9821 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -5065,8 +5065,7 @@ networkCheckBandwidth(virNetworkObjPtr obj, virMacAddrFormat(ifaceMac, ifmac); - if (ifaceBand && ifaceBand->in && ifaceBand->in->floor && - !(netBand && netBand->in)) { + if (virNetDevBandwidthHasFloor(ifaceBand) && !(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"), @@ -5079,8 +5078,9 @@ networkCheckBandwidth(virNetworkObjPtr obj, /* no QoS required, claim success */ return 1; } - if (((!ifaceBand || !ifaceBand->in || !ifaceBand->in->floor) && - (!oldBandwidth || !oldBandwidth->in || !oldBandwidth->in->floor))) { + if (!virNetDevBandwidthHasFloor(ifaceBand) && + !virNetDevBandwidthHasFloor(oldBandwidth)) { + VIR_DEBUG("No old/new interface bandwidth floor"); /* no QoS required, claim success */ return 1; -- 2.24.1
On 2/14/20 5:26 PM, Pavel Mores wrote:
This compound condition will be useful in several places so it makes sense to give it a name for better readability.
Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/conf/netdev_bandwidth_conf.h | 7 +++++++ src/network/bridge_driver.c | 8 ++++---- 2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/conf/netdev_bandwidth_conf.h b/src/conf/netdev_bandwidth_conf.h index 0004e48a4a..5b5ed52566 100644 --- a/src/conf/netdev_bandwidth_conf.h +++ b/src/conf/netdev_bandwidth_conf.h @@ -23,6 +23,7 @@ #include "virbuffer.h" #include "virxml.h" #include "domain_conf.h" +#include "network_conf.h"
This is not needed.
int virNetDevBandwidthParse(virNetDevBandwidthPtr *bandwidth, unsigned int *class_id, @@ -57,3 +58,9 @@ static inline bool virNetDevSupportBandwidth(virDomainNetType type) } return false; } + + +static inline bool virNetDevBandwidthHasFloor(const virNetDevBandwidth *b) +{ + return b && b->in && b->in->floor != 0; +}
Since Jano opposed this in v1, I'm making it regular function.
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 94212eec77..14976c9821 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -5065,8 +5065,7 @@ networkCheckBandwidth(virNetworkObjPtr obj,
virMacAddrFormat(ifaceMac, ifmac);
- if (ifaceBand && ifaceBand->in && ifaceBand->in->floor && - !(netBand && netBand->in)) { + if (virNetDevBandwidthHasFloor(ifaceBand) && !(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"), @@ -5079,8 +5078,9 @@ networkCheckBandwidth(virNetworkObjPtr obj, /* no QoS required, claim success */ return 1; } - if (((!ifaceBand || !ifaceBand->in || !ifaceBand->in->floor) && - (!oldBandwidth || !oldBandwidth->in || !oldBandwidth->in->floor))) { + if (!virNetDevBandwidthHasFloor(ifaceBand) && + !virNetDevBandwidthHasFloor(oldBandwidth)) { + VIR_DEBUG("No old/new interface bandwidth floor"); /* no QoS required, claim success */ return 1;
Michal
This function will be useful in upcoming commits when code to check whether a network can support 'floor' in the first place is introduced. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/conf/netdev_bandwidth_conf.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/conf/netdev_bandwidth_conf.h b/src/conf/netdev_bandwidth_conf.h index 5b5ed52566..0596d555e5 100644 --- a/src/conf/netdev_bandwidth_conf.h +++ b/src/conf/netdev_bandwidth_conf.h @@ -60,6 +60,26 @@ static inline bool virNetDevSupportBandwidth(virDomainNetType type) } +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; +} + + static inline bool virNetDevBandwidthHasFloor(const virNetDevBandwidth *b) { return b && b->in && b->in->floor != 0; -- 2.24.1
On 2/14/20 5:26 PM, Pavel Mores wrote:
This function will be useful in upcoming commits when code to check whether a network can support 'floor' in the first place is introduced.
Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/conf/netdev_bandwidth_conf.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/src/conf/netdev_bandwidth_conf.h b/src/conf/netdev_bandwidth_conf.h index 5b5ed52566..0596d555e5 100644 --- a/src/conf/netdev_bandwidth_conf.h +++ b/src/conf/netdev_bandwidth_conf.h @@ -60,6 +60,26 @@ static inline bool virNetDevSupportBandwidth(virDomainNetType type) }
+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; +} + + static inline bool virNetDevBandwidthHasFloor(const virNetDevBandwidth *b) { return b && b->in && b->in->floor != 0;
This should be merged with 4/6 and per Jano's suggestion turned into regular function. Michal
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 | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2813f084cd..f686b858cf 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11672,9 +11672,21 @@ 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 (virNetDevBandwidthHasFloor(bandwidth)) { + char ifmac[VIR_MAC_STRING_BUFLEN]; + + virMacAddrFormat(&net->mac, ifmac); + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("Invalid use of 'floor' on interface with MAC address %s " + "- 'floor' is only supported for interface type 'network' with forward type 'nat', 'route', 'open' or none"), + ifmac); + goto endjob; + } + } if (virNetDevBandwidthSet(net->ifname, newBandwidth, false, !virDomainNetTypeSharesHostView(net)) < 0) { -- 2.24.1
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 | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 14976c9821..72220e1c64 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -5065,6 +5065,16 @@ networkCheckBandwidth(virNetworkObjPtr obj, virMacAddrFormat(ifaceMac, ifmac); + if (virNetDevBandwidthHasFloor(ifaceBand) && + !virNetDevSupportBandwidthFloor(def->forward.type)) { + + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("Invalid use of 'floor' on interface with MAC address %s " + "- 'floor' is only supported for interface type 'network' with forward type 'nat', 'route', 'open' or none"), + ifmac); + return -1; + } + if (virNetDevBandwidthHasFloor(ifaceBand) && !(netBand && netBand->in)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("Invalid use of 'floor' on interface with MAC " -- 2.24.1
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. It should also be safe as the function pretty much amounts to NOP if no QoS is requested and the new behaviour should not be any worse than before if it is. Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/network/bridge_driver.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 72220e1c64..c8f7f07acb 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,11 @@ 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@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/14/20 5:26 PM, Pavel Mores wrote:
v2 is mostly just integrating requests from Michal's review. The initial two commits introduce new utility functions to be used in the following two commits. The final two commits have no substantial changes since v1.
The only exception are long lines caused by error messages which stay unbroken in v2 as per libvirt's contributor's guidelines (as was also pointed out during review).
Pavel Mores (6): qemu: test if bandwidth has 'floor' factored out to separate function qemu: add function to test if network supports setting 'floor' 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
docs/formatnetwork.html.in | 2 +- src/conf/netdev_bandwidth_conf.h | 27 +++++++++++++++++++++++++++ src/network/bridge_driver.c | 27 +++++++++++++++++++-------- src/qemu/qemu_driver.c | 18 +++++++++++++++--- 4 files changed, 62 insertions(+), 12 deletions(-)
I'm fixing all the small issues I've raised and pushing. Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Privoznik -
Pavel Mores