Hi, Rich Thank you for detailed code review. I comment inline
"Richard W.M. Jones" <[EMAIL PROTECTED]> 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
add_scheduler9.patch
Description: Binary data
-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
