[Libvir] Lxc conf patch

Daniel Veillard veillard at redhat.com
Fri Mar 28 13:46:38 UTC 2008


  Okay, that's something i promised last week, but it has some significant
changes:
   - cleanup the XPath methods to access the XML informations, reusing the
     existing methods from xml.h
   - make string fields from __lxc_vm_def dynamic (except the UUID)
   - fix what looked like a funny leak situation when vm def structures were
     deallocated (see changes around lxcFreeVM/lxcFreeVMs/lxcFreeVMDef),
     i hope i didn't overlooked something but valgrind seems happy now leak
     wise.
Overall it looks a bit simpler to me :-)

Valgrind only report left is
==1== Invalid write of size 4
==1==    at 0x470C8C: lxcCheckContainerSupport (lxc_driver.c:91)
==1==  Address 0xFFFFFFFFFFFFFFEC is not stack'd, malloc'd or (recently) free'd
==1== 
==1== Process terminating with default action of signal 11 (SIGSEGV)
==1==  Access not within mapped region at address 0xFFFFFFFFFFFFFFEC
==1==    at 0x470C8C: lxcCheckContainerSupport (lxc_driver.c:91)

 which is related to the probe code of the driver
---------------------------
    stack = malloc(getpagesize() * 4);
    if(!stack) {
        DEBUG0("Unable to allocate stack");
        rc = -1;
        goto check_complete;
    }

    childStack = stack + (getpagesize() * 4);

    cpid = clone(lxcDummyChild, childStack, flags, NULL);
---------------------------
 I would assume it's valgrind being a bit pedantic because we pass the address
just over the allocated stack limit, which should be fine since stack is
addressed in predecrementing mode (BTW isn't that a portability bug in the
code stack direction should probably be checked no ?). Still maybe it's a
good idea to pass a pointer in the allocated area ...

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/
-------------- next part --------------
Index: src/lxc_conf.c
===================================================================
RCS file: /data/cvs/libxen/src/lxc_conf.c,v
retrieving revision 1.3
diff -p -r1.3 lxc_conf.c
*** src/lxc_conf.c	27 Mar 2008 14:02:57 -0000	1.3
--- src/lxc_conf.c	28 Mar 2008 13:29:58 -0000
***************
*** 41,46 ****
--- 41,47 ----
  #include "buf.h"
  #include "util.h"
  #include "uuid.h"
+ #include "xml.h"
  
  #include "lxc_conf.h"
  
*************** void lxcError(virConnectPtr conn, virDom
*** 70,87 ****
                      codeErrorMessage, errorMessage);
  }
  
