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

[Peter Krempa]

On Thu, Jun 04, 2015 at 07:10:58 -0400, John Ferlan wrote:
> 
> 
> 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.

I intended to put the assignment inside of the block, rather than
outside. I'll move it inside.

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150604/4117fef4/attachment-0001.sig>