[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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
diff -ur --exclude=CVS libvirt-addscheduler10/include/libvirt/libvirt.h libvirt-addscheduler10rich/include/libvirt/libvirt.h
--- libvirt-addscheduler10/include/libvirt/libvirt.h	2007-05-30 16:00:27.000000000 +0100
+++ libvirt-addscheduler10rich/include/libvirt/libvirt.h	2007-05-30 16:10:17.000000000 +0100
@@ -174,13 +174,12 @@
  * 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 */
+    VIR_DOMAIN_SCHED_FIELD_INT     = 1,	/* integer case */
+    VIR_DOMAIN_SCHED_FIELD_UINT    = 2,	/* unsigned integer case */
+    VIR_DOMAIN_SCHED_FIELD_LLONG   = 3,	/* long long case */
+    VIR_DOMAIN_SCHED_FIELD_ULLONG  = 4,	/* unsigned long long case */
+    VIR_DOMAIN_SCHED_FIELD_DOUBLE  = 5,	/* double case */
+    VIR_DOMAIN_SCHED_FIELD_BOOLEAN = 6	/* boolean(character) case */
 } virSchedParameterType;
 
 /**
@@ -201,7 +200,7 @@
 
 struct _virSchedParameter {
     char field[VIR_DOMAIN_SCHED_FIELD_LENGTH];	/* parameter name */
-    int type;	/* Format type should use enum from virSchedParameterType */
+    virSchedParameterType type;	/* parameter type */
     union {
         int i;				/* data for integer case */
         unsigned int ui;	/* data for unsigned integer case */
@@ -230,7 +229,7 @@
  * Change scheduler parameters
  */
 int			virDomainSetSchedulerParameters	(virDomainPtr domain,
-						virSchedParameterPtr params, int *nparams);
+						virSchedParameterPtr params, int nparams);
 
 /**
  * VIR_NODEINFO_MAXCPUS:
diff -ur --exclude=CVS libvirt-addscheduler10/include/libvirt/libvirt.h.in libvirt-addscheduler10rich/include/libvirt/libvirt.h.in
--- libvirt-addscheduler10/include/libvirt/libvirt.h.in	2007-05-30 16:00:27.000000000 +0100
+++ libvirt-addscheduler10rich/include/libvirt/libvirt.h.in	2007-05-30 16:08:59.000000000 +0100
@@ -174,12 +174,12 @@
  * 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_INT     = 1,	/* integer case */
+    VIR_DOMAIN_SCHED_FIELD_UINT    = 2,	/* unsigned integer case */
+    VIR_DOMAIN_SCHED_FIELD_LLONG   = 3,	/* long long case */
+    VIR_DOMAIN_SCHED_FIELD_ULLONG  = 4,	/* unsigned long long case */
+    VIR_DOMAIN_SCHED_FIELD_DOUBLE  = 5,	/* double case */
+    VIR_DOMAIN_SCHED_FIELD_BOOLEAN = 6	/* boolean(character) case */
 } virSchedParameterType;
 
 /**
@@ -200,7 +200,7 @@
 
 struct _virSchedParameter {
     char field[VIR_DOMAIN_SCHED_FIELD_LENGTH];	/* parameter name */
