[libvirt] [PATCH 01/11] qemu: extract helper to get the current balloon

[Eric Blake]

On 09/02/2014 06:31 AM, Francesco Romani wrote:
> Refactor the code to extract an helper method
> to get the current balloon settings.
> 
> Signed-off-by: Francesco Romani <fromani at redhat.com>
> ---
>  src/qemu/qemu_driver.c | 98 ++++++++++++++++++++++++++++++--------------------
>  1 file changed, 60 insertions(+), 38 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 239a300..bbd16ed 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -168,6 +168,9 @@ static int qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid,
>                            const char *path, int oflags,
>                            bool *needUnlink, bool *bypassSecurityDriver);
>  
> +static int qemuDomainGetBalloonMemory(virQEMUDriverPtr driver,
> +                                      virDomainObjPtr vm,
> +                                      unsigned long *memory);

Forward declarations of non-recursive static functions is usually a sign
that you didn't topologically sort your code correctly.  Just  implement
the function here, instead of later on.

>  
>  virQEMUDriverPtr qemu_driver = NULL;
>  
> @@ -2519,6 +2522,60 @@ static int qemuDomainSendKey(virDomainPtr domain,
>      return ret;
>  }
>  
> +static int qemuDomainGetBalloonMemory(virQEMUDriverPtr driver,
> +                                      virDomainObjPtr vm,
> +                                      unsigned long *memory)

Libvirt style is tending towards two blank lines between functions, and
return type on separate line (although we don't enforce either of these
yet, due to the large existing code base that used other styles), as in:

static int
qemuDomainGetBalloonMemory(virQEMUDriverPtr driver, ...

> +{
> +    int ret = -1;
> +    int err = 0;
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +
> +    if ((vm->def->memballoon != NULL) &&
> +        (vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE)) {

[1] Over-parenthesized.  Sufficient to write:

if (vm->def->memballoon &&
    vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE) {

> +        *memory = vm->def->mem.max_balloon;
> +    } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BALLOON_EVENT)) {
> +        *memory = vm->def->mem.cur_balloon;
> +    } else if (qemuDomainJobAllowed(priv, QEMU_JOB_QUERY)) {
> +        unsigned long long balloon;
> +
> +        if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
> +            goto cleanup;
> +        if (!virDomainObjIsActive(vm))
> +            err = 0;
> +        else {

[2] If one leg of if-else has {}, both legs must have it.  This is
documented in HACKING (and I really ought to add a syntax check that
forbids obvious cases of unbalanced braces).

> 
> +
> + cleanup:
> +    if (vm)
> +        virObjectUnlock(vm);
> +    return ret;
> +}

[3] Ouch.  This function is unlocking vm, even though it did not obtain
the lock.  Which it kind of has to do because of the way that
qemuDomainObjEndJob may end up invalidating vm.  While transfer
semantics are workable, they require good comments at the start of the
function, and making sure that the caller doesn't duplicate the efforts,
nor forget anything else.

> @@ -2526,7 +2583,6 @@ static int qemuDomainGetInfo(virDomainPtr dom,
>      virDomainObjPtr vm;
>      int ret = -1;
>      int err;
> -    unsigned long long balloon;
>  
>      if (!(vm = qemuDomObjFromDomain(dom)))
>          goto cleanup;
> @@ -2549,43 +2605,9 @@ static int qemuDomainGetInfo(virDomainPtr dom,
>      info->maxMem = vm->def->mem.max_balloon;
>  
>      if (virDomainObjIsActive(vm)) {
> -        qemuDomainObjPrivatePtr priv = vm->privateData;
> -
> -        if ((vm->def->memballoon != NULL) &&
> -            (vm->def->memballoon->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE)) {

[1] Then again, your parenthesis...

> -            info->memory = vm->def->mem.max_balloon;
> -        } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BALLOON_EVENT)) {
> -            info->memory = vm->def->mem.cur_balloon;
> -        } else if (qemuDomainJobAllowed(priv, QEMU_JOB_QUERY)) {
> -            if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
> -                goto cleanup;
> -            if (!virDomainObjIsActive(vm))
> -                err = 0;
> -            else {

[2] ...and broken if-else bracing is just code motion.  So I could
overlook those (although cleaning them up at some point in the series,
even if in a separate patch, would still be nice).  But


> -            if (!qemuDomainObjEndJob(driver, vm)) {
> -                vm = NULL;
> -                goto cleanup;
> -            }
> -
> -            if (err < 0) {
> -                /* We couldn't get current memory allocation but that's not
> -                 * a show stopper; we wouldn't get it if there was a job
> -                 * active either
> -                 */
> -                info->memory = vm->def->mem.cur_balloon;
> -            } else if (err == 0) {
> -                /* Balloon not supported, so maxmem is always the allocation */
> -                info->memory = vm->def->mem.max_balloon;
> -            } else {
> -                info->memory = balloon;
> -            }
> -        } else {
> -            info->memory = vm->def->mem.cur_balloon;
> -        }
> +        err = qemuDomainGetBalloonMemory(driver, vm, &info->memory);
> +        if (err)
> +            return err;

[3] the fact that you don't use 'goto cleanup' here, but rely on the
awkward transfer semantics, is just confusing the situation.  I'd rather
avoid transfer semantics, but understand why you used them.  But I'd
still rewrite this as:

err = qemuDomainGetBalloonMemory(...);
vm = NULL;

and fall through to the cleanup label, rather than doing an early
return, to make it obvious that the call did a transfer of vm.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 539 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140902/99901b29/attachment-0001.sig>