On 10/01/2013 09:07 AM, Michal Privoznik wrote:

The qemuDomainChangeNet() is called when 'virsh update-device' is invoked on a NIC. Currently, we fail to update the QoS even though we have routines for that.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f06930e..41b942f 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2062,8 +2062,6 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, virDomainNetGetActualDirectMode(olddev) != virDomainNetGetActualDirectMode(olddev) || !virNetDevVPortProfileEqual(virDomainNetGetActualVirtPortProfile(olddev), virDomainNetGetActualVirtPortProfile(newdev)) || - !virNetDevBandwidthEqual(virDomainNetGetActualBandwidth(olddev), - virDomainNetGetActualBandwidth(newdev)) || !virNetDevVlanEqual(virDomainNetGetActualVlan(olddev), virDomainNetGetActualVlan(newdev))) { needReconnect = true; @@ -2081,6 +2079,18 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, goto cleanup; }

+ if (!virNetDevBandwidthEqual(virDomainNetGetActualBandwidth(olddev), + virDomainNetGetActualBandwidth(newdev))) { + if (virNetDevBandwidthSet(newdev->ifname, + virDomainNetGetActualBandwidth(newdev), + false) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot set bandwidth limits on %s"), + newdev->ifname); + goto cleanup; + } + needReplaceDevDef = true; + } if (needBridgeChange) { if (qemuDomainChangeNetBridge(dom->conn, vm, olddev, newdev) < 0) goto cleanup;

qemuDomainChangeNet probably seems a bit mixed up, but the intent was to first perform a bunch of checks and set flags as we determined actions that need to be performed. Then, after all the flags have been set, perform those actions. The reasoning for doing it this way is that in some cases a larger scale action (e.g. unplug and replug the host side of the interface, which isn't actually supported right now due to deficiencies in qemu) would end up implicitly performing the smaller scale actions (e.g. setting the bandwidth) anyway, thus rendering it redundant to do those actions separately (so their boolean would be reset). For that reason, I would prefer if a "needBandwidthSet" boolean was added, and set to true when the old bandwidth doesn't equal the new (up around line 2060), then down below (where you've added the new chunk in this patch) you would just do "if (needBandwidthSet) && virNetDevBandwidthSet(...) < 0) { error } and set needReplaceDevDef. I know that seems like just extra work and confusion for nothing, but if we are ever able to replug the host-side of an interface, it will end up being very useful.