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

[Atsushi SAKAI]

Hi, Rich

Thank you for detailed code review.
I comment inline



"Richard W.M. Jones" <rjones at redhat.com> wrote:

> +char *
> +virDomainGetSchedulerType(virDomainPtr domain, int *nparams)
> +{
> +    virConnectPtr conn;
> +    char *schedtype;
> +
> +    if (domain == NULL) {
> +        TODO
> +        return NULL;
> +    }
> 
> Is this case an error, or were you thinking of adding more semantics 
> later on here?
> 
> (and the same comment for virDomainGet/SetSchedulerParameters)
> 
NO.
I just based on other libvirt functions.


> +static int
> +cmdSchedinfo(vshControl * ctl, vshCmd * cmd)
> +{
> [...]
> +    char *str_weight = strdup("weight");
> +    char *str_cap    = strdup("cap");
> +
> +    nparams = malloc(sizeof(int));
> +    *nparams = 0;
> +
> +    if (!vshConnectionUsability(ctl, ctl->conn, TRUE))
> +        return FALSE;
> 
> There's still a problem memory leak here.
> 
Thanks!
I fixed it.
and I add free(ipt) by valgrind check.

> 
> +    /* Currently supports Xen Credit only */
> +    weight = vshCommandOptInt(cmd, "weight", &weightfound);
> +    if(weightfound){ inputparam++; }
> +
> +    cap    = vshCommandOptInt(cmd, "cap", &capfound);
> +    if(capfound){ inputparam++; }
> 
> and can this be made so it isn't Xen-specific?
>
Yes, this is xen specific(in other words scheduler specific).
Currently developer should add each scheduler options to this command.
Is there any good way to solve this?
I think this implementation is necessary.

 
> +    if ((domain == NULL) || (domain->conn == NULL))
> +        return -1;
> +
> +    priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
> +    if (priv->handle < 0 || domain->id < 0)
> +        return -1;
> 
> At the lowest level, these functions should return error messages back 
> to the upper layers.  Otherwise users have nothing to diagnose errors with.

I also based on other xen_internal.c code.
I think it is enough at this moment.
In future, your suggested error messages should be included.

> 
> +    /*
> +     * Support only dom_interface_version >=5
> +     * (Xen3.1.0 or later)
> +     */
> +    if (dom_interface_version < 5)
> +        return -1;
> 
> Is this because earlier hypervisors didn't support this, or is it just 
> not implemented?

As I already commented in this thread, domctl works funny at xen3.0.4.
So I skip that version.

Thanks
Atsushi SAKAI

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