Re: [libvirt] PATCH: 2/3: Implement schedular params for QEMU

[Daniel Veillard <veillard redhat com>]

On Thu, Feb 26, 2009 at 04:40:18PM +0000, Daniel P. Berrange wrote:
> This patch implements the schedular parameter APIs. This adds a
> single tunable 'cpu_shares' that is provided by cgroups. This is
> a slightly more fancy way of doing nice priorities, giving a way
> to tune relative priority of  VMs

  The scheduler API is one of the parts I'm most afraid of from an
API perspective...

[...]
> +static int qemuSetSchedulerParameters(virDomainPtr domain,
> +                                     virSchedParameterPtr params,
> +                                     int nparams)
[...]
> +    for (i = 0; i < nparams; i++) {
> +        virSchedParameterPtr param = &params[i];
> +
> +        if (STREQ(param->field, "cpu_shares")) {

  I think we should also check param->type to be of type 
VIR_DOMAIN_SCHED_FIELD_ULLONG before passing  value.ui
> +            if (virCgroupSetCpuShares(group, params[i].value.ui) != 0)
  maybe use param->value.ui since we have a pointer handy, avoid
index trouble if the code evolves.

> +                goto cleanup;
> +        } else {
> +            qemudReportError(domain->conn, domain, NULL, VIR_ERR_INVALID_ARG,
> +                             _("Invalid parameter `%s'"), param->field);
> +            goto cleanup;
> +        }
> +    }
> +    ret = 0;

  Patch looks fine otherwise,

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/