[libvirt] [PATCH 2/3] lxc: Make SetMemory work for active domains only

[Daniel Veillard]

On Tue, May 04, 2010 at 11:56:56AM +0200, Jiri Denemark wrote:
> > > @@ -642,27 +642,30 @@ static int lxcDomainSetMemory(virDomainPtr dom, unsigned long newmem) {
> > >          goto cleanup;
> > >      }
> > >  
> > > -    if (virDomainObjIsActive(vm)) {
> > > -        if (driver->cgroup == NULL) {
> > > -            lxcError(VIR_ERR_NO_SUPPORT,
> > > -                     "%s", _("cgroups must be configured on the host"));
> > > -            goto cleanup;
> > > -        }
> > > +    if (!virDomainObjIsActive(vm)) {
> > > +        lxcError(VIR_ERR_OPERATION_INVALID,
> > > +                 "%s", _("Domain is not running"));
> > > +        goto cleanup;
> > > +    }
> > >  
> > > -        if (virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0) != 0) {
> > > -            lxcError(VIR_ERR_INTERNAL_ERROR,
> > > -                     _("Unable to get cgroup for %s"), vm->def->name);
> > > -            goto cleanup;
> > > -        }
> > > +    if (driver->cgroup == NULL) {
> > > +        lxcError(VIR_ERR_NO_SUPPORT,
> > > +                 "%s", _("cgroups must be configured on the host"));
> > > +        goto cleanup;
> > > +    }
> > >  
> > > -        if (virCgroupSetMemory(cgroup, newmem) < 0) {
> > > -            lxcError(VIR_ERR_OPERATION_FAILED,
> > > -                     "%s", _("Failed to set memory for domain"));
> > > -            goto cleanup;
> > > -        }
> > > -    } else {
> > > -        vm->def->memory = newmem;
> > > +    if (virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0) != 0) {
> > > +        lxcError(VIR_ERR_INTERNAL_ERROR,
> > > +                 _("Unable to get cgroup for %s"), vm->def->name);
> > > +        goto cleanup;
> > > +    }
> > > +
> > > +    if (virCgroupSetMemory(cgroup, newmem) < 0) {
> > > +        lxcError(VIR_ERR_OPERATION_FAILED,
> > > +                 "%s", _("Failed to set memory for domain"));
> > > +        goto cleanup;
> > >      }
> > > +
> > >      ret = 0;
> > >  
> > >  cleanup:
> > 
> > I'm not 100% sure of the patch but the new sequence look more logical,
> > I'm still concerned that the new code seems to not update vm->def->memory
> 
> Hmm, the patch generated by git is a bit confusing. In reality it's quite
> simple... Before the patch, there was a whole bunch of code within "if
> (virDomainObjIsActive(vm))" and "vm->def->memory = newmem;" if the VM wasn't
> active. Now, the function works only for active VMs (which is it's correct
> behavior as it was never supposed to work offline, one has to change domain
> XML to make offline changes), that is "vm->def->memory = newmem;" is replaced
> with VIR_ERR_OPERATION_INVALID error. There is no change in semantics for
> active VMs.

  Ah, okay, I didn't realized the vm->def->memory = newmem was only for
non-running domains,

 ACK,
 
 thanks !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/