+ 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

Philippe Berthault wrote: OK, thanks. I still wonder if Atsushi wants the kind of fixed-size types defined in <stdint.h>. 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

On Tue, May 22, 2007 at 11:32:44AM +0100, Richard W.M. Jones wrote: FWIW, we already use 'unsigned long long' in the cpuTime field of the virDomainInfo struct. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|

Hi, Rich Thank you for your comments. I comment with inline. "Richard W.M. Jones" <rjones(a)redhat.com&gt; wrote: I add a help message. If failed just return the NULL string. It means "Scheduler: Unknown" is shown in virsh schedinfo. I change like char str_cap[] = "cap"; But I guess even keeps this config memory leak does not occured. I add macro defined "TODO" for each point. I add this check. I add it. This function also can be defined in xend_internal.c. So I keep loop on xen_unified.c. Thanks Atsushi SAKAI