On 10/02/2014 05:47 AM, Erik Skultety wrote: > 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) Right - those commits added the period setting. There were a couple of iterations of the work. I don't recall exactly since some time has passed since I did that work; however, I'm trying to post rationalize why I wouldn't have gone to endjob.. The call to virDomainSaveStatus is something I certainly missed. In any case, this hunk does look good. Let me know what you think about the latest response to 1/2 and I can push these for you. John >> 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@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