Re: [libvirt] [PATCH 6/8] Implement schedular tunables API using cgroups

On Thu, Jul 23, 2009 at 05:38:45PM +0200, Daniel Veillard wrote: > On Wed, Jul 22, 2009 at 04:23:45PM +0100, Daniel P. Berrange wrote: > > * src/qemu_driver.c: Add driver methods qemuGetSchedulerType, > > qemuGetSchedulerParameters, qemuSetSchedulerParameters > > * src/lxc_driver.c: Fix to use unsigned long long consistently > > for schedular parameters > > * src/cgroup.h, src/cgroup.c: Fix cpu_shares to take unsigned > > long long > > * src/util.c, src/util.h, src/libvirt_private.syms: Add a > > virStrToDouble helper > > * src/virsh.c: Fix handling of --set arg to schedinfo command > > to honour the designated data type of each schedular tunable > > as declared by the driver > > > > Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; > > --- > > src/cgroup.c | 10 +- > > src/cgroup.h | 4 +- > > src/libvirt_private.syms | 1 + > > src/lxc_driver.c | 9 ++- > > src/qemu_driver.c | 151 +++++++++++++++++++++++++++++- > > src/util.c | 20 ++++ > > src/util.h | 3 + > [...] > > @@ -1355,9 +1355,14 @@ static int lxcSetSchedulerParameters(virDomainPtr domain, > > > > for (i = 0; i < nparams; i++) { > > virSchedParameterPtr param = &params[i]; > > + if (param->type != VIR_DOMAIN_SCHED_FIELD_ULLONG) { > > + lxcError(NULL, domain, VIR_ERR_INVALID_ARG, > > + _("invalid type for cpu_shares tunable, expected a 'ullong'")); > > + goto cleanup; > > + } > > Humpf, we are actually breaking the API in some ways there, what is > the argument ? Consistency with qemu scheduling parameters ? Yes & no. It was essentially broken already. - The lxcGetSchedulerParameters method returned cpu_shares with type ULLONG, and filled the '.ul' field. - The lxcSetSchedulerParameters method did not check the type at all, and blindly read the '.ui' field instead. The language bindings need to know the data types for each parameter in order to covert from the languages' type to the parameter type. The Perl and Python both do this by calling virGetSchedulerParameters to fetch current values, and then updating the values in place, and then calling virSetSchedulerParameters to update them. So the fact that the LXC driver 'get' method returned ULLONG, means it should be also accepting ULLONG in the set method, otherwise it is impossible for the language bindings to work. A similar situation exists for virsh with its --set parameter, which can only work by calling 'get' to discover the existing type and then updating it. So this code is just validating what apps should have already been doing, and fixing the mistaken reading of .ui to be .ul