[Libvir] [PATCH] add scheduler API(take 3?)

[Richard W.M. Jones]

+        unsigned long long int ul;

Is "long long" part of ANSI C?  I thought it was a gcc extension.  I 
think you should use <stdint.h> to give you integers of fixed sizes, 
which seems to be what you want.

+/**
+ * virDomainGetSchedulerType:
+ * @domain: pointer to domain object
+ * @nparams: number of scheduler parameters(return value)
+ *
+ * Get the scheduler type.
+ *
+ * Returns NULL in case of error.
+ */

It's good that virDomainGetSchedulerType now returns an allocated 
string, but you need to add to the documentation to tell callers that 
they must free up the string.

+        switch (op.u.getschedulerid.sched_id){
+        case XEN_SCHEDULER_SEDF:
+            schedulertype = strdup("sedf");
+            *nparams = 6;
+            break;
+        case XEN_SCHEDULER_CREDIT:
+            schedulertype = strdup("credit");
+            *nparams = 2;
+            break;
+        default:
+            break;
+        }
+    }
+
+    return(schedulertype);

What happens if strdup fails?

+    if (hypervisor_version > 1) {
+        xen_op_v2_sys op_sys;
+        xen_op_v2_dom op_dom;
+        char *str_weight =strdup("weight");
+        char *str_cap    =strdup("cap");
[...]
+            strncpy(params[0].field,str_weight,strlen(str_weight));

In this code, it wasn't really clear to me why you are 'strdup'-ing 
these fields.  It seems like a memory leak?

+        case XEN_SCHEDULER_SEDF:
+            /* TODO: Implement for Xen/SEDF */
+            break;

This should return an error (or be implemented!)

+            for(i = 0;i < *nparams; i++ ){
+                if(!strncmp(params[i].field,str_weight,strlen(str_weight)))
+                     op_dom.u.getschedinfo.u.credit.weight =
+                         params[i].value.ui;
+                if(!strncmp(params[i].field,str_cap,strlen(str_cap)))
+                     op_dom.u.getschedinfo.u.credit.cap    =
+                         params[i].value.ui;
+            }

It's a good idea to check that the .type field is set to 
VIR_DOMAIN_SCHED_FIELD_UINT.

+static char *
+xenUnifiedDomainGetSchedulerType (virDomainPtr dom, int *nparams)
+{
+    int i;
+    char *schedulertype = NULL;
+    for (i = 0; i < XEN_UNIFIED_NR_DRIVERS; i++) {
+        if (drivers[i]->domainGetSchedulerType) {
+            schedulertype = drivers[i]->domainGetSchedulerType (dom, 
nparams);
+        if (schedulertype != NULL)
+            return(schedulertype);
+        }
+    }
+    return(schedulertype);
+}

There's a couple of things wrong with this (and the other functions in 
xen_unified.c):

(1) You need to check priv->opened[i].  See how the other calls are done.

(2) Since only the xen_internal driver implements the calls, you might 
consider just calling that driver directly instead of having a loop (but 
still check priv->opened[hypervisor_offset]).

I'll let others comment on the more general aspects of this patch.

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3237 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20070522/60a8fb62/attachment-0001.bin>