[libvirt] [PATCH 1/2] LXC: add support for --config in setmaxmem command

[Michal Privoznik]

On 16.07.2014 11:51, Chen Hanxiao wrote:
> In lxc, we could not use setmaxmem command
> with --config options.
> This patch will add support for this.
>
> Signed-off-by: Chen Hanxiao <chenhanxiao at cn.fujitsu.com>
> ---
>   src/lxc/lxc_driver.c | 69 ++++++++++++++++++++++++++++++++++------------------
>   1 file changed, 46 insertions(+), 23 deletions(-)
>
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index b7b4b02..be6ee19 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -721,10 +721,10 @@ static int lxcDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
>       virLXCDomainObjPrivatePtr priv;
>       virLXCDriverPtr driver = dom->conn->privateData;
>       virLXCDriverConfigPtr cfg = NULL;
> -    unsigned long oldmax = 0;
>
>       virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> -                  VIR_DOMAIN_AFFECT_CONFIG, -1);
> +                  VIR_DOMAIN_AFFECT_CONFIG |
> +                  VIR_DOMAIN_MEM_MAXIMUM, -1);
>
>       if (!(vm = lxcDomObjFromDomain(dom)))
>           goto cleanup;
> @@ -743,32 +743,55 @@ static int lxcDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
>                                           &persistentDef) < 0)
>           goto cleanup;
>
> -    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_MEM_MAXIMUM) {
> +        if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> +            if (newmem < vm->def->mem.cur_balloon) {
> +                virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                           _("Cannot resize the max memory less than current"
> +                             " memory for an active domain"));
> +                goto cleanup;
> +            }
> +            vm->def->mem.max_balloon = newmem;

Are things that easy? Don't we need to communicate this with the 
lxc_controler somehow? Even though you allow only extending, I think 
unless we are 100% sure guest will see the resize, we shouldn't allow this.

> +        }
>
> -    if (newmem > oldmax) {
> -        virReportError(VIR_ERR_INVALID_ARG,
> -                       "%s", _("Cannot set memory higher than max memory"));
> -        goto cleanup;
> -    }
> +        if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> +            sa_assert(persistentDef);

Is this assert needed? Did clang complain or is this just a pure lefover 
from copying from qemu_driver.c?

> +            persistentDef->mem.max_balloon = newmem;
> +            if (persistentDef->mem.cur_balloon > newmem)
> +                persistentDef->mem.cur_balloon = newmem;
> +            if (virDomainSaveConfig(cfg->configDir, persistentDef) < 0)
> +                goto cleanup;
> +        }
> +    } else {
> +        unsigned long oldmax = 0;
>
> -    if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> -        if (virCgroupSetMemory(priv->cgroup, newmem) < 0) {
> -            virReportError(VIR_ERR_OPERATION_FAILED,
> -                           "%s", _("Failed to set memory for domain"));
> -            goto cleanup;
> +        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_CONFIG) {
> -        sa_assert(persistentDef);

Well, since it has been here already, I think we can leave it in your 
patch too.

> -        persistentDef->mem.cur_balloon = newmem;
> -        if (virDomainSaveConfig(cfg->configDir, persistentDef) < 0)
> +        if (newmem > oldmax) {
> +            virReportError(VIR_ERR_INVALID_ARG,
> +                           "%s", _("Cannot set memory higher than max memory"));
>               goto cleanup;
> +        }
> +
> +        if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> +            if (virCgroupSetMemory(priv->cgroup, newmem) < 0) {
> +                virReportError(VIR_ERR_OPERATION_FAILED,
> +                               "%s", _("Failed to set memory for domain"));
> +                goto cleanup;
> +            }
> +        }
> +
> +        if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> +            sa_assert(persistentDef);
> +            persistentDef->mem.cur_balloon = newmem;
> +            if (virDomainSaveConfig(cfg->configDir, persistentDef) < 0)
> +                goto cleanup;
> +        }
>       }
>
>       ret = 0;
>

But what I miss in this patch is, what would:

virDomainSetMaxMemoryFlags(dom, newmem, VIR_DOMAIN_AFFECT_CURRENT | 
VIR_DOMAIN_MEM_MAXIMUM)

do? I mean, the _CURRENT is not translated to _LIVE or _CONFIG anywhere 
in the function.

Michal