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
>> [email protected]
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
> Erik
>
--
libvir-list mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/libvir-list
