[libvirt] [PATCH v2] qemu: Set swap_hard_limit before hard_limit

[Martin Kletzander]

On 08/16/2012 12:37 PM, Osier Yang wrote:
> Setting hard_limit larger than previous swap_hard_limit must fail,
> it's not that good if one wants to change the swap_hard_limit
> and hard_limit together. E.g.
> 
> % virsh memtune rhel6
> hard_limit     : 1000000
> soft_limit     : 1000000
> swap_hard_limit: 1000000
> 
> % virsh memtune rhel6 --hard-limit 1000020 --soft-limit 1000020 \
> --swap-hard-limit 1000020 --live
> 
> This patch reoder the limits setting to set the swap_hard_limit
> first, hard_limit then, and soft_limit last if it's greater than
> current swap_hard_limit. And soft_limit first, hard_limit then,
> swap_hard_limit last, if not.
> ---
>  src/qemu/qemu_driver.c |   59 +++++++++++++++++++++++++++++++++++++++++------
>  1 files changed, 51 insertions(+), 8 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 80cfa84..a26d3cd 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6651,11 +6651,18 @@ qemuDomainSetMemoryParameters(virDomainPtr dom,
>      virDomainDefPtr persistentDef = NULL;
>      virCgroupPtr group = NULL;
>      virDomainObjPtr vm = NULL;
> +    virTypedParameterPtr hard_limit = NULL;
> +    virTypedParameterPtr soft_limit = NULL;
> +    virTypedParameterPtr swap_hard_limit = NULL;
> +    virTypedParameterPtr sorted_params[nparams];

You allocate the correct number of params here, but ...

> @@ -6694,13 +6701,49 @@ qemuDomainSetMemoryParameters(virDomainPtr dom,
>          }
>      }
>  
> +    for (i = 0; i < nparams; i++) {
> +        if (STREQ(params[i].field, VIR_DOMAIN_MEMORY_HARD_LIMIT))
> +            hard_limit = &params[i];
> +        else if (STREQ(params[i].field, VIR_DOMAIN_MEMORY_SOFT_LIMIT))
> +            soft_limit = &params[i];
> +        else if (STREQ(params[i].field, VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT))
> +            swap_hard_limit = &params[i];
> +    }
> +
> +    /* It will fail if hard limit greater than swap hard limit anyway */
> +    if (swap_hard_limit &&
> +        hard_limit &&
> +        (hard_limit->value.ul > swap_hard_limit->value.ul)) {
> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
> +                       _("hard limit must be lower than swap hard limit"));
> +        goto cleanup;
> +    }
> +
> +    /* Get current swap hard limit */
> +    rc = virCgroupGetMemSwapHardLimit(group, &val);
> +    if (rc != 0) {
> +        virReportSystemError(-rc, "%s",
> +                             _("unable to get swap hard limit"));
> +        goto cleanup;
> +    }
> +
> +    if (swap_hard_limit->value.ul > val) {
> +        sorted_params[0] = swap_hard_limit;
> +        sorted_params[1] = hard_limit;
> +        sorted_params[2] = soft_limit;
> +    } else {
> +        sorted_params[0] = soft_limit;
> +        sorted_params[1] = hard_limit;
> +        sorted_params[2] = swap_hard_limit;
> +    }

... you always access all 3 of them and you go through this list.
Unfortunately this means that in case you want to change only one value
(e.g. hard limit), you will dereference NULL couple of lines later and
it will crash libvirtd (tried). To test this, you can try running 'virsh
memtune rhel6 --hard-limit 1000020' in your previous scenario.

Martin.