- static inline int lxcIsEmptyXPathStringObj(xmlXPathObjectPtr xpathObj)
- {
-     if ((xpathObj == NULL) ||
-         (xpathObj->type != XPATH_STRING) ||
-         (xpathObj->stringval == NULL) ||
-         (xpathObj->stringval[0] == 0)) {
-         return 1;
-     } else {
-         return 0;
-     }
- }
- 
  static int lxcParseMountXML(virConnectPtr conn, xmlNodePtr nodePtr,
                              lxc_mount_t *lxcMount)
  {
--- 71,76 ----
*************** static int lxcParseMountXML(virConnectPt
*** 92,98 ****
      int strLen;
      int rc = -1;
  
!     if (NULL == (fsType = xmlGetProp(nodePtr, BAD_CAST "type"))) {
          lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
                   _("missing filesystem type"));
          goto error;
--- 81,88 ----
      int strLen;
      int rc = -1;
  
!     fsType = xmlGetProp(nodePtr, BAD_CAST "type");
!     if (NULL == fsType) {
          lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
                   _("missing filesystem type"));
          goto error;
*************** static int lxcParseMountXML(virConnectPt
*** 155,160 ****
--- 145,151 ----
      rc = 0;
  
  error:
+     xmlFree(fsType);
      xmlFree(mountSource);
      xmlFree(mountTarget);
  
*************** error:
*** 164,218 ****
  static int lxcParseDomainName(virConnectPtr conn, char **name,
                                xmlXPathContextPtr contextPtr)
  {
!     int rc = -1;
!     xmlXPathObjectPtr xpathObj = NULL;
  
!     xpathObj = xmlXPathEval(BAD_CAST "string(/domain/name[1])", contextPtr);
!     if (lxcIsEmptyXPathStringObj(xpathObj)) {
          lxcError(conn, NULL, VIR_ERR_NO_NAME, NULL);
!         goto parse_complete;
      }
  
!     *name = strdup((const char *)xpathObj->stringval);
!     if (NULL == *name) {
!         lxcError(conn, NULL, VIR_ERR_NO_MEMORY, NULL);
!         goto parse_complete;
!     }
! 
! 
!     rc = 0;
! 
! parse_complete:
!     xmlXPathFreeObject(xpathObj);
!     return rc;
  }
  
  static int lxcParseDomainUUID(virConnectPtr conn, unsigned char *uuid,
                                xmlXPathContextPtr contextPtr)
  {
!     int rc = -1;
!     xmlXPathObjectPtr xpathObj = NULL;
  
!     xpathObj = xmlXPathEval(BAD_CAST "string(/domain/uuid[1])", contextPtr);
!     if (lxcIsEmptyXPathStringObj(xpathObj)) {
!         if ((rc = virUUIDGenerate(uuid))) {
              lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
!                      _("failed to generate uuid: %s"), strerror(rc));
!             goto parse_complete;
          }
      } else {
!         if (virUUIDParse((const char *)xpathObj->stringval, uuid) < 0) {
              lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
                       _("invalid uuid element"));
!             goto parse_complete;
          }
      }
! 
!     rc = 0;
! 
! parse_complete:
!     xmlXPathFreeObject(xpathObj);
!     return rc;
  }
  
  static int lxcParseDomainMounts(virConnectPtr conn,
--- 155,194 ----
  static int lxcParseDomainName(virConnectPtr conn, char **name,
                                xmlXPathContextPtr contextPtr)
  {
!     char *res;
  
!     res = virXPathString("string(/domain/name[1])", contextPtr);
!     if (res == NULL) {
          lxcError(conn, NULL, VIR_ERR_NO_NAME, NULL);
!         return(-1);
      }
  
!     *name = res;
!     return(0);
  }
  
  static int lxcParseDomainUUID(virConnectPtr conn, unsigned char *uuid,
                                xmlXPathContextPtr contextPtr)
  {
!     char *res;
  
!     res = virXPathString("string(/domain/uuid[1])", contextPtr);
!     if (res == NULL) {
!         if (virUUIDGenerate(uuid)) {
              lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
!                      _("failed to generate uuid"));
!             return(-1);
          }
      } else {
!         if (virUUIDParse(res, uuid) < 0) {
              lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
                       _("invalid uuid element"));
! 	    free(res);
!             return(-1);
          }
+ 	free(res);
      }
!     return(0);
  }
  
  static int lxcParseDomainMounts(virConnectPtr conn,
*************** static int lxcParseDomainMounts(virConne
*** 220,246 ****
                                  xmlXPathContextPtr contextPtr)
  {
      int rc = -1;
-     xmlXPathObjectPtr xpathObj = NULL;
      int i;
      lxc_mount_t *mountObj;
      lxc_mount_t *prevObj = NULL;
      int nmounts = 0;
  
!     xpathObj = xmlXPathEval(BAD_CAST "/domain/devices/filesystem",
!                             contextPtr);
!     if ((xpathObj != NULL) &&
!         (xpathObj->type == XPATH_NODESET) &&
!         (xpathObj->nodesetval != NULL) &&
!         (xpathObj->nodesetval->nodeNr >= 0)) {
!         for (i = 0; i < xpathObj->nodesetval->nodeNr; ++i) {
              mountObj = calloc(1, sizeof(lxc_mount_t));
              if (NULL == mountObj) {
                  lxcError(conn, NULL, VIR_ERR_NO_MEMORY, "mount");
                  goto parse_complete;
              }
  
!             rc = lxcParseMountXML(conn, xpathObj->nodesetval->nodeTab[i],
!                                   mountObj);
              if (0 > rc) {
                  free(mountObj);
                  goto parse_complete;
--- 196,218 ----
                                  xmlXPathContextPtr contextPtr)
  {
      int rc = -1;
      int i;
      lxc_mount_t *mountObj;
      lxc_mount_t *prevObj = NULL;
      int nmounts = 0;
+     xmlNodePtr *list;
+     int res;
  
!     res = virXPathNodeSet("/domain/devices/filesystem", contextPtr, &list);
!     if (res > 0) {
!         for (i = 0; i < res; ++i) {
              mountObj = calloc(1, sizeof(lxc_mount_t));
              if (NULL == mountObj) {
                  lxcError(conn, NULL, VIR_ERR_NO_MEMORY, "mount");
                  goto parse_complete;
              }
  
!             rc = lxcParseMountXML(conn, list[i], mountObj);
              if (0 > rc) {
                  free(mountObj);
                  goto parse_complete;
*************** static int lxcParseDomainMounts(virConne
*** 256,356 ****
              }
              prevObj = mountObj;
          }
      }
  
      rc = nmounts;
  
  parse_complete:
-     xmlXPathFreeObject(xpathObj);
- 
      return rc;
  }
  
! static int lxcParseDomainInit(virConnectPtr conn, char* init, xmlXPathContextPtr contextPtr)
  {
!     int rc = -1;
!     xmlXPathObjectPtr xpathObj = NULL;
  
!     xpathObj = xmlXPathEval(BAD_CAST "string(/domain/os/init[1])",
!                            contextPtr);
!     if (lxcIsEmptyXPathStringObj(xpathObj)) {
          lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
                   _("invalid or missing init element"));
!         goto parse_complete;
      }
  
!     if (strlen((const char*)xpathObj->stringval) >= PATH_MAX - 1) {
          lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
                   _("init string too long"));
!         goto parse_complete;
      }
  
!     strcpy(init, (const char *)xpathObj->stringval);
! 
!     rc = 0;
! 
! parse_complete:
!     xmlXPathFreeObject(xpathObj);
  
!     return rc;
  }
  
  
! static int lxcParseDomainTty(virConnectPtr conn, char *tty, xmlXPathContextPtr contextPtr)
  {
!     int rc = -1;
!     xmlXPathObjectPtr xpathObj = NULL;
  
!     xpathObj = xmlXPathEval(BAD_CAST "string(/domain/devices/console[1]/@tty)",
!                             contextPtr);
!     if (lxcIsEmptyXPathStringObj(xpathObj)) {
          /* make sure the tty string is empty */
!         tty[0] = 0x00;
      } else {
!         /* check the source string length */
!         if (strlen((const char*)xpathObj->stringval) >= LXC_MAX_TTY_NAME - 1) {
!             lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
!                      _("tty name is too long"));
!             goto parse_complete;
!         }
!         strcpy(tty, (const char *)xpathObj->stringval);
      }
  
!     rc = 0;
! 
! parse_complete:
!     xmlXPathFreeObject(xpathObj);
! 
!     return rc;
  }
  
  static int lxcParseDomainMemory(virConnectPtr conn, int* memory, xmlXPathContextPtr contextPtr)
  {
!     int rc = -1;
!     xmlXPathObjectPtr xpathObj = NULL;
!     char *endChar = NULL;
  
!     xpathObj = xmlXPathEval(BAD_CAST "string(/domain/memory[1])", contextPtr);
!     if (lxcIsEmptyXPathStringObj(xpathObj)) {
          /* not an error, default to an invalid value so it's not used */
!         *memory = -1;
      } else {
!         *memory = strtoll((const char*)xpathObj->stringval,
!                                           &endChar, 10);
!         if ((endChar == (const char*)xpathObj->stringval) ||
!            (*endChar != '\0')) {
!             lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
!                      _("invalid memory value"));
!             goto parse_complete;
!         }
      }
! 
!     rc = 0;
! 
! parse_complete:
!     xmlXPathFreeObject(xpathObj);
! 
!     return rc;
  }
  
  static lxc_vm_def_t * lxcParseXML(virConnectPtr conn, xmlDocPtr docPtr)
--- 228,303 ----
              }
              prevObj = mountObj;
          }
