Re: [Libvir] [PATCH] add scheduler API(take 3?)
+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) +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. + /* 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? + 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. + /* + * 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? Rich.
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
This patch is based on today's libvirt. Changes are two points. 1)Sunou's memory leak fixes (I removed free(ipt) from this patch) 2)Valgrind detected bug fixes in params[i].type & params[i].fields src/virsh.c) line 467 of this patch. + for (i = 0; i < nparams; i++){ + params[i].type = VIR_DOMAIN_SCHED_FIELD_INIT; + strncpy(params[i].field," ",15); + } error message detected by valgrind. ==5227== Conditional jump or move depends on uninitialised value(s) ==5227== at 0x4A066D8: strlen (mc_replace_strmem.c:246) ==5227== by 0x30E0445B87: vfprintf (in /lib64/libc-2.5.so) ==5227== by 0x30E04E04D0: __printf_chk (in /lib64/libc-2.5.so) ==5227== by 0x408EE5: cmdSchedinfo (virsh.c:1035) ==5227== by 0x408445: vshCommandRun (virsh.c:3056) ==5227== by 0x408C29: main (virsh.c:3801) Thanks Atsushi SAKAI
Atsushi SAKAI wrote:
This patch is based on today's libvirt. Changes are two points. 1)Sunou's memory leak fixes (I removed free(ipt) from this patch) 2)Valgrind detected bug fixes in params[i].type & params[i].fields src/virsh.c)
line 467 of this patch. + for (i = 0; i < nparams; i++){ + params[i].type = VIR_DOMAIN_SCHED_FIELD_INIT; + strncpy(params[i].field," ",15); + }
Just some very minor points: /** + * virDomainSchedParameterType: + * + * A scheduler parameter field type + */ +typedef enum { + VIR_DOMAIN_SCHED_FIELD_INT = 0, /* integer case */ + VIR_DOMAIN_SCHED_FIELD_UINT = 1, /* unsigned integer case */ + VIR_DOMAIN_SCHED_FIELD_LLONG = 2, /* long long case */ + VIR_DOMAIN_SCHED_FIELD_ULLONG = 3, /* unsigned long long case */ + VIR_DOMAIN_SCHED_FIELD_DOUBLE = 4, /* double case */ + VIR_DOMAIN_SCHED_FIELD_BOOLEAN = 5, /* boolean(character) case */ + VIR_DOMAIN_SCHED_FIELD_INIT = 6 /* for valgrind check */ +} virSchedParameterType; I would renumber these from 1 and omit the _INIT field altogether, then ... + int type; /* Format type should use enum from virSchedParameterType */ ... here turn this into 'virSchedParameterType type' (that's what it is, after all), and ... + for (i = 0; i < nparams; i++){ + params[i].type = VIR_DOMAIN_SCHED_FIELD_INIT; + strncpy(params[i].field," ",15); + } ... here change this to: for (i = 0; i < nparams; ++i) { params[i].type = 0; memset (params[i].field, 0, sizeof params[i].field); } I'm going to apply this patch to my private copy & try and break it further, so stay tuned ... Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
Atsushi: The attached patch fixes some things in your add_scheduler10.patch (you need to apply this patch on top of your patch): * Lots of error checking added. If the lowest level function detects an error, it *must* report a useful message. * Make libvirt.h match libvirt.h.in. If you edit just libvirt.h then any changes you make will be lost next time someone runs ./configure. * Removed VIR_DOMAIN_SCHED_FIELD_INIT. This field is now initialised with zero. * Changed the type of that field to the more logical virSchedParameterType type. * virDomainSetSchedulerParameters, nparams does not really need to be a pointer (can it ever be changed?) * In virsh, only call virDomainSetSchedulerParameters if the user has requested a parameter to change. * In virDomainGetSchedulerType, allow nparams (pointer) to be NULL - eg. in case the caller doesn't care about the number of parameters. Note: My Xen machine is completely broken at the moment so I haven't been able to test this, just compile it. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
Hi, Rich Thank you very much for your patch. It works fine. And Your patch is very helpful to understand the Error handling policy on libvirt. It is improves my knowledge. Thanks Atsushi SAKAI "Richard W.M. Jones" <rjones@redhat.com> wrote:
Atsushi:
The attached patch fixes some things in your add_scheduler10.patch (you need to apply this patch on top of your patch):
* Lots of error checking added. If the lowest level function detects an error, it *must* report a useful message.
* Make libvirt.h match libvirt.h.in. If you edit just libvirt.h then any changes you make will be lost next time someone runs ./configure.
* Removed VIR_DOMAIN_SCHED_FIELD_INIT. This field is now initialised with zero.
* Changed the type of that field to the more logical virSchedParameterType type.
* virDomainSetSchedulerParameters, nparams does not really need to be a pointer (can it ever be changed?)
* In virsh, only call virDomainSetSchedulerParameters if the user has requested a parameter to change.
* In virDomainGetSchedulerType, allow nparams (pointer) to be NULL - eg. in case the caller doesn't care about the number of parameters.
Note: My Xen machine is completely broken at the moment so I haven't been able to test this, just compile it.
Rich.
-- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Wed, May 30, 2007 at 04:40:39PM +0100, Richard W.M. Jones wrote:
* Changed the type of that field to the more logical virSchedParameterType type.
Actually there is one thing I would revert from your patch it's the change from int to virSchedParameterType in structs and parameters. Basically the problem comes from the fact that C does not define the size of an enum, so some compiler may use 8 bits to encode it, others may use 32bits, or worse say you add a new error value -1 it could change from 8 to 32 bits breaking ABI compatibility. I really prefer to use int for the virSchedParameterType in public structs and public functions parameters for this reason, it's far from ideal, but I prefer this than having weird ABI incompatibilities due to compiler variations :-) otherwise thanks a lot for the cleanup ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Daniel Veillard wrote:
On Wed, May 30, 2007 at 04:40:39PM +0100, Richard W.M. Jones wrote:
* Changed the type of that field to the more logical virSchedParameterType type.
Actually there is one thing I would revert from your patch it's the change from int to virSchedParameterType in structs and parameters. Basically the problem comes from the fact that C does not define the size of an enum, so some compiler may use 8 bits to encode it, others may use 32bits, or worse say you add a new error value -1 it could change from 8 to 32 bits breaking ABI compatibility. I really prefer to use int for the virSchedParameterType in public structs and public functions parameters for this reason, it's far from ideal, but I prefer this than having weird ABI incompatibilities due to compiler variations :-)
Fair enough ... I didn't know that about enums actually. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
participants (3)
-
Atsushi SAKAI -
Daniel Veillard -
Richard W.M. Jones