When the <bandwidth> of an interface is changed with update-device,
the old settings are cleared with tc, then new settings added with
tc. But if the <bandwidth has been removed, the old settings weren't
being removed, so the bandwidth restrictions would still be active on
the interface although the interface status in libvirt showed that
they had been removed.

This patch fixes it by calling virNetDevBandwidthClear() if the
"modification" to the interface bandwidth was to completely clear
it.

An alternative could have been to modify virNetDevBandwidthSet() to
always clear existing bandwith settings at the beginning of the
function (currently it short circuits in that case, doing nothing),
but that would have led to cases where virNetDevBandwidthClear() was
now being called in cases where it previously wasn't, and while many
of those cases would be NOPs, there could be cases where it would
cause an error. The way this patch works, the ...Clear() function is
only called in cases where the ...Set() function had previously been
called successfully, so the risk of regression is minimized.

  Resolves: https://bugzilla.redhat.com/1454709
---

I'm currently unable to test this fix because
https://bugzilla.redhat.com/1514963 (filed against the iproute
package) is unresolved in the version of Fedora running on my host.

 src/qemu/qemu_hotplug.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 24631cb8f..3658fc20a 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3318,11 +3318,19 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
     }
 
     if (needBandwidthSet) {
-        if (virNetDevBandwidthSet(newdev->ifname,
-                                  virDomainNetGetActualBandwidth(newdev),
-                                  false,
-                                  !virDomainNetTypeSharesHostView(newdev)) < 0)
-            goto cleanup;
+        virNetDevBandwidthPtr newb = virDomainNetGetActualBandwidth(newdev);
+
+        if (newb) {
+            if (virNetDevBandwidthSet(newdev->ifname, newb, false,
+                                      !virDomainNetTypeSharesHostView(newdev)) < 0)
+                goto cleanup;
+        } else {
+            /*
+             * virNetDevBandwidthSet() doesn't clear any existing
+             * setting unless something new is being set.
+             */
+            virNetDevBandwidthClear(newdev->ifname);
+        }
         needReplaceDevDef = true;
     }
 
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 12/13/2017 08:24 PM, Laine Stump wrote:
> When the <bandwidth> of an interface is changed with update-device,
> the old settings are cleared with tc, then new settings added with
> tc. But if the <bandwidth has been removed, the old settings weren't
> being removed, so the bandwidth restrictions would still be active on
> the interface although the interface status in libvirt showed that
> they had been removed.
> 
> This patch fixes it by calling virNetDevBandwidthClear() if the
> "modification" to the interface bandwidth was to completely clear
> it.
> 
> An alternative could have been to modify virNetDevBandwidthSet() to
> always clear existing bandwith settings at the beginning of the
> function (currently it short circuits in that case, doing nothing),
> but that would have led to cases where virNetDevBandwidthClear() was
> now being called in cases where it previously wasn't, and while many
> of those cases would be NOPs, there could be cases where it would
> cause an error. The way this patch works, the ...Clear() function is
> only called in cases where the ...Set() function had previously been
> called successfully, so the risk of regression is minimized.

Yeah. Let's go the way you implement here.

> 
>   Resolves: https://bugzilla.redhat.com/1454709
> ---
> 
> I'm currently unable to test this fix because
> https://bugzilla.redhat.com/1514963 (filed against the iproute
> package) is unresolved in the version of Fedora running on my host.

I'm running a fixed version of iproute and successfully tested this patch.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list