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 }

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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

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...

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(a)redhat.com >> https://www.redhat.com/mailman/listinfo/libvir-list >> > > Erik > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

Up until now, we set memballoon period in monitor successfully, however we did not update domain definition structure, thus dumpxml was omitting period attribute in memballoon element Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140960 --- src/qemu/qemu_driver.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ede8880..d73288a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2460,9 +2460,15 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period, qemuDomainObjEnterMonitor(driver, vm); r = qemuMonitorSetMemoryStatsPeriod(priv->mon, period); qemuDomainObjExitMonitor(driver, vm); - if (r < 0) + if (r < 0) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("unable to set balloon driver collection period")); + goto endjob; + }

+ + vm->def->memballoon->period = period; + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) + goto endjob; } if (flags & VIR_DOMAIN_AFFECT_CONFIG) {

On 10/01/2014 01:55 AM, John Ferlan wrote: > > > On 09/22/2014 06:41 AM, Erik Skultety wrote: >> Up until now, we set memballoon period in monitor successfully, however >> we did not update domain definition structure, thus dumpxml was omitting >> period attribute in memballoon element >> >> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140960 >> --- >> src/qemu/qemu_driver.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >> index ede8880..d73288a 100644 >> --- a/src/qemu/qemu_driver.c >> +++ b/src/qemu/qemu_driver.c >> @@ -2460,9 +2460,15 @@ static int qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period, >> qemuDomainObjEnterMonitor(driver, vm); >> r = qemuMonitorSetMemoryStatsPeriod(priv->mon, period); >> qemuDomainObjExitMonitor(driver, vm); >> - if (r < 0) >> + if (r < 0) { >> virReportError(VIR_ERR_OPERATION_INVALID, "%s", >> _("unable to set balloon driver collection period")); >> + goto endjob; >> + } > > I'm trying to remember if there was a reason for not jumping to error. > It probably has to do with "at some point in time" during development > this setting would/could be done through calls via qemu_process.c and > causing a failure through that path wasn't good. Now since this only > accessible via a virsh command - I agree going to endjob is right... > > If you care to walk the history - start here: > > http://www.redhat.com/archives/libvir-list/2013-July/msg00770.html I had a little look at the history you pointed out. From what I've found, the sub-element 'period' was introduced along with qemu driver-specific API and libvirt domain API in 10-part v3 patch series (http://www.redhat.com/archives/libvir-list/2013-July/msg00774.html). I detached HEAD before these commits were pushed and I didn't find any trace about memballoon period setting. All the APIs and necessary structures/members were introduced by the patch series mentioned above, is that correct? :) (in that case I guess it's fine to add the jump to 'endjob' label)

> ACK (to what's here) > > John >> + >> + vm->def->memballoon->period = period; >> + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) >> + goto endjob; >> } >> >> if (flags & VIR_DOMAIN_AFFECT_CONFIG) { >> > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > Erik