Hi, Rich
Thank you for your comments.
I comment with inline.
"Richard W.M. Jones" <rjones@xxxxxxxxxx> 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
Attachment:
add_scheduler8.patch
Description: Binary data