Daniel Veillard wrote:

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 :-)

I agree :-)

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 ?).

This sounds like a pretty uncommon case - only the HP PA-RISC that I've found. This could certainly be addressed though...

Still maybe it's a good idea to pass a pointer in the allocated area ...

This could be done with a small loss (looks like 16 bytes) of stack space.

Daniel

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; } } [...] --- 819,825 ---- void lxcFreeVM(lxc_vm_t *vm) { lxcFreeVMDef(vm->def); ! free(vm); }

This breaks doesn't it? After calling lxcFreeVM(), curVm is no longer valid since it was free()'d. Need to pull out the next pointer before lxcFreeVM(). -- Best Regards, Dave Leskovec IBM Linux Technology Center Open Virtualization

On Mon, Mar 31, 2008 at 03:24:22PM -0700, Dave Leskovec wrote:

Meant to include this in the last reply - Here's a patch for the lxcFreeVMs() issue:

...

This breaks doesn't it? After calling lxcFreeVM(), curVm is no longer valid since it was free()'d. Need to pull out the next pointer before lxcFreeVM().

yup, obviously access to freed memory, my bad ...

while (curVm) { - lxcFreeVM(curVm); nextVm = curVm->next; + lxcFreeVM(curVm); curVm = nextVm;

Applied and commited, 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/

Daniel Veillard wrote:

On Fri, Mar 28, 2008 at 09:03:26AM -0400, Daniel Veillard wrote:

...

Patch against CVS version, it tries to avoid 2 issues: - problem with global data settings when loading of the driver got interrupted. Check more NULLs and reset to NULL, i was getting crashes when running the regression tests and the /etc/libvirtd/lxc was not accessible. - avoid trying to open the driver if not running as root, that's not a problem in normal case because the daemon runs as root, but when running 'make check' as a normal user libvirtd is run as an user and compiling the lxc driver in would break the regression tests

Patch applied to CVS too,

Daniel

Great, looks good. Thanks! -- Best Regards, Dave Leskovec IBM Linux Technology Center Open Virtualization