Hi, Rich Thank you for detailed code review. I comment inline "Richard W.M. Jones" <rjones@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