On 10/01/2014 05:39 PM, John Ferlan wrote:
On 10/01/2014 11:17 AM, Erik Skultety wrote:
On 10/01/2014 01:55 AM, John Ferlan wrote:
On 09/22/2014 06:41 AM, Erik Skultety wrote:
When trying to update bandwidth limits on a running domain, limits get
updated in our internal structures, however XML parser reads
bandwidth limits from network 'actual' definition. Commiting this patch
s/Commiting/Committing
it is now available to update bandwidth 'actual' definition as well,
thus updating domain runtime XML
---
src/qemu/qemu_driver.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 702d3cc..ede8880 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10028,7 +10028,19 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
} else {
net->bandwidth = NULL;
}
+
+ if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
+ virNetDevBandwidthFree(net->data.network.actual->bandwidth);
This will set net->data.network.actual->bandwidth to NULL
It's also remove it when net->bandwidth == NULL thus
causing actual to be lost, which it doesn't seem is
desired, but perhaps it is. Gues
+ if (!net->bandwidth ||
+ virNetDevBandwidthCopy(&net->data.network.actual->bandwidth,
+ net->bandwidth) < 0)
+ net->data.network.actual->bandwidth = NULL;
Making this irrelevant, but I wonder if the < 0 here meant to do
something else perhaps?
+ }
The above hunk needs some space formatting. Also since the
virNetDevBandwidthCopy() has a if (!src) check, "(!net->bandwidth) ||"
is unnecessary.
if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
virNetDevBandwidthFree(net->data.network.actual->bandwidth);
if (virNetDevBandwidthCopy(&net->data.network.actual->bandwidth,
net->bandwidth) < 0)
goto cleanup
}
Looks better, thanks Jon, but you'd still loose 'actual' in the hunk
above, maybe add a check like
if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
net->bandwidth)
if you want to preserve 'actual' when net->bandwidth is NULL??
Right - I thought about that too, but then I thought the purpose of
making the change to actual was to copy what happened to net->bandwidth.
Looking at the hunk just above:
virNetDevBandwidthFree(net->bandwidth);
if (newBandwidth->in || newBandwidth->out) {
net->bandwidth = newBandwidth;
newBandwidth = NULL;
} else {
net->bandwidth = NULL;
}
That says to me in this live path, we're freeing net->bandwidth and only
replacing it with 'newBandwidth' if in/out were found.
Thus, it seems reasonable that we'd want to remove the bandwidth from
actual in this case as well which we wouldn't do if the the "&&
net->bandwidth" was added to the condition.
Also, upon further reflection the "net->data.network.actual->bandwidth =
NULL;" after the virNetDevBandwidthFree() will be necessary since we're
passing by value and not reference...
Hmm, I think this last thing you mentioned won't be necessary at all, if
you further inspect virNetDevBandwidthCopy(), we're passing by reference
and the second action that takes place is assigning NULL to destination
pointer. Sent v2 series.
Erik
John
Whether this is what is "expected" is perhaps something Laine can answer...
John
+
+ if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
+ goto cleanup;
}
+
if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
if (!persistentNet->bandwidth) {
persistentNet->bandwidth = bandwidth;
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
Erik
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list