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

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

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

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 ?

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

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