On 10/02/2013 03:34 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 | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f06930e..275284d 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1799,6 +1799,7 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, bool needFilterChange = false; bool needLinkStateChange = false; bool needReplaceDevDef = false; + bool needBandwidthSet = false; int ret = -1;

if (!devslot || !(olddev = *devslot)) { @@ -2062,8 +2063,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; @@ -2072,6 +2071,10 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, if (olddev->linkstate != newdev->linkstate) needLinkStateChange = true;

+ if (!virNetDevBandwidthEqual(virDomainNetGetActualBandwidth(olddev), + virDomainNetGetActualBandwidth(newdev))) + needBandwidthSet = true; + /* FINALLY - actually perform the required actions */

if (needReconnect) { @@ -2081,6 +2084,19 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, goto cleanup; }

+ + if (needBandwidthSet) { + 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;

ACK

On 10/02/2013 03:34 AM, Michal Privoznik wrote:

So far the virNetDevBandwidthEqual() expected both ->in and ->out items to be allocated for both @a and @b compared. This is not necessary true for all our code. For instance, running 'update-device' twice over a NIC with the very same XML results in SIGSEGV-ing in this function.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdevbandwidth.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c index 42b0a50..17f4fa3 100644 --- a/src/util/virnetdevbandwidth.c +++ b/src/util/virnetdevbandwidth.c @@ -335,16 +335,30 @@ virNetDevBandwidthEqual(virNetDevBandwidthPtr a, return false;

/* in */ - if (a->in->average != b->in->average || - a->in->peak != b->in->peak || - a->in->burst != b->in->burst) + if (a->in) { + if (!b->in) + return false; + + if (a->in->average != b->in->average || + a->in->peak != b->in->peak || + a->in->burst != b->in->burst) + return false; + } else if (b->in) { return false; + }

/*out*/ - if (a->out->average != b->out->average || - a->out->peak != b->out->peak || - a->out->burst != b->out->burst) + if (a->out) { + if (!b->out) + return false; + + if (a->out->average != b->out->average || + a->out->peak != b->out->peak || + a->out->burst != b->out->burst) + return false; + } else if (b->out) { return false; + }

return true; }

ACK. Could this lead to a segv prior to applying the previous patch? Or does it only become a problem once you support bandwidth change in qemuChangeNet? In either case, I think this patch should be pushed upstream *before* patch 1/2, so that we don't create a window in the history where a new segv is introduced (just in case someone is doing a bisect and hits on that particular revision).