[libvirt] [PATCH v4 10/13] Implement driver interface domainSetMemoryParamters for LXC

[Daniel Veillard]

On Fri, Oct 08, 2010 at 05:46:28PM +0530, Nikunj A. Dadhania wrote:
> From: Nikunj A. Dadhania <nikunj at linux.vnet.ibm.com>
> 
> Add support in the lxc driver for various memory controllable parameters
> 
> v4:
> + prototype change: add unsigned int flags
> 
> v2:
> + Use #define string constants for "hard_limit", etc
> + fix typo: min_guarantee
> 
> Acked-by: "Daniel P. Berrange" <berrange at redhat.com>
> Signed-off-by: Nikunj A. Dadhania <nikunj at linux.vnet.ibm.com>
[...]
> +static int lxcDomainSetMemoryParameters(virDomainPtr dom,
> +                                        virMemoryParameterPtr params,
> +                                        int nparams, 
> +                                        unsigned int flags ATTRIBUTE_UNUSED)
> +{
> +    lxc_driver_t *driver = dom->conn->privateData;
> +    int i;
> +    virCgroupPtr cgroup = NULL;
> +    virDomainObjPtr vm = NULL;
> +    int ret = -1;
> +
> +    lxcDriverLock(driver);
> +    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> +
> +    if (vm == NULL) {
> +        char uuidstr[VIR_UUID_STRING_BUFLEN];
> +        virUUIDFormat(dom->uuid, uuidstr);
> +        lxcError(VIR_ERR_NO_DOMAIN,
> +                 _("No domain with matching uuid '%s'"), uuidstr);
> +        goto cleanup;
> +    }

  Hum, the qemu driver was reporting

    if (vm == NULL) {
        qemuReportError(VIR_ERR_INTERNAL_ERROR,
        _("No such domain %s"), dom->uuid);
        goto cleanup;
    }

the 2 should be harmonized I guess, but since the LXC reporting is better
I left this as a TODO, seems that's a more general cleanup needed between
drivers.

> +    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;
> +    }

And QEmu reported here:

        qemuReportError(VIR_ERR_INTERNAL_ERROR,
                        _("cannot find cgroup for domain %s"), vm->def->name);
        goto cleanup;

why diverging strings ? ... I used the same string instead !

> +    for (i = 0; i < nparams; i++) {
> +        virMemoryParameterPtr param = &params[i];
> +
> +        if (STREQ(param->field, VIR_DOMAIN_MEMORY_HARD_LIMIT)) {
> +            int rc;
> +            if (param->type != VIR_DOMAIN_MEMORY_FIELD_ULLONG) {
> +                lxcError(VIR_ERR_INVALID_ARG, "%s",
> +                         _("invalid type for memory hard_limit tunable, expected a 'ullong'"));
> +                continue;
> +            }
> +
> +            rc = virCgroupSetMemoryHardLimit(cgroup, params[i].value.ul);
> +            if (rc != 0) {
> +                virReportSystemError(-rc, "%s",
> +                                     _("unable to set memory hard_limit tunable"));
> +            }
> +        } else if (STREQ(param->field, VIR_DOMAIN_MEMORY_SOFT_LIMIT)) {
> +            int rc;
> +            if (param->type != VIR_DOMAIN_MEMORY_FIELD_ULLONG) {
> +                lxcError(VIR_ERR_INVALID_ARG, "%s",
> +                         _("invalid type for memory soft_limit tunable, expected a 'ullong'"));
> +                continue;
> +            }
> +
> +            rc = virCgroupSetMemorySoftLimit(cgroup, params[i].value.ul);
> +            if (rc != 0) {
> +                virReportSystemError(-rc, "%s",
> +                                     _("unable to set memory soft_limit tunable"));
> +            }
> +        } else if (STREQ(param->field, VIR_DOMAIN_SWAP_HARD_LIMIT)) {
> +            int rc;
> +            if (param->type != VIR_DOMAIN_MEMORY_FIELD_ULLONG) {
> +                lxcError(VIR_ERR_INVALID_ARG, "%s",
> +                         _("invalid type for swap_hard_limit tunable, expected a 'ullong'"));
> +                continue;
> +            }
> +
> +            rc = virCgroupSetSwapHardLimit(cgroup, params[i].value.ul);
> +            if (rc != 0) {
> +                virReportSystemError(-rc, "%s",
> +                                     _("unable to set swap_hard_limit tunable"));
> +            }
> +        } else if (STREQ(param->field, VIR_DOMAIN_MEMORY_MIN_GUARANTEE)) {
> +            lxcError(VIR_ERR_INVALID_ARG,
> +                     _("Memory tunable `%s' not implemented"), param->field);
> +        } else {
> +            lxcError(VIR_ERR_INVALID_ARG,
> +                     _("Parameter `%s' not supported"), param->field);
> +        }
> +    }
> +    ret = 0;

  Same problem of error reporting as in the QEmu driver, I moved ret = 0;
before the loop and et ret = -1; on all errors !

> +cleanup:
> +    if (cgroup)
> +        virCgroupFree(&cgroup);
> +    if (vm)
> +        virDomainObjUnlock(vm);
> +    lxcDriverUnlock(driver);
> +    return ret;
> +}
> +

 After those modifications, ACK, applied to my tree,

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/