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

[John Ferlan]

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


John

>              goto cleanup;
>          }
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index e2abbe0..4458e02 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -2326,18 +2326,19 @@ static int qemuDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
> 
>      } else {
>          /* resize the current memory */
> -        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 endjob;
>          }
> 
> -        if (newmem > oldmax) {
> +        if (flags & VIR_DOMAIN_AFFECT_CONFIG &&
> +            newmem > persistentDef->mem.max_balloon) {
>              virReportError(VIR_ERR_INVALID_ARG, "%s",
> -                           _("cannot set memory higher than max memory"));
> +                           _("max memory of the persistent configuration can't "
> +                             "be less than the current memory"));
>              goto endjob;
>          }
>