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

Re: [Libvir] [PATCH] lxc: start domain



On Wed, Mar 26, 2008 at 11:20:59PM -0700, Dave Leskovec wrote:
> This is a repost of patch four in the last series I posted.  It contains the
> start container support.  I've made some changes corresponding to Dan B's patch
> moving the lxc driver under libvirtd.  I removed the isolation forks and cleaned
> up the status handling and PID storing.

  General comment over the patch is that I prefer to see all function with
an header comment, it's a bit painful, but helps graps the context when an
error arise and someone who don't know the code try to sort it out. That's
not a blocker for commits but better for long term maintainance :-)

[...]
> +/* Functions */
> +static int lxcExecContainerInit(lxc_vm_def_t *vmDef)
> +{
> +    int rc = -1;
> +    char* execString;
> +    int execStringLen = strlen(vmDef->init) + 1 + 5;
> +
> +    if(NULL == (execString = calloc(execStringLen, sizeof(char)))) {

   An error should really be reported here.

> +        goto error_out;
> +    }
> +
> +    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

> +    execl("/bin/sh", "sh", "-c", execString, (char*)NULL);
> +    DEBUG("execl failed: %s", strerror(errno));
> +
> +error_out:
> +    exit(rc);
> +}
[...]

> +static int lxcTtyForward(int fd1, int fd2, int *loopFlag, int pollmsecs)
> +{
> +    int rc = -1;
> +    int i;
> +    char buf[2];
> +    struct pollfd fds[2];
> +    int numFds = 0;
> +
> +    if (0 <= fd1) {
> +        fds[numFds].fd = fd1;
> +        fds[numFds].events = POLLIN;
> +        ++numFds;
> +    }
> +
> +    if (0 <= fd2) {
> +        fds[numFds].fd = fd2;
> +        fds[numFds].events = POLLIN;
> +        ++numFds;
> +    }
> +
> +    if (0 == numFds) {
> +        DEBUG0("No fds to monitor, return");
> +        goto cleanup;
> +    }
> +
> +    while (!(*loopFlag)) {
> +        if ((rc = poll(fds, numFds, pollmsecs)) <= 0) {
> +            if(*loopFlag) {
> +                goto cleanup;
> +            }
> +
> +            if ((0 == rc) || (errno == EINTR) || (errno == EAGAIN)) {
> +                continue;
> +            }
> +
> +            DEBUG("poll returned error: %s", strerror(errno));
> +            goto cleanup;
> +        }
> +
> +        for (i = 0; i < numFds; ++i) {
> +            if (!fds[i].revents) {
> +                continue;
> +            }
> +
> +            if (fds[i].revents & POLLIN) {
> +                saferead(fds[i].fd, buf, 1);
> +                if (1 < numFds) {
> +                    safewrite(fds[i ^ 1].fd, buf, 1);
> +                }
> +

  So if only one fd is given we discard all data read, and if 2 fds are given
all data from one is forwarded to the other, right ?

> +static int exitChildLoop;
> +static void lxcExecChildHandler(int sig ATTRIBUTE_UNUSED,
> +                                siginfo_t *signalInfo,
> +                                void *context ATTRIBUTE_UNUSED)
> +{
> +    DEBUG("lxcExecChildHandler signal from %d\n", signalInfo->si_pid);
> +
> +    if (signalInfo->si_pid == initPid) {
> +        exitChildLoop = 1;
> +    } else {
> +        waitpid(signalInfo->si_pid, NULL, WNOHANG);
> +    }
> +
> +}
> +
> +static int lxcExecWithTty(lxc_vm_t *vm)
> +{
> +    int rc = -1;
> +    lxc_vm_def_t *vmDef = vm->def;
> +    int ttymaster = -1;
> +    int ttyslave = -1;
> +    struct sigaction sigAction;
> +    sigset_t sigMask;
> +    int childStatus;
> +
> +    if (lxcSetupContainerTty(&ttymaster, &ttyslave) < 0) {
> +        goto exit_with_error;
> +    }
> +
> +    sigAction.sa_sigaction = lxcExecChildHandler;
> +    sigfillset(&sigMask);
> +    sigAction.sa_mask = sigMask;
> +    sigAction.sa_flags = SA_SIGINFO;
> +    if (0 != sigaction(SIGCHLD, &sigAction, NULL)) {
> +        DEBUG("sigaction failed: %s\n", strerror(errno));
> +        goto exit_with_error;
> +    }
> +
> +    exitChildLoop = 0;
> +    if ((initPid = fork()) == 0) {
> +        if(lxcSetContainerStdio(ttyslave) < 0) {
> +            exitChildLoop = 1;
> +            goto exit_with_error;
> +        }
> +
> +        lxcExecContainerInit(vmDef);
> +        /* this function will not return.  if it fails, it will exit */
> +    }
> +
> +    close(ttyslave);
> +    lxcTtyForward(ttymaster, vm->parentTty,
> +                  &exitChildLoop, 100);
> +
> +    DEBUG("child waiting on pid %d", initPid);
> +    waitpid(initPid, &childStatus, 0);
> +    rc = WEXITSTATUS(childStatus);
> +    DEBUG("container exited with rc: %d", rc);
> +
> +exit_with_error:
> +    exit(rc);
> +}

  So this is forked for each container created, right ?

[...]
> Index: b/src/lxc_container.h
ok

> Index: b/src/lxc_driver.c
> ===================================================================
> --- a/src/lxc_driver.c	2008-03-21 11:46:11.000000000 -0700
> +++ b/src/lxc_driver.c	2008-03-24 16:46:48.000000000 -0700
> @@ -25,14 +25,17 @@
[...]
> +static int lxcStartContainer(virConnectPtr conn,
> +                             lxc_driver_t* driver,
> +                             lxc_vm_t *vm)
> +{
> +    int rc = -1;
> +    int flags;
> +    int stacksize = getpagesize() * 4;
> +    void *stack, *stacktop;
> +
> +    /* allocate a stack for the container */
> +    stack = malloc(stacksize);
> +    if (!stack) {
> +        lxcError(conn, NULL, VIR_ERR_NO_MEMORY,
> +                 _("unable to allocate container stack"));
> +        goto error_exit;
> +    }
> +    stacktop = (char*)stack + stacksize;
> +
> +    flags = CLONE_NEWPID|CLONE_NEWNS|CLONE_NEWUTS|CLONE_NEWUSER|CLONE_NEWIPC|SIGCHLD;
> +
> +    vm->pid = clone(lxcChild, stacktop, flags, (void *)vm);
> +
> +    DEBUG("clone() returned, %d", vm->pid);
> +
> +    if (vm->pid < 0) {
> +        lxcError(conn, NULL, VIR_ERR_INTERNAL_ERROR,
> +                 _("clone() failed, %s"), strerror(errno));
> +        goto error_exit;
> +    }
> +
> +    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 ?

> +    rc = 0;
> +
> +error_exit:
> +    return rc;
> +}
[...]
> +static int lxcVmStart(virConnectPtr conn,
> +                      lxc_driver_t * driver,
> +                      lxc_vm_t * vm)
> +{
> +    int rc = -1;
> +    lxc_vm_def_t *vmDef = vm->def;
> +
> +    /* open tty for the container */
> +    if(lxcSetupTtyTunnel(conn, vmDef, &vm->parentTty) < 0) {
> +        goto cleanup;
> +    }
> +
> +    rc = lxcStartContainer(conn, driver, vm);
> +
> +    if(rc == 0) {
> +        vm->state = VIR_DOMAIN_RUNNING;
> +        driver->ninactivevms--;
> +        driver->nactivevms++;
> +    }
> +
> +cleanup:
> +    return rc;
> +}
[...]

  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

[...]
>  
>  static int lxcStartup(void)
>  {
> @@ -459,7 +666,7 @@
>      NULL, /* getCapabilities */
>      lxcListDomains, /* listDomains */
>      lxcNumDomains, /* numOfDomains */
> -    NULL/*lxcDomainCreateLinux*/, /* domainCreateLinux */
> +    lxcDomainCreateAndStart, /* domainCreateLinux */
>      lxcDomainLookupByID, /* domainLookupByID */
>      lxcDomainLookupByUUID, /* domainLookupByUUID */
>      lxcDomainLookupByName, /* domainLookupByName */
> @@ -483,7 +690,7 @@
>      lxcDomainDumpXML, /* domainDumpXML */
>      lxcListDefinedDomains, /* listDefinedDomains */
>      lxcNumDefinedDomains, /* numOfDefinedDomains */
> -    NULL, /* domainCreate */
> +    lxcDomainStart, /* domainCreate */
>      lxcDomainDefine, /* domainDefineXML */
>      lxcDomainUndefine, /* domainUndefine */
>      NULL, /* domainAttachDevice */

  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.

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/


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