10:26 a.m.
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
10:26 a.m.
New subject: [libvirt PATCH v2 1/6] qemu: test if bandwidth has 'floor' factored out to separate function
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(a)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
10:31 a.m.
New subject: [libvirt PATCH v2 1/6] qemu: test if bandwidth has 'floor' factored out to separate function
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(a)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
10:26 a.m.
New subject: [libvirt PATCH v2 2/6] qemu: add function to test if network supports setting 'floor'
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(a)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
10:31 a.m.
New subject: [libvirt PATCH v2 2/6] qemu: add function to test if network supports setting 'floor'
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(a)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
10:26 a.m.
New subject: [libvirt PATCH v2 3/6] qemu: fail on attempt to set 'floor' if interface type is not 'network'
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>
---
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
10:26 a.m.
New subject: [libvirt PATCH v2 4/6] qemu: check if 'floor' is supported for given interface and network
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 | 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
10:26 a.m.
New subject: [libvirt PATCH v2 5/6] qemu: call networkPlugBandwidth() for all types of network
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(a)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
10:26 a.m.
New subject: [libvirt PATCH v2 6/6] docs: QoS parameter 'floor' is supported for 'open' networks too
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>
---
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
10:30 a.m.
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(a)redhat.com>
Michal
1743
days inactive
1746
days old
9 comments
2 participants
participants (2)
-
Michal Privoznik -
Pavel Mores