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

Re: [Libvir] [PATCH]OpenVZ Enhancements - Cleaned up



On Mon, Sep 03, 2007 at 08:39:27AM -0400, Daniel Veillard wrote:
> On Mon, Sep 03, 2007 at 08:01:00AM -0400, Shuveb Hussain wrote:
> > Hi,
> > 
> > Attached are the OpenVZ driver enhancements. Cleanups have been done  
> > according to Daniel .V's suggestions. Please see my previous mail for  
> > summary.
> 
>   Okay, that looks better, thanks a lot !
> I will apply them but there is a few things I still have issues with,
> I will try to fix them before commiting the patches:

  Okay I commited to CVS earlier today

>   - some really bizare changes of an xmlDoc name field:
>     +    if (!(xml->name = calloc(1, PATH_MAX))) {
>     +        openvzLog(OPENVZ_ERR, "Error in allocating memory to store xml URI");
>     +        xmlFreeDoc(xml);
>     +        return NULL;
>     +    }
>     +    if (displayName)
>     +        strncpy(xml->name, displayName, PATH_MAX - 1);
>     +    else
>     +        strncpy(xml->name, "domain.xml", PATH_MAX - 1);
>     that's bad because: 
>       + it will probably leak the previous value of xml->name
>       + xml->name should be allocated using xmlMalloc
>       + why allocate MAX_PATH when you can allocate just the size
>     so I will cleanup that part, still I don't undertsand what you
>     tried to do there.

  I removed that part. The URI information passed to the xmlDocRead earlier
should result in the same xml->URI being set, so I don't see the point
of that duplication, dropped for now.

>  This should hit CVS later today when I cleaned up those things and possibly
> a couple of warnings,

  I am also applying that second patch to clean thing up, this removes most
warnings at compile time, fix a serious deallocation problem on error when
listing the domains, and change strtoI to indicate it does not change the
input string.
  There is still 3 serious warnings in openvz_conf.c around line 700, where
the return value of write is not checked and as I indicated previously we
should use a wrapper function handling interrupted calls at least. But that
can be done later,

  thanks,

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/
Index: src/openvz_conf.c
===================================================================
RCS file: /data/cvs/libxen/src/openvz_conf.c,v
retrieving revision 1.5
diff -u -r1.5 openvz_conf.c
--- src/openvz_conf.c	3 Sep 2007 15:37:07 -0000	1.5
+++ src/openvz_conf.c	3 Sep 2007 16:18:08 -0000
@@ -59,9 +59,6 @@
 static struct openvz_vm_def *openvzParseXML(virConnectPtr conn, xmlDocPtr xml);
 static int openvzGetVPSUUID(int vpsid, char *uuidstr);
 static int openvzSetUUID(int vpsid);
-static struct openvz_vm *openvzLoadConfig(struct openvz_driver *driver,
-                                          const char *path,
-                                          const char *xmlStr);
 
 /* For errors internal to this library. */
 static void
@@ -117,7 +114,7 @@
 }
 
 int
-strtoI(char *str)
+strtoI(const char *str)
 {
     int base = 10;
     char *endptr;
@@ -340,7 +337,7 @@
     }
     
     /* rejecting VPS ID <= OPENVZ_RSRV_VM_LIMIT for they are reserved */
