[Libvir] Re: Lxc conf patch

[Dave Leskovec]

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