-    int type;	/* Format type should use enum from virSchedParameterType */
+    virSchedParameterType type;	/* parameter type */
     union {
         int i;				/* data for integer case */
         unsigned int ui;	/* data for unsigned integer case */
@@ -229,7 +229,7 @@
  * Change scheduler parameters
  */
 int			virDomainSetSchedulerParameters	(virDomainPtr domain,
-						virSchedParameterPtr params, int *nparams);
+						virSchedParameterPtr params, int nparams);
 
 /**
  * VIR_NODEINFO_MAXCPUS:
diff -ur --exclude=CVS libvirt-addscheduler10/src/driver.h libvirt-addscheduler10rich/src/driver.h
--- libvirt-addscheduler10/src/driver.h	2007-05-30 16:00:27.000000000 +0100
+++ libvirt-addscheduler10rich/src/driver.h	2007-05-30 16:09:22.000000000 +0100
@@ -167,7 +167,7 @@
 
 typedef int
 	(*virDrvDomainSetSchedulerParameters)	(virDomainPtr domain,
-					virSchedParameterPtr params, int *nparams);
+					virSchedParameterPtr params, int nparams);
 
 typedef struct _virDriver virDriver;
 typedef virDriver *virDriverPtr;
diff -ur --exclude=CVS libvirt-addscheduler10/src/libvirt.c libvirt-addscheduler10rich/src/libvirt.c
--- libvirt-addscheduler10/src/libvirt.c	2007-05-30 16:00:27.000000000 +0100
+++ libvirt-addscheduler10rich/src/libvirt.c	2007-05-30 16:11:34.000000000 +0100
@@ -1463,6 +1463,7 @@
         return schedtype;
     }
 
+    virLibConnError (conn, VIR_ERR_CALL_FAILED, __FUNCTION__);
     return NULL;
 }
 
@@ -1478,7 +1479,7 @@
  *
  * Get the scheduler parameters
  *
- * Returns NULL in case of error.
+ * Returns -1 in case of error.
  */
 int
 virDomainGetSchedulerParameters(virDomainPtr domain,
@@ -1504,6 +1505,7 @@
     if (conn->driver->domainGetSchedulerParameters)
         return conn->driver->domainGetSchedulerParameters (domain, params, nparams);
 
+    virLibConnError (conn, VIR_ERR_CALL_FAILED, __FUNCTION__);
     return -1;
 }
 
@@ -1511,17 +1513,17 @@
  * virDomainSetSchedulerParameters:
  * @domain: pointer to domain object
  * @params: pointer to scheduler parameter object
- * @nparams: pointer to number of scheduler parameter
+ * @nparams: number of scheduler parameter
  *          (this value should be same or less as 
  *           the nparams of virDomainGetSchedulerType)
  *
  * Change the scheduler parameters
  *
- * Returns NULL in case of error.
+ * Returns -1 in case of error.
  */
 int
 virDomainSetSchedulerParameters(virDomainPtr domain, 
-				virSchedParameterPtr params, int *nparams)
+				virSchedParameterPtr params, int nparams)
 {
     virConnectPtr conn;
 
@@ -1543,6 +1545,7 @@
     if (conn->driver->domainSetSchedulerParameters)
         return conn->driver->domainSetSchedulerParameters (domain, params, nparams);
 
+    virLibConnError (conn, VIR_ERR_CALL_FAILED, __FUNCTION__);
     return -1;
 }
 
diff -ur --exclude=CVS libvirt-addscheduler10/src/virsh.c libvirt-addscheduler10rich/src/virsh.c
--- libvirt-addscheduler10/src/virsh.c	2007-05-30 16:00:27.000000000 +0100
+++ libvirt-addscheduler10rich/src/virsh.c	2007-05-30 16:08:22.000000000 +0100
@@ -29,6 +29,7 @@
 #include <ctype.h>
 #include <fcntl.h>
 #include <locale.h>
+#include <assert.h>
 
 #include <libxml/parser.h>
 #include <libxml/tree.h>
@@ -964,7 +965,8 @@
     virSchedParameterPtr params;
     int i, ret;
     int nparams = 0;
-    int inputparam = 0;
+    int nr_inputparams = 0;
+    int inputparams = 0;
     int weightfound = 0;
     int weight;
     int capfound = 0;
@@ -980,30 +982,39 @@
 
     /* Currently supports Xen Credit only */
     weight = vshCommandOptInt(cmd, "weight", &weightfound);
-    if(weightfound){ inputparam++; }
+    if (weightfound) nr_inputparams++;
             
     cap    = vshCommandOptInt(cmd, "cap", &capfound);
-    if(capfound){ inputparam++; }    
+    if (capfound) nr_inputparams++;
 