+ 	free(list);
      }
  
      rc = nmounts;
  
  parse_complete:
      return rc;
  }
  
! static int lxcParseDomainInit(virConnectPtr conn, char** init,
!                               xmlXPathContextPtr contextPtr)
  {
!     char *res;
  
!     res = virXPathString("string(/domain/os/init[1])", contextPtr);
!     if (res == NULL) {
          lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
                   _("invalid or missing init element"));
!         return(-1);
      }
  
!     if (strlen(res) >= PATH_MAX - 1) {
          lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
                   _("init string too long"));
!         free(res);
! 	return(-1);
      }
  
!     *init = res;
  
!     return(0);
  }
  
  
! static int lxcParseDomainTty(virConnectPtr conn, char **tty, xmlXPathContextPtr contextPtr)
  {
!     char *res;
  
!     res = virXPathString("string(/domain/devices/console[1]/@tty)", contextPtr);
!     if (res == NULL) {
          /* make sure the tty string is empty */
!         *tty = strdup("");
! 	if (*tty == NULL) {
! 	    lxcError(conn, NULL, VIR_ERR_NO_MEMORY, NULL);
! 	    return(-1);
! 	}
      } else {
!         *tty = res;
      }
  
!     return(0);
  }
  
  static int lxcParseDomainMemory(virConnectPtr conn, int* memory, xmlXPathContextPtr contextPtr)
  {
!     long res;
!     int rc;
  
!     rc = virXPathLong("string(/domain/memory[1])", contextPtr, &res);
!     if ((rc == -2) || ((rc == 0) && (res <= 0))) {
! 	*memory = -1;
! 	lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
! 	         _("invalid memory value"));
!     } else if (rc < 0) {
          /* not an error, default to an invalid value so it's not used */
! 	*memory = -1;
      } else {
! 	*memory = (int) res;
      }
