----- Original Message -----
> From: "Eric Blake" <eblake@xxxxxxxxxx>
> To: "Francesco Romani" <fromani@xxxxxxxxxx>, libvir-list@xxxxxxxxxx
> Sent: Tuesday, September 2, 2014 11:01:25 PM
> Subject: Re: [PATCH 01/11] qemu: extract helper to get the current balloon
Hi Eric, thanks for the review(s).
> 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@xxxxxxxxxx>
> > ---
> > 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.
Will do.
>
> >
> > 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,
My mistake. I did run 'make syntax-check', I wonder if that was supposed to catch
this.
> 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, ...
I was a bit confused by the mixed styles found in the code.
Will stick with the one you pointed out in this and in future patches.
>
> > +{
> > + 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) {
Will change.
>
> > + *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).
I must have missed. Will fix.
> >
> > +
> > + 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.
Will add comment documenting this. Is this sufficient or there is something
better I could do?
>
> > @@ -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
No need to overlook. Will fix.
>
>
> > - 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.
Looks better to me. Will change in this direction.
Thanks,
--
Francesco Romani
RedHat Engineering Virtualization R & D
Phone: 8261328
IRC: fromani
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list