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

[Nikunj A. Dadhania]

On Tue, 12 Oct 2010 18:32:19 +0200, Daniel Veillard <veillard at redhat.com> wrote:
> 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>
[...]
> > +    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.
> 
Let me look at this and I will provide a patch.

> 
> > +    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 !
>
One clarification:
Will it return error back immediately if an error occurs?
Or will it try setting all of them one by one and if anyone of them succeed,
success is returned.

> > +cleanup:
> > +    if (cgroup)
> > +        virCgroupFree(&cgroup);
> > +    if (vm)
> > +        virDomainObjUnlock(vm);
> > +    lxcDriverUnlock(driver);
> > +    return ret;
> > +}
> > +
> 
>  After those modifications, ACK, applied to my tree,
> 
Thanks

Nikunj