!     return(0);
  }
  
  static lxc_vm_def_t * lxcParseXML(virConnectPtr conn, xmlDocPtr docPtr)
*************** static lxc_vm_def_t * lxcParseXML(virCon
*** 411,417 ****
          goto error;
      }
  
!     if (lxcParseDomainInit(conn, containerDef->init, contextPtr) < 0) {
          goto error;
      }
  
--- 358,364 ----
          goto error;
      }
  
!     if (lxcParseDomainInit(conn, &(containerDef->init), contextPtr) < 0) {
          goto error;
      }
  
*************** static lxc_vm_def_t * lxcParseXML(virCon
*** 425,431 ****
          goto error;
      }
  
!     if (lxcParseDomainTty(conn, containerDef->tty, contextPtr) < 0) {
          goto error;
      }
  
--- 372,378 ----
          goto error;
      }
  
!     if (lxcParseDomainTty(conn, &(containerDef->tty), contextPtr) < 0) {
          goto error;
      }
  
*************** int lxcLoadContainerInfo(lxc_driver_t *d
*** 708,714 ****
              rc = 0;
          } else {
              lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
!                      _("failed to open config directory: %s"), strerror(errno));
          }
  
          goto load_complete;
--- 655,662 ----
              rc = 0;
          } else {
              lxcError(NULL, NULL, VIR_ERR_INTERNAL_ERROR,
!                      _("failed to open config directory %s: %s"),
! 		     driver->configDir, strerror(errno));
          }
  
          goto load_complete;
*************** no_memory:
*** 837,845 ****
  
  void lxcFreeVMDef(lxc_vm_def_t *vmdef)
  {
!     lxc_mount_t *curMount = vmdef->mounts;
      lxc_mount_t *nextMount;
  
      while (curMount) {
          nextMount = curMount->next;
          free(curMount);
--- 785,797 ----
  
  void lxcFreeVMDef(lxc_vm_def_t *vmdef)
  {
!     lxc_mount_t *curMount;
      lxc_mount_t *nextMount;
  
+     if (vmdef == NULL)
+         return;
+ 
+     curMount = vmdef->mounts;
      while (curMount) {
          nextMount = curMount->next;
          free(curMount);
*************** void lxcFreeVMDef(lxc_vm_def_t *vmdef)
*** 847,854 ****
      }
  
      free(vmdef->name);
!     vmdef->name = NULL;
! 
  }
  
  void lxcFreeVMs(lxc_vm_t *vms)
--- 799,807 ----
      }
  
      free(vmdef->name);
!     free(vmdef->init);
!     free(vmdef->tty);
!     free(vmdef);
  }
  
  void lxcFreeVMs(lxc_vm_t *vms)
*************** void lxcFreeVMs(lxc_vm_t *vms)
*** 859,865 ****
      while (curVm) {
          lxcFreeVM(curVm);
          nextVm = curVm->next;
-         free(curVm);
          curVm = nextVm;
      }
  }
--- 812,817 ----
*************** void lxcFreeVMs(lxc_vm_t *vms)
*** 867,873 ****
  void lxcFreeVM(lxc_vm_t *vm)
  {
      lxcFreeVMDef(vm->def);
!     free(vm->def);
  }
  
  lxc_vm_t *lxcFindVMByID(const lxc_driver_t *driver, int id)
--- 819,825 ----
  void lxcFreeVM(lxc_vm_t *vm)
  {
      lxcFreeVMDef(vm->def);
!     free(vm);
  }
  
  lxc_vm_t *lxcFindVMByID(const lxc_driver_t *driver, int id)
Index: src/lxc_conf.h
===================================================================
RCS file: /data/cvs/libxen/src/lxc_conf.h,v
retrieving revision 1.2
diff -p -r1.2 lxc_conf.h
*** src/lxc_conf.h	27 Mar 2008 09:34:06 -0000	1.2
--- src/lxc_conf.h	28 Mar 2008 13:29:58 -0000
*************** struct __lxc_vm_def {
*** 51,57 ****
      int id;
  
      /* init command string */
!     char init[PATH_MAX];
  
      int maxMemory;
  
--- 51,57 ----
      int id;
  
      /* init command string */
!     char *init;
  
      int maxMemory;
  
*************** struct __lxc_vm_def {
*** 60,66 ****
      lxc_mount_t *mounts;
  
      /* tty device */
!     char tty[LXC_MAX_TTY_NAME];
  };
  
  typedef struct __lxc_vm lxc_vm_t;
--- 60,66 ----
      lxc_mount_t *mounts;
  
      /* tty device */
!     char *tty;
  };
  
  typedef struct __lxc_vm lxc_vm_t;


More information about the libvir-list mailing list