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

[Ryota Ozaki <ozaki ryota gmail com>]

On Mon, May 3, 2010 at 10:48 PM, Daniel Veillard <veillard redhat com> wrote:
> On Mon, May 03, 2010 at 02:20:27PM +0200, jdenemar redhat com wrote:
>> From: Jiri Denemark <jdenemar redhat com>
>>
>> ---
>>  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

I think it's OK because the behavior is same as qemu driver
and does not bother me, a user of lxc ;-)

Thanks,
  ozaki-r

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