[libvirt] [PATCH] qemu: fix unsuitable error report when get memory stats

[Wang Yufei]

On 2015/6/1 15:36, Peter Krempa wrote:

> On Sat, May 30, 2015 at 09:35:35 +0800, Wang Yufei wrote:
>> On 2015/5/29 19:16, Peter Krempa wrote:
>>
>>> On Fri, May 29, 2015 at 17:17:07 +0800, Wang Yufei wrote:
>>>> From: Zhang Bo <oscar.zhangbo at huawei.com>
>>>>
>>>> when we run the command 'virsh dommemstat xxx',
>>>> althrough memballoon's model is set 'none' in vm's XML,
>>>> it still reports an error in libvirtd.log.
>>>> error : qemuMonitorFindBalloonObjectPath:1042 : internal error: Cannot determine balloon device path
>>>> Apparently, if we don't set memballoon, we don't need to
>>>> set balloon device path.
> 
> So this patch is fixing a logged error message in case the memory
> balloon was not configured.
> 
>>>>
>>>> Signed-off-by: Wang Yufei <james.wangyufei at huawei.com>
>>>> Signed-off-by: Zhang Bo <oscar.zhangbo at huawei.com>
>>>> ---
>>>>  .gnulib                 | 2 +-
>>>>  src/qemu/qemu_monitor.c | 4 +++-
>>>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/.gnulib b/.gnulib
>>>> index 875ec93..106a386 160000
>>>> --- a/.gnulib
>>>> +++ b/.gnulib
>>>> @@ -1 +1 @@
>>>> -Subproject commit 875ec93e1501d2d2a8bab1b64fa66b8ceb51dc67
>>>> +Subproject commit 106a3866d01f9dd57ab4f10dbeb0d5a8db73a9f7
>>>
>>> Regular patches should not change the gnulib commit id. Also it is
>>> probably decreasing version of gnulib. Please make sure to update the
>>> submodule before commiting code.
>>>
>>
>> OK, I'll check it, thanks.
>>
>>>> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
>>>> index f959b74..c72702d 100644
>>>> --- a/src/qemu/qemu_monitor.c
>>>> +++ b/src/qemu/qemu_monitor.c
>>>> @@ -1711,7 +1711,9 @@ qemuMonitorGetMemoryStats(qemuMonitorPtr mon,
>>>>      QEMU_CHECK_MONITOR(mon);
>>>>  
>>>>      if (mon->json) {
>>>> -        ignore_value(qemuMonitorFindBalloonObjectPath(mon, "/"));
>>>> +        if (mon->vm->def->memballoon &&
>>>> +            mon->vm->def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_NONE)
>>>> +            ignore_value(qemuMonitorFindBalloonObjectPath(mon, "/"));
>>>
>>> The qemu monitor code is not the right place to do this check. The API
>>> that is calling the function should make sure that it makes sense to do
>>> this call.
>>>
>>
>> In my opinion, no matter whether we set up memballoon, we both can call
>> qemuMonitorGetMemoryStats to get mem stats. There're two situation to get mem
>> stats in qemuMonitorGetMemoryStats: one with memballoon, the other without
>> memballoon. Right?
>>
>> If we judge the memballoon in the caller 'qemuDomainMemoryStats',
>>
>>         qemuDomainObjEnterMonitor(driver, vm);
>>         ret = qemuMonitorGetMemoryStats(priv->mon, stats, nr_stats);
>>         if (qemuDomainObjExitMonitor(driver, vm) < 0)
>>
>> what should we do? Supply two different functions to get mem stats?
> 
> The function that is actually requesting the stats from the monitor is
> handling well if the monitor does not exist. In my opinion we should
> make it optional for qemuMonitorFindBalloonObjectPath to report errors.
> 
> Since it's used only in two places that should be an easy enough fix.
> 

If there's no balloon set, we still call qemuMonitorFindBalloonObjectPath to find
the path, which is a little strange logic.

> Peter