[libvirt] [PATCH 2/7] qemu: lxc: Clarify error message when setting current memory

[Peter Krempa]

On Fri, Feb 20, 2015 at 08:16:17 -0500, John Ferlan wrote:
> On 02/18/2015 09:16 AM, Peter Krempa wrote:
> > Commit 60f7303c151cccdbe214b9f9ac59ecaf95cbf24b introduced the error
> > message but it's unclear whether the persistent config or the live
> > config tripped the message. Later the LXC driver copied the same code.
> > 
> > Separate the message which will also clarify the code.
> > ---
> >  src/lxc/lxc_driver.c   | 21 +++++++++++----------
> >  src/qemu/qemu_driver.c | 19 ++++++++++---------
> >  2 files changed, 21 insertions(+), 19 deletions(-)
> > 
> > diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> > index 3adb21d..5af0554 100644
> > --- a/src/lxc/lxc_driver.c
> > +++ b/src/lxc/lxc_driver.c
> > @@ -742,18 +742,19 @@ static int lxcDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
> >                  goto cleanup;
> >          }
> >      } else {
> > -        unsigned long oldmax = 0;
> > -
> > -        if (flags & VIR_DOMAIN_AFFECT_LIVE)
> > -            oldmax = vm->def->mem.max_balloon;
> > -        if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> > -            if (!oldmax || oldmax > persistentDef->mem.max_balloon)
> > -                oldmax = persistentDef->mem.max_balloon;
> > +        if (flags & VIR_DOMAIN_AFFECT_LIVE &&
> > +            newmem > vm->def->mem.max_balloon) {
> > +            virReportError(VIR_ERR_INVALID_ARG, "%s",
> > +                           _("max memory of the live instance can't be less "
> > +                             "than the current memory"));
> > +            goto cleanup;
> >          }
> > 
> > -        if (newmem > oldmax) {
> > -            virReportError(VIR_ERR_INVALID_ARG,
> > -                           "%s", _("Cannot set memory higher than max memory"));
> > +        if (flags & VIR_DOMAIN_AFFECT_CONFIG &&
> > +            newmem > persistentDef->mem.max_balloon) {
> > +            virReportError(VIR_ERR_INVALID_ARG, "%s",
> > +                           _("max memory of the persistent configuration can't "
> > +                             "be less than the current memory"));
> 
> This reads awkwardly... This is the non maxmem path - that is we're just
> setting memory and the comparison is that if the new memory value is
> greater than the defined maximum, then there is an error.
> 
> Could be done in one (somewhat ugly) statement too...

I specifically wanted to avoid one statement ...

> 
>     if ((flags & VIR_DOMAIN_AFFECT_LIVE &&
>          newmem > vm->def->mem.max_balloon) ||
>         (flags & VIR_DOMAIN_AFFECT_CONFIG &&
>          newmem > persistentDef->mem.max_balloon)) {
>         unsigned long maxmem = flags & VIR_DOMAIN_AFFECT_LIVE ?
>                                vm->def->mem.max_balloon :
>                                persistentDef->mem.max_balloon;a

... as you need an intermediate variable ...


>         virReportError(VIR_ERR_INVALID_ARG,
>                        _("new memory value '%lu' cannot be greater "
>                          "than the maximum value '%lu' for the %s"),
>                        newmem, maxmem, flags & VIR_DOMAIN_AFFECT_LIVE ?

... ternary operators ...

>                        _("live instance") :
>                        _("persistent configuration"));

... and hard to translate strings.

> 
> ACK on the idea and I'm sure you'll be able to do the message properly.
> I did try to do a "cond ? arg2, arg3 : arg2, arg3" instead of setting
> maxmem, but wasn't successful in a quick attempt...

I'll try to improve the error message but I will not change the
two-condition approach.

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/20150224/4f7fc9a6/attachment-0001.sig>