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

From: "Eric Blake" <eblake(a)redhat.com&gt; To: "Francesco Romani" <fromani(a)redhat.com&gt;, libvir-list(a)redhat.com Sent: Tuesday, September 2, 2014 11:01:25 PM Subject: Re: [libvirt] [PATCH 01/11] qemu: extract helper to get the current balloon

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(a)redhat.com&gt; > --- > 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.