Re: [Libvir] [PATCH] add scheduler API(take 3?)
Hi, Rich
Thank you for detailed code review.
I comment inline
"Richard W.M. Jones" <rjones(a)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
Attachments:
- add_scheduler9.patch (application/octet-stream — 29.5 KB)