Re: [libvirt] [PATCH v3 08/12] LXC: controller: change the owner of tty devices to the root user of container

On 05/23/2013 01:52 PM, Richard RW. Weinberger wrote: > Hi! > > ----- Ursprüngliche Mail ----- >> Since these tty devices will be used by container, >> the owner of them should be the root user of container. >> >> Signed-off-by: Gao feng <gaofeng(a)cn.fujitsu.com&gt; >> --- >> src/lxc/lxc_controller.c | 43 >> +++++++++++++++++++++++++++++++++++++------ >> 1 file changed, 37 insertions(+), 6 deletions(-) >> >> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c >> index 7d10660..4660f25 100644 >> --- a/src/lxc/lxc_controller.c >> +++ b/src/lxc/lxc_controller.c >> @@ -1380,13 +1380,14 @@ static int >> lxcSetPersonality(virDomainDefPtr >> def) >> * *TTYNAME. Heavily borrowed from glibc, but doesn't require >> that >> * devpts == "/dev/pts" */ >> static int >> -lxcCreateTty(char *ptmx, int *ttymaster, char **ttyName) >> +lxcCreateTty(virLXCControllerPtr ctrl, int *ttymaster, >> + char **ttyName, char **ttyHostPath) >> { >> int ret = -1; >> int ptyno; >> int unlock = 0; >> >> - if ((*ttymaster = open(ptmx, O_RDWR|O_NOCTTY|O_NONBLOCK)) < >> 0) >> + if ((*ttymaster = open(ctrl->devptmx, >> O_RDWR|O_NOCTTY|O_NONBLOCK)) < 0) >> goto cleanup; >> >> if (ioctl(*ttymaster, TIOCSPTLCK, &unlock) < 0) >> @@ -1407,6 +1408,13 @@ lxcCreateTty(char *ptmx, int *ttymaster, >> char >> **ttyName) >> goto cleanup; >> } >> >> + if (virAsprintf(ttyHostPath, "/%s/%s.devpts/%d", >> LXC_STATE_DIR, >> + ctrl->def->name, ptyno) < 0) { >> + virReportOOMError(); >> + errno = ENOMEM; >> + goto cleanup; >> + } >> + >> ret = 0; >> >> cleanup: >> @@ -1552,18 +1560,41 @@ >> virLXCControllerSetupConsoles(virLXCControllerPtr ctrl, >> char **containerTTYPaths) >> { >> size_t i; >> + int ret = -1; >> + uid_t uid = (uid_t)-1; >> + gid_t gid = (gid_t)-1; >> + char *ttyHostPath = NULL; >> + >> + if (ctrl->def->idmap.uidmap) { >> + uid = ctrl->def->idmap.uidmap[0].target; >> + gid = ctrl->def->idmap.gidmap[0].target; >> + } >> >> for (i = 0; i < ctrl->nconsoles; i++) { >> VIR_DEBUG("Opening tty on private %s", ctrl->devptmx); >> - if (lxcCreateTty(ctrl->devptmx, >> + if (lxcCreateTty(ctrl, >> &ctrl->consoles[i].contFd, >> - &containerTTYPaths[i]) < 0) { >> + &containerTTYPaths[i], &ttyHostPath) < >> 0) { >> virReportSystemError(errno, "%s", >> _("Failed to allocate >> tty")); >> - return -1; >> + goto out; >> } >> + >> + /* Change the owner of tty device to the root user of >> container */ >> + if (chown(ttyHostPath, uid, gid) < 0) { >> + virReportSystemError(errno, >> + _("Failed to change owner of >> tty" >> + " %s to %u:%u"), >> + ttyHostPath, uid, gid); >> + goto out; >> + } >> + VIR_FREE(ttyHostPath); > > Why do you free ttyHostPath here? > You already do it in the exit path. > It has a cycle here, we need free the ttyHostPath since we allocate it in lxcCreateTty every cycle. >> } >> - return 0; >> + >> + ret = 0; >> +out: >> + VIR_FREE(ttyHostPath); > > Double free? > Don't worry about it, VIR_FREE does some extra jobs for us. ;)