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