Re: [libvirt] [PATCH 16/35] qemu: Add helper to update domain balloon size and refactor usage places

On 06/03/2015 09:16 AM, Ján Tomko wrote: > On Fri, May 29, 2015 at 03:33:37PM +0200, Peter Krempa wrote: >> When qemu does not support the balloon event the current memory size >> needs to be queried. Since there are two places that implement the same >> logic, split it out into a function and reuse. >> --- >> src/qemu/qemu_domain.c | 64 ++++++++++++++++++++++++++++++++++++++ >> src/qemu/qemu_domain.h | 3 ++ >> src/qemu/qemu_driver.c | 84 +++++--------------------------------------------- >> 3 files changed, 75 insertions(+), 76 deletions(-) >> >> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c >> index db8554b..661181f 100644 >> --- a/src/qemu/qemu_domain.c >> +++ b/src/qemu/qemu_domain.c >> @@ -3182,3 +3182,67 @@ qemuDomainMachineIsI440FX(const virDomainDef *def) >> STRPREFIX(def->os.machine, "pc-i440") || >> STRPREFIX(def->os.machine, "rhel")); >> } >> + >> + >> +/** >> + * qemuDomainUpdateCurrentMemorySize: >> + * >> + * Updates the current balloon size from the monitor if necessary. In case when >> + * the balloon is not present for the domain, the function recalculates the >> + * maximum size to reflect possible changes. >> + * >> + * Returns 0 on success and updates vm->def->mem.cur_balloon if necessary, -1 on >> + * error and reports libvirt error. >> + */ >> +int >> +qemuDomainUpdateCurrentMemorySize(virQEMUDriverPtr driver, >> + virDomainObjPtr vm) >> +{ >> + qemuDomainObjPrivatePtr priv = vm->privateData; >> + unsigned long long balloon; >> + int ret = -1; >> + >> + /* inactive domain doesn't need size update */ >> + if (!virDomainObjIsActive(vm)) >> + return 0; >> + >> + /* if no balloning is available, the current size equals to the current >> + * full memory size */ >> + if (!vm->def->memballoon || >> + vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE) { >> + vm->def->mem.cur_balloon = virDomainDefGetMemoryActual(vm->def); >> + return 0; >> + } >> + >> + /* current size is always automagically updated via the event */ >> + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BALLOON_EVENT)) >> + return 0; >> + >> + /* here we need to ask the monitor */ >> + >> + /* Don't delay if someone's using the monitor, just use existing most >> + * recent data instead */ >> + if (qemuDomainJobAllowed(priv, QEMU_JOB_QUERY)) { >> + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) >> + return -1; >> + >> + if (!virDomainObjIsActive(vm)) { >> + virReportError(VIR_ERR_OPERATION_INVALID, "%s", >> + _("domain is not running")); >> + goto endjob; >> + } >> + >> + qemuDomainObjEnterMonitor(driver, vm); >> + ret = qemuMonitorGetBalloonInfo(priv->mon, &balloon); >> + if (qemuDomainObjExitMonitor(driver, vm) < 0) >> + ret = -1; >> + >> + endjob: >> + qemuDomainObjEndJob(driver, vm); >> + >> + if (ret < 0) >> + return -1; > > ACK if you actually use the 'balloon' value to update cur_balloon here. > > Jan > Making Coverity unhappy... 3215 qemuDomainObjPrivatePtr priv = vm->privateData; (1) Event var_decl: Declaring variable "balloon" without initializer. Also see events: [uninit_use] 3216 unsigned long long balloon; ... 238 * recent data instead */ (9) Event cond_false: Condition "qemuDomainJobAllowed(priv, QEMU_JOB_QUERY)", taking false branch 3239 if (qemuDomainJobAllowed(priv, QEMU_JOB_QUERY)) { ... 3258 return -1; (10) Event if_end: End of if statement 3259 } 3260 (11) Event uninit_use: Using uninitialized value "balloon". Also see events: [var_decl] 3261 vm->def->mem.cur_balloon = balloon; 3262 So if QEMU_JOB_QUERY is not allowed, then balloon is not initialized and setting to cur_balloon is bad Adding an "if (ret == 0)" prior to the setting works.