[libvirt] [PATCH 0/3] Follow up patches for QoS
Thanks to Peter who made me test this more and find some corner cases. And even take a look on Coverity output. Michal Privoznik (3): networkBandwidthUpdate: Don't blindly dereference pointers networkBandwidthGenericChecks: Drop useless check cmdAttachInterface: Fully implement @floor support src/network/bridge_driver.c | 7 ++----- tools/virsh-domain.c | 8 ++++++-- 2 files changed, 8 insertions(+), 7 deletions(-) -- 2.4.6
It may happen that an interface don't have any bandwidth set and a new one is to be set. In that case, @ifaceBand will be NULL. This will cause troubles later in the code when deciding what to do. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 666078c..f57c400 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -5048,8 +5048,8 @@ networkBandwidthUpdate(virDomainNetDefPtr iface, /* Okay, there are three possible scenarios: */ - if (ifaceBand->in && ifaceBand->in->floor && - newBandwidth->in && newBandwidth->in->floor) { + if (ifaceBand && ifaceBand->in && ifaceBand->in->floor && + newBandwidth && newBandwidth->in && newBandwidth->in->floor) { /* Either we just need to update @floor .. */ if (virNetDevBandwidthUpdateRate(network->def->bridge, -- 2.4.6
There's a check right at the beginning of the function that shortcuts if the function was called over all NULL arguments. However, this was meant just as a fool-proof check so that we don't crash if function is used in a bad manner. Anyway, it makes Coverity unhappy as it then thinks any of the arguments could be NULL. Well, with the current state of the code it can't. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index f57c400..c343e5b 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4960,9 +4960,6 @@ networkBandwidthGenericChecks(virDomainNetDefPtr iface, virNetDevBandwidthPtr ifaceBand; unsigned long long old_floor, new_floor; - if (!iface && !newBandwidth) - return false; - if (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_NETWORK) { /* This is not an interface that's plugged into a network. * We don't care. Thus from our POV bandwidth change is allowed. */ -- 2.4.6
On 08/12/2015 05:13 AM, Michal Privoznik wrote:
There's a check right at the beginning of the function that shortcuts if the function was called over all NULL arguments. However, this was meant just as a fool-proof check so that we don't crash if function is used in a bad manner. Anyway, it makes Coverity unhappy as it then thinks any of the arguments could be NULL. Well, with the current state of the code it can't.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/network/bridge_driver.c | 3 --- 1 file changed, 3 deletions(-)
ACK - it was something I pointed out in my review though ;-) (http://www.redhat.com/archives/libvir-list/2015-August/msg00374.html). Hence why I pointed out to use ATTRIBUTE_NONNULL for both args John
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index f57c400..c343e5b 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4960,9 +4960,6 @@ networkBandwidthGenericChecks(virDomainNetDefPtr iface, virNetDevBandwidthPtr ifaceBand; unsigned long long old_floor, new_floor;
- if (!iface && !newBandwidth) - return false; - if (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_NETWORK) { /* This is not an interface that's plugged into a network. * We don't care. Thus from our POV bandwidth change is allowed. */
In my previous commit d7f5c88961b52 I tried to introduce support for inbound.floor. But the code change was incomplete. This is the change needed to fully enable the feature. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 9f54691..c8b0e76 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1023,12 +1023,16 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) if (inboundStr || outboundStr) { virBufferAddLit(&buf, "<bandwidth>\n"); virBufferAdjustIndent(&buf, 2); - if (inboundStr && inbound.average > 0) { - virBufferAsprintf(&buf, "<inbound average='%llu'", inbound.average); + if (inboundStr && (inbound.average || inbound.floor)) { + virBufferAddLit(&buf, "<inbound"); + if (inbound.average > 0) + virBufferAsprintf(&buf, " average='%llu'", inbound.average); if (inbound.peak > 0) virBufferAsprintf(&buf, " peak='%llu'", inbound.peak); if (inbound.burst > 0) virBufferAsprintf(&buf, " burst='%llu'", inbound.burst); + if (inbound.floor > 0) + virBufferAsprintf(&buf, " floor='%llu'", inbound.floor); virBufferAddLit(&buf, "/>\n"); } if (outboundStr && outbound.average > 0) { -- 2.4.6
On 08/12/2015 05:13 AM, Michal Privoznik wrote:
Thanks to Peter who made me test this more and find some corner cases. And even take a look on Coverity output.
Michal Privoznik (3): networkBandwidthUpdate: Don't blindly dereference pointers networkBandwidthGenericChecks: Drop useless check cmdAttachInterface: Fully implement @floor support
src/network/bridge_driver.c | 7 ++----- tools/virsh-domain.c | 8 ++++++-- 2 files changed, 8 insertions(+), 7 deletions(-)
ACK 1 & 3 too - (1 hasn't come to an inbox near me yet, but I see it in the archive). John
participants (2)
-
John Ferlan -
Michal Privoznik