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

On Mon, May 03, 2010 at 02:20:27PM +0200, jdenemar(a)redhat.com wrote: > From: Jiri Denemark <jdenemar(a)redhat.com&gt; > > --- >  src/lxc/lxc_driver.c |   39 +++++++++++++++++++++------------------ >  1 files changed, 21 insertions(+), 18 deletions(-) > > diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c > index f118b7d..6884fea 100644 > --- a/src/lxc/lxc_driver.c > +++ b/src/lxc/lxc_driver.c > @@ -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

 ACK, once double-checked it's not needed :-) Daniel -- Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/ daniel(a)veillard.com  | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library  http://libvirt.org/ -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list