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

-----Original Message----- From: Michal Privoznik [mailto:mprivozn@redhat.com] Sent: Thursday, July 24, 2014 5:00 PM To: Chen, Hanxiao/陈 晗霄; libvir-list(a)redhat.com Subject: Re: [libvirt] [PATCH 1/2] LXC: add support for --config in setmaxmem command 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(a)cn.fujitsu.com&gt; > --- > 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.