[libvirt] [PATCH v4 06/13] Implement driver interface domainSetMemoryParamters for QEmu

[Daniel Veillard]

On Fri, Oct 08, 2010 at 05:45:44PM +0530, Nikunj A. Dadhania wrote:
> From: Nikunj A. Dadhania <nikunj at linux.vnet.ibm.com>
> 
> Driver interface for setting memory hard_limit, soft_limit and swap
> hard_limit.
[...]
> +static int qemuDomainSetMemoryParameters(virDomainPtr dom,
> +                                         virMemoryParameterPtr params,
> +                                         int nparams, 
> +                                         unsigned int flags ATTRIBUTE_UNUSED)
> +{
> +    struct qemud_driver *driver = dom->conn->privateData;
> +    int i;
> +    virCgroupPtr group = NULL;
> +    virDomainObjPtr vm = NULL;
> +    int ret = -1;
> +
> +    qemuDriverLock(driver);
> +    if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_MEMORY)) {
> +        qemuReportError(VIR_ERR_NO_SUPPORT,
> +                        __FUNCTION__);
> +        goto cleanup;
> +    }
> +
> +    vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> +
> +    if (vm == NULL) {
> +        qemuReportError(VIR_ERR_INTERNAL_ERROR,
> +                        _("No such domain %s"), dom->uuid);
> +        goto cleanup;
> +    }
> +
> +    if (virCgroupForDomain(driver->cgroup, vm->def->name, &group, 0) != 0) {
> +        qemuReportError(VIR_ERR_INTERNAL_ERROR,
> +                        _("cannot find cgroup for domain %s"), vm->def->name);
> +        goto cleanup;
> +    }
> +
> +    for (i = 0; i < nparams; i++) {
> +        virMemoryParameterPtr param = &params[i];

  Bingo if you passed a NULL pointer at the library entry point this
would just crash. Well you can still get this to crash with a value of
nparams bigger than the array. It's one of my main concern with this
API, it's a bit easy for the user to get things wrong, at least we
should provide minimal checkings.
  I still prefer to keep those checks in the top function in libvirt.c
to avoid duplicating in all drivers, but here we could check that
nparams is between 1 and 3 c.f. the comment on next patch:

+        /* Current number of memory parameters supported by cgroups is
3
+         * FIXME: Magic number, need to see where should this go
+         */
+        *nparams = 3;

  that 3 need to be abstracted somehow as

#define QEMU_NB_MEM_PARAM  3

  which I'm adding at the beginning of the file.
However if we check that nparams > 3, we will introduce an
incompatibility, for example if we try to set tunables from a newver
library version but talking to an older server not implementing the
new ones, we return an error, but ...

[...]
> +        } else if (STREQ(param->field, VIR_DOMAIN_MEMORY_MIN_GUARANTEE)) {
> +            qemuReportError(VIR_ERR_INVALID_ARG,
> +                            _("Memory tunable `%s' not implemented"), param->field);
> +        } else {
> +            qemuReportError(VIR_ERR_INVALID_ARG,
> +                            _("Parameter `%s' not supported"), param->field);
> +        }

  right now the code just ignores if you failed to set a tunable.
I think this is a problem, we should at least return an error if a
tunable could not be set. Right now the error would be raised within
libvirt daemon but since there is no error code the application may
not get the problem. So in those 2 cases I suggest to set an error.
So I'm changing this to set ret to 0 before the loop and setting
ret = -1; in any of the error cases found within.

> +    }
> +    ret = 0;

  moved in front of loop.
Quite a bit of change but once done I commited it 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/