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

[Atsushi SAKAI]

Hi, Rich

Thank you for your comments.
I comment with inline.


"Richard W.M. Jones" <rjones at 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: add_scheduler8.patch
Type: application/octet-stream
Size: 30173 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20070524/56de44ac/attachment-0001.obj>