-    params = vshMalloc(ctl, sizeof(virSchedParameter)* inputparam);
+    params = vshMalloc(ctl, sizeof (virSchedParameter) * nr_inputparams);
+    if (params == NULL) return FALSE;
 
-    inputparam=0;
     if (weightfound) {
-         strncpy(params[inputparam].field,str_weight,sizeof(str_weight));
-         params[inputparam].type = VIR_DOMAIN_SCHED_FIELD_UINT;
-         params[inputparam].value.ui = weight;
-         inputparam++; 
+         strncpy(params[inputparams].field,str_weight,sizeof(str_weight));
+         params[inputparams].type = VIR_DOMAIN_SCHED_FIELD_UINT;
+         params[inputparams].value.ui = weight;
+         inputparams++; 
     }
 
     if (capfound) {
-         strncpy(params[inputparam].field,str_cap,sizeof(str_cap));
-         params[inputparam].type = VIR_DOMAIN_SCHED_FIELD_UINT;
-         params[inputparam].value.ui = cap;
-         inputparam++;
-    }    
+         strncpy(params[inputparams].field,str_cap,sizeof(str_cap));
+         params[inputparams].type = VIR_DOMAIN_SCHED_FIELD_UINT;
+         params[inputparams].value.ui = cap;
+         inputparams++;
+    }
     /* End Currently supports Xen Credit only */
 
