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

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(a)huawei.com&gt; >>> >>> 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(a)huawei.com&gt; >>> Signed-off-by: Zhang Bo <oscar.zhangbo(a)huawei.com&gt; >>> --- >>> .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.

Peter