[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



----- Original Message -----
> From: "Eric Blake" <eblake redhat com>
> To: "Francesco Romani" <fromani redhat com>, libvir-list 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

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 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.

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]