-    /* Get SchedulerType */
+    assert (inputparams == nr_inputparams);
+
+    /* Set SchedulerParameters */
+    if (inputparams > 0) {
+        ret = virDomainSetSchedulerParameters(dom, params, inputparams);
+        if (ret == -1) return FALSE;
+    }
+    free(params);
+
+    /* Print SchedulerType */
     schedulertype = virDomainGetSchedulerType(dom, &nparams);
     if (schedulertype!= NULL){
         vshPrint(ctl, "%-15s %s\n", _("Scheduler:"),
@@ -1011,20 +1022,17 @@
         free(schedulertype);
     } else {
         vshPrint(ctl, "%-15s %s\n", _("Scheduler:"), _("Unknown"));
-        return -1;
+        return FALSE;
     }
 
-    /* Set SchedulerParameters */
-    ret = virDomainSetSchedulerParameters(dom, params, &inputparam);
-    free(params);
-
     /* Get SchedulerParameters */
     params = vshMalloc(ctl, sizeof(virSchedParameter)* nparams);
     for (i = 0; i < nparams; i++){
-        params[i].type = VIR_DOMAIN_SCHED_FIELD_INIT;
-        strncpy(params[i].field,"              ",15);
+        params[i].type = 0;
+        memset (params[i].field, 0, sizeof params[i].field);
     }
     ret = virDomainGetSchedulerParameters(dom, params, &nparams);
+    if (ret == -1) return FALSE;
     if(nparams){
         for (i = 0; i < nparams; i++){
             switch (params[i].type) {
diff -ur --exclude=CVS libvirt-addscheduler10/src/xen_internal.c libvirt-addscheduler10rich/src/xen_internal.c
--- libvirt-addscheduler10/src/xen_internal.c	2007-05-30 16:00:27.000000000 +0100
+++ libvirt-addscheduler10rich/src/xen_internal.c	2007-05-30 16:21:03.000000000 +0100
@@ -964,19 +964,25 @@
     char *schedulertype = NULL;
     xenUnifiedPrivatePtr priv;
 
-    if ((domain == NULL) || (domain->conn == NULL))
-        return(schedulertype);
+    if ((domain == NULL) || (domain->conn == NULL)) {
+        virXenError (VIR_ERR_INTERNAL_ERROR, "xenHypervisorGetSchedulerType: domain or conn is NULL", 0);
+        return NULL;
+    }
 
     priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
-    if (priv->handle < 0 || domain->id < 0)
-        return(schedulertype);
+    if (priv->handle < 0 || domain->id < 0) {
+        virXenError (VIR_ERR_INTERNAL_ERROR, "xenHypervisorGetSchedulerType: priv->handle or domain->id invalid", 0);
+        return NULL;
+    }
 
     /*
      * Support only dom_interface_version >=5
      * (Xen3.1.0 or later)
      */
-    if (dom_interface_version < 5)
-        return(schedulertype);
+    if (dom_interface_version < 5) {
+        virXenError(VIR_ERR_NO_XEN, "xenHypervisorGetSchedulerType unsupported in dom interface < 5", 0);
+        return NULL;
+    }
 
     if (hypervisor_version > 1) {
         xen_op_v2_sys op;
@@ -989,19 +995,18 @@
         switch (op.u.getschedulerid.sched_id){
         case XEN_SCHEDULER_SEDF:
             schedulertype = strdup("sedf");
-            *nparams = 6;
+            if (nparams) *nparams = 6;
             break;
         case XEN_SCHEDULER_CREDIT:
             schedulertype = strdup("credit");
-            *nparams = 2;
+            if (nparams) *nparams = 2;
             break;
         default:
             break;
         }
     }
 
-    return(schedulertype);
-
+    return schedulertype;
 }
 
 /**
@@ -1021,32 +1026,39 @@
 xenHypervisorGetSchedulerParameters(virDomainPtr domain,
 				 virSchedParameterPtr params, int *nparams)
 {
-    int ret = -1;
     xenUnifiedPrivatePtr priv;
     char str_weight[] ="weight";
     char str_cap[]    ="cap";
 
-    if ((domain == NULL) || (domain->conn == NULL))
+    if ((domain == NULL) || (domain->conn == NULL)) {
+        virXenError (VIR_ERR_INTERNAL_ERROR, "xenHypervisorGetSchedulerParameters: domain or conn is NULL", 0);
         return -1;
+    }
 
     priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
-    if (priv->handle < 0 || domain->id < 0)
+    if (priv->handle < 0 || domain->id < 0) {
+        virXenError (VIR_ERR_INTERNAL_ERROR, "xenHypervisorGetSchedulerParameters: priv->handle or domain->id invalid", 0);
         return -1;
+    }
 
     /*
      * Support only dom_interface_version >=5
      * (Xen3.1.0 or later)
      */
-    if (dom_interface_version < 5)
+    if (dom_interface_version < 5) {
+        virXenError(VIR_ERR_NO_XEN, "xenHypervisorGetSchedulerParameters unsupported in dom interface < 5", 0);
         return -1;
+    }
 
     if (hypervisor_version > 1) {
         xen_op_v2_sys op_sys;
         xen_op_v2_dom op_dom;
+        int ret;
 
         memset(&op_sys, 0, sizeof(op_sys));
         op_sys.cmd = XEN_V2_OP_GETSCHEDULERID;
         ret = xenHypervisorDoV2Sys(priv->handle, &op_sys);
+        if (ret == -1) return -1;
 
         switch (op_sys.u.getschedulerid.sched_id){
         case XEN_SCHEDULER_SEDF:
@@ -1071,13 +1083,16 @@
             params[1].type = VIR_DOMAIN_SCHED_FIELD_UINT;
             params[1].value.ui = op_dom.u.getschedinfo.u.credit.cap;
 
-            ret = 0;
             break;
+
         default:
-            break;
+            virXenError(VIR_ERR_INVALID_ARG,
+                        "sched_id", op_sys.u.getschedulerid.sched_id);
+            return -1;
         }
     }
-    return ret;
+
+    return 0;
 }
 
 /**
@@ -1091,35 +1106,42 @@
  */
 int
 xenHypervisorSetSchedulerParameters(virDomainPtr domain,
-				 virSchedParameterPtr params, int *nparams)
+				 virSchedParameterPtr params, int nparams)
 {
-    int ret = -1;
     int i;
     xenUnifiedPrivatePtr priv;
     char str_weight[] ="weight";
     char str_cap[]    ="cap";
 
-    if ((domain == NULL) || (domain->conn == NULL))
+    if ((domain == NULL) || (domain->conn == NULL)) {
+        virXenError (VIR_ERR_INTERNAL_ERROR, "xenHypervisorSetSchedulerParameters: domain or conn is NULL", 0);
         return -1;
+    }
 
     priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
-    if (priv->handle < 0 || domain->id < 0)
+    if (priv->handle < 0 || domain->id < 0) {
+        virXenError (VIR_ERR_INTERNAL_ERROR, "xenHypervisorSetSchedulerParameters: priv->handle or domain->id invalid", 0);
         return -1;
+    }
 
     /*
      * Support only dom_interface_version >=5
      * (Xen3.1.0 or later)
      */
-    if (dom_interface_version < 5)
+    if (dom_interface_version < 5) {
+        virXenError(VIR_ERR_NO_XEN, "xenHypervisorSetSchedulerParameters unsupported in dom interface < 5", 0);
         return -1;
+    }
 
     if (hypervisor_version > 1) {
         xen_op_v2_sys op_sys;
         xen_op_v2_dom op_dom;
+        int ret;
 
         memset(&op_sys, 0, sizeof(op_sys));
         op_sys.cmd = XEN_V2_OP_GETSCHEDULERID;
         ret = xenHypervisorDoV2Sys(priv->handle, &op_sys);
+        if (ret == -1) return -1;
 
         switch (op_sys.u.getschedulerid.sched_id){
         case XEN_SCHEDULER_SEDF:
@@ -1138,7 +1160,7 @@
             op_dom.u.getschedinfo.u.credit.weight = 0;
             op_dom.u.getschedinfo.u.credit.cap    = (uint16_t)~0U;
 
-            for(i = 0;i < *nparams; i++ ){
+            for(i = 0;i < nparams; i++ ){
                 if(!strncmp(params[i].field,str_weight,strlen(str_weight)) &&
                   params[i].type == VIR_DOMAIN_SCHED_FIELD_UINT)
                      op_dom.u.getschedinfo.u.credit.weight = 
@@ -1149,13 +1171,16 @@
                          params[i].value.ui;
             }
             ret = xenHypervisorDoV2Dom(priv->handle, &op_dom);
+            if (ret == -1) return -1;
             break;
         default:
-            break;
+            virXenError(VIR_ERR_INVALID_ARG,
+                        "sched_id", op_sys.u.getschedulerid.sched_id);
+            return -1;
         }
     }
      
-    return(ret);
+    return 0;
 }
 
 /**
diff -ur --exclude=CVS libvirt-addscheduler10/src/xen_internal.h libvirt-addscheduler10rich/src/xen_internal.h
--- libvirt-addscheduler10/src/xen_internal.h	2007-05-30 16:00:27.000000000 +0100
+++ libvirt-addscheduler10rich/src/xen_internal.h	2007-05-30 16:10:45.000000000 +0100
@@ -75,7 +75,7 @@
 
 int	xenHypervisorSetSchedulerParameters		(virDomainPtr domain,
 					 virSchedParameterPtr params,
-					 int *nparams);
+					 int nparams);
 
 #ifdef __cplusplus
 }
diff -ur --exclude=CVS libvirt-addscheduler10/src/xen_unified.c libvirt-addscheduler10rich/src/xen_unified.c
--- libvirt-addscheduler10/src/xen_unified.c	2007-05-30 16:00:27.000000000 +0100
+++ libvirt-addscheduler10rich/src/xen_unified.c	2007-05-30 16:08:16.000000000 +0100
@@ -810,7 +810,7 @@
 
 static int
 xenUnifiedDomainSetSchedulerParameters (virDomainPtr dom,
-                    virSchedParameterPtr params, int *nparams)
+                    virSchedParameterPtr params, int nparams)
 {
     GET_PRIVATE(dom->conn);
     int i;

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]