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]).