Philippe Berthault wrote:

Richard W.M. Jones a écrit :

...

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.

long long integer type is part of ISO C99 and gcc is conform to C99 norm.

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:

+ 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.

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@redhat.com> wrote:

+ 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.

I add a help message.

+ 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 failed just return the NULL string. It means "Scheduler: Unknown" is shown in virsh schedinfo.

+ 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?

I change like char str_cap[] = "cap"; But I guess even keeps this config memory leak does not occured.

+ case XEN_SCHEDULER_SEDF: + /* TODO: Implement for Xen/SEDF */ + break;

This should return an error (or be implemented!)

I add macro defined "TODO" for each point.

+ 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.

I add this check.

+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.

I add it.

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

This function also can be defined in xend_internal.c. So I keep loop on xen_unified.c. Thanks Atsushi SAKAI