-    if (strtoI(BAD_CAST obj->stringval) <= OPENVZ_RSRV_VM_LIMIT) {
+    if (strtoI((const char *) obj->stringval) <= OPENVZ_RSRV_VM_LIMIT) {
         error(conn, VIR_ERR_INTERNAL_ERROR, 
                 "VPS ID Error (must be an integer greater than 100");
         goto bail_out;
@@ -531,13 +528,18 @@
         *pnext = calloc(1, sizeof(struct openvz_vm));
         if(!*pnext) {
             error(conn, VIR_ERR_INTERNAL_ERROR, "calloc failed");
-            return NULL;
+            goto error;
         }
         
         if(!vm)
             vm = *pnext;
 
-        fscanf(fp, "%d %s\n", &veid, status);
+        if (fscanf(fp, "%d %s\n", &veid, status) != 2) {
+	    error(conn, VIR_ERR_INTERNAL_ERROR,
+	          "Failed to parse vzlist output");
+            free(*pnext);
+	    goto error;
+	}
         if(strcmp(status, "stopped")) { 
             (*pnext)->status = VIR_DOMAIN_RUNNING;
             driver->num_active ++;
@@ -546,14 +548,18 @@
         else {
             (*pnext)->status = VIR_DOMAIN_SHUTOFF;
             driver->num_inactive ++;
-            (*pnext)->vpsid = -1;    /* inactive domains don't have their ID set in libvirt,
-                                        thought this doesn't make sense for OpenVZ */
+	    /*
+	     * inactive domains don't have their ID set in libvirt,
+	     * thought this doesn't make sense for OpenVZ
+	     */
+            (*pnext)->vpsid = -1; 
         }
 
         vmdef = calloc(1, sizeof(struct openvz_vm_def));
         if(!vmdef) {
             error(conn, VIR_ERR_INTERNAL_ERROR, "calloc failed");
-            return NULL;
+            free(*pnext);
+	    goto error;
         }
         
         snprintf(vmdef->name, OPENVZ_NAME_MAX,  "%i", veid);
@@ -561,14 +567,27 @@
         ret = virUUIDParse(uuidstr, vmdef->uuid);
 
         if(ret == -1) {
-            error(conn, VIR_ERR_INTERNAL_ERROR, "UUID in config file malformed");
-            return NULL;
+            error(conn, VIR_ERR_INTERNAL_ERROR,
+	          "UUID in config file malformed");
+            free(*pnext);
+	    free(vmdef);
+            goto error;
         }
 
         (*pnext)->vmdef = vmdef;
         pnext = &(*pnext)->next;
     }
     return vm;
+error:
+    while (vm != NULL) {
+        struct openvz_vm *next;
+
+	next = vm->next;
+	free(vm->vmdef);
+	free(vm);
+	vm = next;
+    }
+    return NULL;
 }
 
 static char 
@@ -579,7 +598,7 @@
 
     while(conf_dir_list[i]) {
         if(!access(conf_dir_list[i], F_OK))
-                return strdup(conf_dir_list[i]);
+	    return strdup(conf_dir_list[i]);
         i ++;
     }
 
@@ -660,7 +679,7 @@
     char uuidstr[VIR_UUID_STRING_BUFLEN];
     unsigned char uuid[VIR_UUID_BUFLEN];
     char *conf_dir;
-    int fd, ret, i;
+    int fd, ret;
 
     conf_dir = openvzLocateConfDir();
     sprintf(conf_file, "%s/%d.conf", conf_dir, vpsid);
Index: src/openvz_conf.h
===================================================================
RCS file: /data/cvs/libxen/src/openvz_conf.h,v
retrieving revision 1.4
diff -u -r1.4 openvz_conf.h
--- src/openvz_conf.h	3 Sep 2007 15:37:07 -0000	1.4
+++ src/openvz_conf.h	3 Sep 2007 16:18:08 -0000
@@ -129,5 +129,5 @@
 void openvzFreeDriver(struct openvz_driver *driver);
 void openvzFreeVM(struct openvz_driver *driver, struct openvz_vm *vm, int checkCallee);
 void openvzFreeVMDef(struct openvz_vm_def *def);
-int strtoI(char *str);
+int strtoI(const char *str);
 #endif /* OPENVZ_CONF_H */
Index: src/openvz_driver.c
===================================================================
RCS file: /data/cvs/libxen/src/openvz_driver.c,v
retrieving revision 1.5
diff -u -r1.5 openvz_driver.c
--- src/openvz_driver.c	3 Sep 2007 15:37:07 -0000	1.5
+++ src/openvz_driver.c	3 Sep 2007 16:18:08 -0000
@@ -164,7 +164,7 @@
     return dom;
 }
 
-static char *openvzGetOSType(virDomainPtr dom)
+static char *openvzGetOSType(virDomainPtr dom ATTRIBUTE_UNUSED)
 {
     /* OpenVZ runs on Linux and runs only Linux */
     return strdup("linux");
@@ -275,7 +275,8 @@
     return ret;
 }
 
-static int openvzDomainReboot(virDomainPtr dom, unsigned int flags) {
+static int openvzDomainReboot(virDomainPtr dom,
+                              unsigned int flags ATTRIBUTE_UNUSED) {
     char cmdbuf[CMDBUF_LEN];
     int ret;
     char *cmdExec[OPENVZ_MAX_ARG];
@@ -631,7 +632,7 @@
     return got;
 }
 
-static int openvzNumDomains(virConnectPtr conn) {
+static int openvzNumDomains(virConnectPtr conn ATTRIBUTE_UNUSED) {
     return ovz_driver.num_active;
 }
 
@@ -662,7 +663,7 @@
     return got;
 }
 
-static int openvzNumDefinedDomains(virConnectPtr conn) {
+static int openvzNumDefinedDomains(virConnectPtr conn ATTRIBUTE_UNUSED) {
     return ovz_driver.num_inactive; 
 }
 
@@ -687,11 +688,11 @@
     return 1;
 }
 
-static int openvzCloseNetwork(virConnectPtr conn) {
+static int openvzCloseNetwork(virConnectPtr conn ATTRIBUTE_UNUSED) {
     return 0;
 }
 
-static virDrvOpenStatus openvzOpenNetwork(virConnectPtr conn,
+static virDrvOpenStatus openvzOpenNetwork(virConnectPtr conn ATTRIBUTE_UNUSED,
                                          const char *name ATTRIBUTE_UNUSED,
                                          int flags ATTRIBUTE_UNUSED) {
     return VIR_DRV_OPEN_SUCCESS;
@@ -747,6 +748,11 @@
     NULL, /* domainGetSchedulerType */
     NULL, /* domainGetSchedulerParameters */
     NULL, /* domainSetSchedulerParameters */
+    NULL, /* domainMigratePrepare */
+    NULL, /* domainMigratePerform */
+    NULL, /* domainMigrateFinish */
+    NULL, /* domainBlockStats */
+    NULL, /* domainInterfaceStats */
 };
 
 static virNetworkDriver openvzNetworkDriver = {

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