[Libvir] [PATCH] lxc: start domain

[Daniel Veillard]

On Thu, Mar 27, 2008 at 02:50:20PM -0700, Dave Leskovec wrote:
> Daniel Veillard wrote:
> >> +    int execStringLen = strlen(vmDef->init) + 1 + 5;
> >> +    strcpy(execString, "exec ");
> >> +    strcat(execString, vmDef->init);
> > 
> >  Hum, it seems there is an off by one allocation error, you don't allocate
> > the space for the terminating 0
> 
> A comment probably would have helped here :-)  the + 1 up there in setting the
> execStringLen is for the NULL.

  Oops, for some reason I counted 5 for 'exec', ahum :-)

[...]
> >> +    vm->def->id = vm->pid;
> >> +    lxcSaveConfig(NULL, driver, vm, vm->def);
> > 
> >   We should make sure at some point taht there is some kind of locking
> > mechanism when creating those config files, no ?
> 
> Good question.  Right now libvirtd should be the only process accessing the
> file.  Is it multi-threaded that would cause collisions?  The other possibility
> is if we found we needed to save the config file from within the container.
> That's not currently the case and I'd stay away from that if at all possible.

  Okay, then that's not needed, fine by me :-)

> > [...]
> > 
> >   Hum, now that config names are saved based on domain names, wouldn't
> > it be better to use a name based lookup ? Less precise but more direct
> 
> Not sure what you're referring to here.  name based lookup for what?

  Hum, wrong part quoted, it's about lxcDomainStart() just below:

:+static int lxcDomainStart(virDomainPtr dom)
:+{
:+    int rc = -1;
:+    virConnectPtr conn = dom->conn;
:+    lxc_driver_t *driver = (lxc_driver_t *)(conn->privateData);
:+    lxc_vm_t *vm = lxcFindVMByUUID(driver, dom->uuid);
:+

> >   Looks fine overall. I wonder if 1 fork/clone per container can't be
> > reduced to a single process doing the fd processing for all the containers
> > running. But that's probably harder, more fragile and for little gain.
> 
> Yes, that's been tossed around.  I'm holding off pursuing that for now until
> devpts namespace and it's impacts are known.

  Okay, just wondering :-)

    thanks !

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/