Re: [libvirt] [PATCH 2/2] qemu: Fix updating balloon period in live XML

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