Re: [Libvir] [PATCH] add scheduler API(take 3?)

+ unsigned long long int ul; Is "long long" part of ANSI C? I thought it was a gcc extension. I think you should use <stdint.h> to give you integers of fixed sizes, which seems to be what you want. +/** + * virDomainGetSchedulerType: + * @domain: pointer to domain object + * @nparams: number of scheduler parameters(return value) + * + * Get the scheduler type. + * + * Returns NULL in case of error. + */ It's good that virDomainGetSchedulerType now returns an allocated string, but you need to add to the documentation to tell callers that they must free up the string. + switch (op.u.getschedulerid.sched_id){ + case XEN_SCHEDULER_SEDF: + schedulertype = strdup("sedf"); + *nparams = 6; + break; + case XEN_SCHEDULER_CREDIT: + schedulertype = strdup("credit"); + *nparams = 2; + break; + default: + break; + } + } + + return(schedulertype); What happens if strdup fails? + if (hypervisor_version > 1) { + xen_op_v2_sys op_sys; + xen_op_v2_dom op_dom; + char *str_weight =strdup("weight"); + char *str_cap =strdup("cap"); [...] + strncpy(params[0].field,str_weight,strlen(str_weight)); In this code, it wasn't really clear to me why you are 'strdup'-ing these fields. It seems like a memory leak? + case XEN_SCHEDULER_SEDF: + /* TODO: Implement for Xen/SEDF */ + break; This should return an error (or be implemented!) + for(i = 0;i < *nparams; i++ ){ + if(!strncmp(params[i].field,str_weight,strlen(str_weight))) + op_dom.u.getschedinfo.u.credit.weight = + params[i].value.ui; + if(!strncmp(params[i].field,str_cap,strlen(str_cap))) + op_dom.u.getschedinfo.u.credit.cap = + params[i].value.ui; + } It's a good idea to check that the .type field is set to VIR_DOMAIN_SCHED_FIELD_UINT. +static char * +xenUnifiedDomainGetSchedulerType (virDomainPtr dom, int *nparams) +{ + int i; + char *schedulertype = NULL; + for (i = 0; i < XEN_UNIFIED_NR_DRIVERS; i++) { + if (drivers[i]->domainGetSchedulerType) { + schedulertype = drivers[i]->domainGetSchedulerType (dom, nparams); + if (schedulertype != NULL) + return(schedulertype); + } + } + return(schedulertype); +} There's a couple of things wrong with this (and the other functions in xen_unified.c): (1) You need to check priv->opened[i]. See how the other calls are done. (2) Since only the xen_internal driver implements the calls, you might consider just calling that driver directly instead of having a loop (but still check priv->opened[hypervisor_offset]). I'll let others comment on the more general aspects of this patch. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903