[libvirt] [PATCH 1/2] Fix occasional container creation failure due to misuse of grantpt
glibc's grantpt and ptsname cannot be used on a fd for a pty not in /dev/pts. The lxc controller tries to do just that. So if you try to start a container on a system where /dev/pts/0 is not available, it will fail. You can make this happen by opening a terminal on /dev/pts/0, and doing 'sleep 2h & disown; exit'. To fix this, I call the virFileOpenTtyAt() from a forked task in a new mount ns, and first mount the container's /dev/pts onto /dev/pts. (Then the opened fd must be passed back to the lxc driver). Another solution would be to just do it all by hand without grantpt and ptsname. Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/863629 Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com> --- src/lxc/lxc_controller.c | 117 ++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 112 insertions(+), 5 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 51488e7..1a56e0c 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -780,6 +780,113 @@ static int lxcSetPersonality(virDomainDefPtr def) # define MS_SLAVE (1<<19) #endif +static int send_pty(int sock, int *pty) +{ + struct iovec vector; + struct msghdr msg; + struct cmsghdr * cmsg; + int ret; + + vector.iov_base = "PTY"; + vector.iov_len = 3; + + msg.msg_name = NULL; + msg.msg_namelen = 0; + msg.msg_iov = &vector; + msg.msg_iovlen = 1; + + cmsg = alloca(sizeof(struct cmsghdr) + sizeof(*pty)); + cmsg->cmsg_len = sizeof(struct cmsghdr) + sizeof(*pty); + cmsg->cmsg_level = SOL_SOCKET; + cmsg->cmsg_type = SCM_RIGHTS; + + memcpy(CMSG_DATA(cmsg), pty, sizeof(*pty)); + + msg.msg_control = cmsg; + msg.msg_controllen = cmsg->cmsg_len; + + ret = sendmsg(sock, &msg, 0); + if (ret < 0) + return -1; + return 0; +} + +static int recv_pty(int sock, int *pty, char **path, char *devpts) +{ + char buf[50]; + struct iovec vector; + struct msghdr msg; + struct cmsghdr * cmsg; + int ret; + + vector.iov_base = buf; + vector.iov_len = 50; + + msg.msg_name = NULL; + msg.msg_namelen = 0; + msg.msg_iov = &vector; + msg.msg_iovlen = 1; + + cmsg = alloca(sizeof(struct cmsghdr) + sizeof(*pty)); + cmsg->cmsg_len = sizeof(struct cmsghdr) + sizeof(*pty); + msg.msg_control = cmsg; + msg.msg_controllen = cmsg->cmsg_len; + + ret = recvmsg(sock, &msg, 0); + if (ret < 0) + return ret; + + memcpy(pty, CMSG_DATA(cmsg), sizeof(*pty)); + + if (VIR_ALLOC_N(*path, PATH_MAX) < 0) { + virReportSystemError(errno, "%s", + _("Failed to allocate space for ptyname")); + return -ENOMEM; + } + //snprintf(*path, PATH_MAX, "%s/0", devpts); + snprintf(*path, PATH_MAX, "/dev/pts/0"); + return 0; +} + +static int private_open_tty_at(char *devpts, char *devptmx, + int *containerPty, + char **containerPtyPath, int rawmode) +{ + int pid; + int ret; + int status; + int s[2]; + + ret = socketpair(PF_UNIX, SOCK_DGRAM, 0, s); + if (ret < 0) + return ret; + + pid = fork(); + if (pid < 0) + exit(pid); + if (pid == 0) { + close(s[1]); + ret = unshare(CLONE_NEWNS); + if (ret < 0) + exit(ret); + ret = mount(devpts, "/dev/pts", "none", MS_BIND, NULL); + if (ret < 0) + exit(ret); + ret = virFileOpenTtyAt(devptmx, containerPty, containerPtyPath, rawmode); + if (ret < 0) + exit(ret); + send_pty(s[0], containerPty); + exit(ret); + } + close(s[0]); + ret = recv_pty(s[1], containerPty, containerPtyPath, devpts); + close(s[1]); + if (ret) + return ret; + waitpid(pid, &status, 0); + return WEXITSTATUS(status); +} + static int lxcControllerRun(virDomainDefPtr def, unsigned int nveths, @@ -894,12 +1001,12 @@ lxcControllerRun(virDomainDefPtr def, if (devptmx) { VIR_DEBUG("Opening tty on private %s", devptmx); - if (virFileOpenTtyAt(devptmx, - &containerPty, - &containerPtyPath, - 0) < 0) { + if (private_open_tty_at(devpts, devptmx, + &containerPty, + &containerPtyPath, + 0) < 0) { virReportSystemError(errno, "%s", - _("Failed to allocate tty")); + _("Failed to allocate tty")); goto cleanup; } } else { -- 1.7.5.4
s/Mouting/Mounting. Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com> --- src/lxc/lxc_controller.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 1a56e0c..6557c07 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -984,7 +984,7 @@ lxcControllerRun(virDomainDefPtr def, goto cleanup; } - VIR_DEBUG("Mouting 'devpts' on %s", devpts); + VIR_DEBUG("Mounting 'devpts' on %s", devpts); if (mount("devpts", devpts, "devpts", 0, "newinstance,ptmxmode=0666,mode=0620,gid=5") < 0) { virReportSystemError(errno, -- 1.7.5.4
On Wed, Oct 12, 2011 at 09:32:03PM -0500, Serge E. Hallyn wrote:
s/Mouting/Mounting.
Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com> --- src/lxc/lxc_controller.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 1a56e0c..6557c07 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -984,7 +984,7 @@ lxcControllerRun(virDomainDefPtr def, goto cleanup; }
- VIR_DEBUG("Mouting 'devpts' on %s", devpts); + VIR_DEBUG("Mounting 'devpts' on %s", devpts); if (mount("devpts", devpts, "devpts", 0, "newinstance,ptmxmode=0666,mode=0620,gid=5") < 0) { virReportSystemError(errno, --
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Subject line: s/type/typo/ On 10/13/2011 03:07 AM, Daniel P. Berrange wrote:
On Wed, Oct 12, 2011 at 09:32:03PM -0500, Serge E. Hallyn wrote:
s/Mouting/Mounting.
Signed-off-by: Serge Hallyn<serge.hallyn@canonical.com> --- src/lxc/lxc_controller.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 1a56e0c..6557c07 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -984,7 +984,7 @@ lxcControllerRun(virDomainDefPtr def, goto cleanup; }
- VIR_DEBUG("Mouting 'devpts' on %s", devpts); + VIR_DEBUG("Mounting 'devpts' on %s", devpts); if (mount("devpts", devpts, "devpts", 0, "newinstance,ptmxmode=0666,mode=0620,gid=5")< 0) { virReportSystemError(errno, --
ACK
Pushed. I'll ask on the gnulib list what to do about the grantpt issue. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Oct 12, 2011 at 09:31:28PM -0500, Serge E. Hallyn wrote:
glibc's grantpt and ptsname cannot be used on a fd for a pty not in /dev/pts. The lxc controller tries to do just that. So if you try to start a container on a system where /dev/pts/0 is not available, it will fail. You can make this happen by opening a terminal on /dev/pts/0, and doing 'sleep 2h & disown; exit'. To fix this, I call the virFileOpenTtyAt() from a forked task in a new mount ns, and first mount the container's /dev/pts onto /dev/pts. (Then the opened fd must be passed back to the lxc driver). Another solution would be to just do it all by hand without grantpt and ptsname.
GNULIB already has a 'grantpt' implementation for MinGW, OpenBSD and OS-X. So I wonder if they would like to fix the broken Linux glibc version too ? Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/12/2011 08:31 PM, Serge E. Hallyn wrote:
glibc's grantpt and ptsname cannot be used on a fd for a pty not in /dev/pts. The lxc controller tries to do just that. So if you try to start a container on a system where /dev/pts/0 is not available, it will fail. You can make this happen by opening a terminal on /dev/pts/0, and doing 'sleep 2h& disown; exit'. To fix this, I call the virFileOpenTtyAt() from a forked task in a new mount ns, and first mount the container's /dev/pts onto /dev/pts. (Then the opened fd must be passed back to the lxc driver). Another solution would be to just do it all by hand without grantpt and ptsname.
Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/863629
Signed-off-by: Serge Hallyn<serge.hallyn@canonical.com> --- src/lxc/lxc_controller.c | 117 ++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 112 insertions(+), 5 deletions(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 51488e7..1a56e0c 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -780,6 +780,113 @@ static int lxcSetPersonality(virDomainDefPtr def) # define MS_SLAVE (1<<19) #endif
+static int send_pty(int sock, int *pty) +{ + struct iovec vector; + struct msghdr msg; + struct cmsghdr * cmsg; + int ret; +
Yuck. Why not just use gnulib's sendfd/recvfd interfaces, and greatly shrink the size of this patch? We're already using those functions elsewhere, for much more compact fd passing.
+ if (VIR_ALLOC_N(*path, PATH_MAX)< 0) { + virReportSystemError(errno, "%s", + _("Failed to allocate space for ptyname")); + return -ENOMEM; + } + //snprintf(*path, PATH_MAX, "%s/0", devpts);
Also, looks like you left some debug stuff behind. Have you filed a bug against glibc's grantpt? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 10/13/2011 09:47 AM, Eric Blake wrote:
On 10/12/2011 08:31 PM, Serge E. Hallyn wrote:
glibc's grantpt and ptsname cannot be used on a fd for a pty not in /dev/pts. The lxc controller tries to do just that. So if you try to start a container on a system where /dev/pts/0 is not available, it will fail. You can make this happen by opening a terminal on /dev/pts/0, and doing 'sleep 2h& disown; exit'. To fix this, I call the virFileOpenTtyAt() from a forked task in a new mount ns, and first mount the container's /dev/pts onto /dev/pts. (Then the opened fd must be passed back to the lxc driver). Another solution would be to just do it all by hand without grantpt and ptsname.
Have you filed a bug against glibc's grantpt?
I should have read your URL, which points to http://www.cygwin.com/ml/libc-alpha/2011-10/msg00008.html. My conclusions from that thread are: We are outside the bounds of POSIX the moment we create a private pts mount point not located at /dev/pts. Gnulib can't fix the situation for us (gnulib grantpt is only useful in the context of an application trying to comply with existing POSIX rules, and cloning/unsharing namespaces to create a private pts is not in scope for gnulib). So I don't see that anything is worth reporting to gnulib after all. Now, after reading more of the code, I see that src/util/util.c exports virFileOpenTty(), which is hard-coded to opening /dev/ptmx (when it should _really_ be using posix_openpt to be portable to all platforms). It also provides (but we forgot to export in libvirt_private.syms) virFileOpenTtyAt, which is Linux-specific in trying to accommodate alternate mount points for the pts system. Currently, the only clients for these two functions is lxc; although virFileOpenTty() might be useful to other clients that aren't doing Linux-specific container games with the file system. So the problem at hand is that as soon as you have a private pts, glibc doesn't interact well with it. That is, virFileOpenTtyAt() is worthless in that situation. Serge's patch provides a replacement for _just_ the private pts situation, which uses glibc by forking and bind-mounting the private pts back into the glibc expectations of /dev/pts, then passing that fd back to the parent; the alternative solution would be to ditch glibc interfaces and do the raw ioctl calls on the master pty ourselves. Since lxc is already Linux-specific, I think that I would favor this approach as being simpler (that is, completely ditch virFileOpenTtyAt, and teach lxc to just make the raw ioctl calls instead of using grantpt and unlockpt, all without forking or passing fds across a socket). Serge, is this something you can attempt, or should I look into it more? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Quoting Eric Blake (eblake@redhat.com): ...
of /dev/pts, then passing that fd back to the parent; the alternative solution would be to ditch glibc interfaces and do the raw ioctl calls on the master pty ourselves. Since lxc is already Linux-specific, I think that I would favor this approach as being simpler (that is, completely ditch virFileOpenTtyAt, and teach lxc to just make the raw ioctl calls instead of using grantpt and unlockpt, all without forking or passing fds across a socket).
Serge, is this something you can attempt, or should I look into it more?
Hey Eric, yup, if it's acceptable to you I think that's the better, simpler approach. I'll send out a new patch. thanks, -serge
The glibc ones cannot handle ptys opened in a devpts not mounted at /dev/pts. Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com> --- src/lxc/lxc_controller.c | 82 +++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 78 insertions(+), 4 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 89ce7f5..d79f5ba 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -41,6 +41,8 @@ #include <locale.h> #include <linux/loop.h> #include <dirent.h> +#include <grp.h> +#include <sys/stat.h> #if HAVE_CAPNG # include <cap-ng.h> @@ -781,6 +783,81 @@ static int lxcSetPersonality(virDomainDefPtr def) #endif static int +lxcGetTtyGid(void) { + char *grtmpbuf; + struct group grbuf; + size_t grbuflen = sysconf(_SC_GETGR_R_SIZE_MAX); + struct group *p; + int ret; + gid_t tty_gid = -1; + + /* Get the group ID of the special `tty' group. */ + if (grbuflen == (size_t) -1L) + /* `sysconf' does not support _SC_GETGR_R_SIZE_MAX. + Try a moderate value. */ + grbuflen = 1024; + + grtmpbuf = (char *) alloca (grbuflen); + ret = getgrnam_r("tty", &grbuf, grtmpbuf, grbuflen, &p); + if (ret || p != NULL) + tty_gid = p->gr_gid; + + return tty_gid == -1 ? getgid() : tty_gid; +} + +#define ACCESSPERMS (S_IRWXU|S_IRWXG|S_IRWXO) /* 0777 */ + +/* heavily borrowed from glibc, but don't assume devpts == "/dev/pts" */ +static int +lxcCreateTty(char *ptmx, int *ttymaster, char **ttyName) +{ + int rc = -1; + int ret; + int ptyno; + uid_t uid; + gid_t gid; + struct stat st; + int unlock = 0; + + if ((*ttymaster = open(ptmx, O_RDWR|O_NOCTTY|O_NONBLOCK)) < 0) + goto cleanup; + + if (ioctl(*ttymaster, TIOCSPTLCK, &unlock) < 0) + goto cleanup; + + ret = ioctl(*ttymaster, TIOCGPTN, &ptyno); + if (ret < 0) + goto cleanup; + + ret = fstat(*ttymaster, &st); + + uid = getuid(); + gid = lxcGetTtyGid(); + if (st.st_uid != uid || st.st_gid != gid) + if (fchown(*ttymaster, uid, gid) < 0) + goto cleanup; + + if ((st.st_mode & ACCESSPERMS) != (S_IRUSR|S_IWUSR|S_IWGRP)) + if (fchmod(*ttymaster, S_IRUSR|S_IWUSR|S_IWGRP) < 0) + goto cleanup; + + if (VIR_ALLOC_N(*ttyName, PATH_MAX) < 0) { + errno = ENOMEM; + goto cleanup; + } + + snprintf(*ttyName, PATH_MAX, "/dev/pts/%d", ptyno); + + rc = 0; + +cleanup: + if (rc != 0) + VIR_FORCE_CLOSE(*ttymaster); + + return rc; +} + +static int lxcControllerRun(virDomainDefPtr def, unsigned int nveths, char **veths, @@ -894,10 +971,7 @@ lxcControllerRun(virDomainDefPtr def, if (devptmx) { VIR_DEBUG("Opening tty on private %s", devptmx); - if (virFileOpenTtyAt(devptmx, - &containerPty, - &containerPtyPath, - 0) < 0) { + if (lxcCreateTty(devptmx, &containerPty, &containerPtyPath) < 0) { virReportSystemError(errno, "%s", _("Failed to allocate tty")); goto cleanup; -- 1.7.5.4
hi Serge : I checked the code , only in lxc_controller.c call virFileOpenTtyAt(). I didn't test the path, but my suggestion is that modify the utility function in /src/util/util.c instead of adding a new function.
The glibc ones cannot handle ptys opened in a devpts not mounted at /dev/pts.
Signed-off-by: Serge Hallyn<serge.hallyn@canonical.com> --- src/lxc/lxc_controller.c | 82 +++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 78 insertions(+), 4 deletions(-)
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 89ce7f5..d79f5ba 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -41,6 +41,8 @@ #include<locale.h> #include<linux/loop.h> #include<dirent.h> +#include<grp.h> +#include<sys/stat.h>
#if HAVE_CAPNG # include<cap-ng.h> @@ -781,6 +783,81 @@ static int lxcSetPersonality(virDomainDefPtr def) #endif
static int +lxcGetTtyGid(void) { + char *grtmpbuf; + struct group grbuf; + size_t grbuflen = sysconf(_SC_GETGR_R_SIZE_MAX); + struct group *p; + int ret; + gid_t tty_gid = -1; + + /* Get the group ID of the special `tty' group. */ + if (grbuflen == (size_t) -1L) + /* `sysconf' does not support _SC_GETGR_R_SIZE_MAX. + Try a moderate value. */ + grbuflen = 1024; + + grtmpbuf = (char *) alloca (grbuflen); use this macro VIR_ALLOC_N(ptr, count) to allocate memory and you should also check if it is valid. + ret = getgrnam_r("tty",&grbuf, grtmpbuf, grbuflen,&p); + if (ret || p != NULL) + tty_gid = p->gr_gid; + + return tty_gid == -1 ? getgid() : tty_gid; +} + +#define ACCESSPERMS (S_IRWXU|S_IRWXG|S_IRWXO) /* 0777 */ + +/* heavily borrowed from glibc, but don't assume devpts == "/dev/pts" */ +static int +lxcCreateTty(char *ptmx, int *ttymaster, char **ttyName) +{ + int rc = -1; + int ret; + int ptyno; + uid_t uid; + gid_t gid; + struct stat st; + int unlock = 0; + + if ((*ttymaster = open(ptmx, O_RDWR|O_NOCTTY|O_NONBLOCK))< 0) + goto cleanup; + + if (ioctl(*ttymaster, TIOCSPTLCK,&unlock)< 0) + goto cleanup; + + ret = ioctl(*ttymaster, TIOCGPTN,&ptyno); + if (ret< 0) + goto cleanup; if (ioctl(*ttymaster, TIOCGPTN, &ptyno) < 0) goto clearup; + + ret = fstat(*ttymaster,&st); + + uid = getuid(); + gid = lxcGetTtyGid(); + if (st.st_uid != uid || st.st_gid != gid) + if (fchown(*ttymaster, uid, gid)< 0) + goto cleanup; + + if ((st.st_mode& ACCESSPERMS) != (S_IRUSR|S_IWUSR|S_IWGRP)) + if (fchmod(*ttymaster, S_IRUSR|S_IWUSR|S_IWGRP)< 0) + goto cleanup; + + if (VIR_ALLOC_N(*ttyName, PATH_MAX)< 0) { + errno = ENOMEM; + goto cleanup; + } + + snprintf(*ttyName, PATH_MAX, "/dev/pts/%d", ptyno); + + rc = 0; + +cleanup: + if (rc != 0) + VIR_FORCE_CLOSE(*ttymaster); + + return rc; +} + +static int lxcControllerRun(virDomainDefPtr def, unsigned int nveths, char **veths, @@ -894,10 +971,7 @@ lxcControllerRun(virDomainDefPtr def,
if (devptmx) { VIR_DEBUG("Opening tty on private %s", devptmx); - if (virFileOpenTtyAt(devptmx, -&containerPty, -&containerPtyPath, - 0)< 0) { + if (lxcCreateTty(devptmx,&containerPty,&containerPtyPath)< 0) { virReportSystemError(errno, "%s", _("Failed to allocate tty")); goto cleanup; -- 1.7.5.4 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- best regards eli
Quoting Eli Qiao (taget@linux.vnet.ibm.com):
hi Serge :
Thanks for taking a look.
I checked the code , only in lxc_controller.c call virFileOpenTtyAt(). I didn't test the path, but my suggestion is that modify the utility function in /src/util/util.c instead of adding a new function.
But virFileOpenTtyAt() is called by virFileOpenTty() in the same file. So we really do want a new function using its own versions of grantpt+unlockpt, because I think that, when we can, we want to continue using the glibc versions. So the safe approach seemed to me to be: put the container-specific code into src/lxc/lxc_controller.c, then (in a separate patch) just fold virFileOpenTtyAt(), straight into virFileOpenTty(). If you agree, I'll post a new version incorporating your other feedback, especially the garish use of alloca :) (If you disagree, please feel free to send your own patch - I'm in no way attached to having my version included, I mainly wanted to point out the bug) thanks, -serge
On 10/17/2011 07:26 AM, Serge E. Hallyn wrote:
Quoting Eli Qiao (taget@linux.vnet.ibm.com):
hi Serge :
Thanks for taking a look.
I checked the code , only in lxc_controller.c call virFileOpenTtyAt(). I didn't test the path, but my suggestion is that modify the utility function in /src/util/util.c instead of adding a new function.
But virFileOpenTtyAt() is called by virFileOpenTty() in the same file. So we really do want a new function using its own versions of grantpt+unlockpt, because I think that, when we can, we want to continue using the glibc versions.
So the safe approach seemed to me to be: put the container-specific code into src/lxc/lxc_controller.c, then (in a separate patch) just fold virFileOpenTtyAt(), straight into virFileOpenTty().
Correct - my intent was that we have: src/util/util.c: virFileOpenTty() - generally useful by any driver, uses glibc, and also should use posix_openpt rather than than open("/dev/ptmx") for portability (gnulib provides posix_openpt). Completely ditch virFileOpenTtyAt() by inlining it back to virFileOpenTty(), and since no one could use what virFileOpenTtyAt() claimed to provide except in the portable case where virFileOpenTty() was sufficient. src/lxc/lxc_controller.c - LXC-specific function that uses ioctl() instead of glibc for opening a private tty. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
The glibc ones cannot handle ptys opened in a devpts not mounted at /dev/pts. Changelog: Oct 17: Per Eli Qiao, use VIR_ALLOC_N when appropriate, make sure to check return values, and follow coding style convention. Change lxcGetTtyGid() to return -1 on error, otherwise return gid in passed-in argument. Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com> --- src/lxc/lxc_controller.c | 89 +++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 85 insertions(+), 4 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 89ce7f5..8c9caee 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -41,6 +41,8 @@ #include <locale.h> #include <linux/loop.h> #include <dirent.h> +#include <grp.h> +#include <sys/stat.h> #if HAVE_CAPNG # include <cap-ng.h> @@ -781,6 +783,88 @@ static int lxcSetPersonality(virDomainDefPtr def) #endif static int +lxcGetTtyGid(int *gid) { + char *grtmpbuf; + struct group grbuf; + size_t grbuflen = sysconf(_SC_GETGR_R_SIZE_MAX); + struct group *p; + int ret; + gid_t tty_gid = -1; + + /* Get the group ID of the special `tty' group. */ + if (grbuflen == (size_t) -1L) + /* `sysconf' does not support _SC_GETGR_R_SIZE_MAX. + Try a moderate value. */ + grbuflen = 1024; + + if ((VIR_ALLOC_N(grtmpbuf, grbuflen)) < 0) + return -1; + + ret = getgrnam_r("tty", &grbuf, grtmpbuf, grbuflen, &p); + if (ret || p != NULL) + tty_gid = p->gr_gid; + + VIR_FREE(grtmpbuf); + + *gid = tty_gid == -1 ? getgid() : tty_gid; + return 0; +} + +#define ACCESSPERMS (S_IRWXU|S_IRWXG|S_IRWXO) /* 0777 */ + +/* heavily borrowed from glibc, but don't assume devpts == "/dev/pts" */ +static int +lxcCreateTty(char *ptmx, int *ttymaster, char **ttyName) +{ + int rc = -1; + int ret; + int ptyno; + uid_t uid; + gid_t gid; + struct stat st; + int unlock = 0; + + if ((*ttymaster = open(ptmx, O_RDWR|O_NOCTTY|O_NONBLOCK)) < 0) + goto cleanup; + + if (ioctl(*ttymaster, TIOCSPTLCK, &unlock) < 0) + goto cleanup; + + if (ioctl(*ttymaster, TIOCGPTN, &ptyno) < 0) + goto cleanup; + + if (fstat(*ttymaster, &st) < 0) + goto cleanup; + + if (lxcGetTtyGid(&gid) < 0) + goto cleanup; + + uid = getuid(); + if (st.st_uid != uid || st.st_gid != gid) + if (fchown(*ttymaster, uid, gid) < 0) + goto cleanup; + + if ((st.st_mode & ACCESSPERMS) != (S_IRUSR|S_IWUSR|S_IWGRP)) + if (fchmod(*ttymaster, S_IRUSR|S_IWUSR|S_IWGRP) < 0) + goto cleanup; + + if (VIR_ALLOC_N(*ttyName, PATH_MAX) < 0) { + errno = ENOMEM; + goto cleanup; + } + + snprintf(*ttyName, PATH_MAX, "/dev/pts/%d", ptyno); + + rc = 0; + +cleanup: + if (rc != 0) + VIR_FORCE_CLOSE(*ttymaster); + + return rc; +} + +static int lxcControllerRun(virDomainDefPtr def, unsigned int nveths, char **veths, @@ -894,10 +978,7 @@ lxcControllerRun(virDomainDefPtr def, if (devptmx) { VIR_DEBUG("Opening tty on private %s", devptmx); - if (virFileOpenTtyAt(devptmx, - &containerPty, - &containerPtyPath, - 0) < 0) { + if (lxcCreateTty(devptmx, &containerPty, &containerPtyPath) < 0) { virReportSystemError(errno, "%s", _("Failed to allocate tty")); goto cleanup; -- 1.7.5.4
On 10/17/2011 01:04 PM, Serge E. Hallyn wrote:
The glibc ones cannot handle ptys opened in a devpts not mounted at /dev/pts.
Changelog: Oct 17: Per Eli Qiao, use VIR_ALLOC_N when appropriate, make sure to check return values, and follow coding style convention. Change lxcGetTtyGid() to return -1 on error, otherwise return gid in passed-in argument.
Signed-off-by: Serge Hallyn<serge.hallyn@canonical.com> --- src/lxc/lxc_controller.c | 89 +++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 85 insertions(+), 4 deletions(-)
@@ -781,6 +783,88 @@ static int lxcSetPersonality(virDomainDefPtr def) #endif
static int +lxcGetTtyGid(int *gid) { + char *grtmpbuf; + struct group grbuf; + size_t grbuflen = sysconf(_SC_GETGR_R_SIZE_MAX); + struct group *p; + int ret; + gid_t tty_gid = -1;
Hmm. This gets called once per lxc guest, instead of once per libvirtd process, even though the gid won't change in the meantime. Furthermore, we have _already_ hardcoded this to 5, based on the options we gave to mount("devpts") - either we need to fix that mount call (better), or we can skip this function altogether (assuming that our hard-coding of 5 is correct, there's no need to requery it, although that does sound like a worse solution). For that matter, the whole point of the mount("devpts",...",gid=5") designation is that the new pty will be owned by gid 5, without needing to fchown() it. Are there kernels that are new enough to support newinstance mounting, yet old enough to not honor gid=5? That would be the only case where we would have to chown in the first place. But if all kernels new enough to support newinstance mounting correctly honor the gid=5 option, then we don't even have to do chown() calls [but we still have to fix the hard-coding of gid=5 in the mount() option].
+ if (fstat(*ttymaster,&st)< 0) + goto cleanup; + + if (lxcGetTtyGid(&gid)< 0) + goto cleanup; + + uid = getuid(); + if (st.st_uid != uid || st.st_gid != gid) + if (fchown(*ttymaster, uid, gid)< 0) + goto cleanup; + + if ((st.st_mode& ACCESSPERMS) != (S_IRUSR|S_IWUSR|S_IWGRP)) + if (fchmod(*ttymaster, S_IRUSR|S_IWUSR|S_IWGRP)< 0) + goto cleanup;
Likewise, I think this fchmod() is useless; the mode=0620 in the mount option should have already set this up.
+ + if (VIR_ALLOC_N(*ttyName, PATH_MAX)< 0) { + errno = ENOMEM; + goto cleanup; + }
Wasteful. PATH_MAX is MUCH bigger than we need.
+ + snprintf(*ttyName, PATH_MAX, "/dev/pts/%d", ptyno);
Instead, I'd just do this as: virAsprintf(ttyName, "/dev/pts/%d", ptyno); Also, do we want this to be the name of the pty, _as seen from the guest after the fs pivot is complete_, or do we want this to be the name of the pty, as seen from the host prior to the pivot, in which case it would be some derivative of "%s/dev/pts/%d", root->src, ptyno? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Quoting Eric Blake (eblake@redhat.com):
On 10/17/2011 01:04 PM, Serge E. Hallyn wrote:
The glibc ones cannot handle ptys opened in a devpts not mounted at /dev/pts.
Changelog: Oct 17: Per Eli Qiao, use VIR_ALLOC_N when appropriate, make sure to check return values, and follow coding style convention. Change lxcGetTtyGid() to return -1 on error, otherwise return gid in passed-in argument.
Signed-off-by: Serge Hallyn<serge.hallyn@canonical.com> --- src/lxc/lxc_controller.c | 89 +++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 85 insertions(+), 4 deletions(-)
@@ -781,6 +783,88 @@ static int lxcSetPersonality(virDomainDefPtr def) #endif
static int +lxcGetTtyGid(int *gid) { + char *grtmpbuf; + struct group grbuf; + size_t grbuflen = sysconf(_SC_GETGR_R_SIZE_MAX); + struct group *p; + int ret; + gid_t tty_gid = -1;
Hmm. This gets called once per lxc guest, instead of once per libvirtd process, even though the gid won't change in the meantime.
Furthermore, we have _already_ hardcoded this to 5, based on the options we gave to mount("devpts") - either we need to fix that mount call (better), or we can skip this function altogether (assuming that our hard-coding of 5 is correct, there's no need to requery it, although that does sound like a worse solution). For that matter, the whole point of the mount("devpts",...",gid=5") designation is that the new pty will be owned by gid 5, without needing to fchown() it. Are there kernels that are new enough to support newinstance mounting, yet old enough to not honor gid=5?
No. Mode and gid precede multiple devpts instances.
That would be the only case where we would have to chown in the first place. But if all kernels new enough to support newinstance mounting correctly honor the gid=5 option, then we don't even have to do chown() calls [but we still have to fix the hard-coding of gid=5 in the mount() option].
I missed something - why do we have to fix that? It seems to me you're right - this is a linux-specific fn and we are setting gid to 5 and the mode, we can skip this whole lxcGetTtyGid function and the corresponding fchown and fchmod calls. Nice!
+ if (fstat(*ttymaster,&st)< 0) + goto cleanup; + + if (lxcGetTtyGid(&gid)< 0) + goto cleanup; + + uid = getuid(); + if (st.st_uid != uid || st.st_gid != gid) + if (fchown(*ttymaster, uid, gid)< 0) + goto cleanup; + + if ((st.st_mode& ACCESSPERMS) != (S_IRUSR|S_IWUSR|S_IWGRP)) + if (fchmod(*ttymaster, S_IRUSR|S_IWUSR|S_IWGRP)< 0) + goto cleanup;
Likewise, I think this fchmod() is useless; the mode=0620 in the mount option should have already set this up.
Yup.
+ + if (VIR_ALLOC_N(*ttyName, PATH_MAX)< 0) { + errno = ENOMEM; + goto cleanup; + }
Wasteful. PATH_MAX is MUCH bigger than we need.
I thought so, but it was being done that way before, and didn't seem all that important.
+ + snprintf(*ttyName, PATH_MAX, "/dev/pts/%d", ptyno);
Instead, I'd just do this as:
virAsprintf(ttyName, "/dev/pts/%d", ptyno);
Where virAsprintf will allocate when ttyName starts as null?
Also, do we want this to be the name of the pty, _as seen from the guest after the fs pivot is complete_, or do we want this to be the name of the pty, as seen from the host prior to the pivot, in which case it would be some derivative of "%s/dev/pts/%d", root->src, ptyno?
This gets passed into lxc_container which then prepends the chroot location. Don't know if there is any reason why we can't just set it to the full name here, haven't looked further. -serge
On 10/17/2011 03:29 PM, Serge E. Hallyn wrote:
that matter, the whole point of the mount("devpts",...",gid=5") designation is that the new pty will be owned by gid 5, without needing to fchown() it. Are there kernels that are new enough to support newinstance mounting, yet old enough to not honor gid=5?
No. Mode and gid precede multiple devpts instances.
Good to hear. In other words, glibc has to worry about it when dealing with "/dev/pts", because it caters to older kernels that lacked multiple devpts, but lxc can take shortcuts based on guarantees present by virtue of the existence of multiple instances. But definitely some comments in the code are called for, explaining why we are taking these shortcuts.
[but we still have to fix the hard-coding of gid=5 in the mount() option].
I missed something - why do we have to fix that?
We don't have to fix it now, but we should fix it someday. There's nothing that says a distro has to map 'tty' to gid 5, and while most distros do that, we should instead be portable to compilation on a distro where 'tty' is gid 6 (or some other unusual number).
Instead, I'd just do this as:
virAsprintf(ttyName, "/dev/pts/%d", ptyno);
Where virAsprintf will allocate when ttyName starts as null?
Yep - the whole point of virAsprintf is to allocate the string on your behalf, and to gracefully ensure that ttyName is NULL on allocation failure - much more compact than using snprintf yourself, and avoids the waste of a PATH_MAX allocation. And, while you are at it, what about also fixing src/util/util.c to remove virFileOpenTtyAt (now that no one calls that), by moving its body into virFileOpenTty except for using posix_openpt instead of open("/dev/ptmx"). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Quoting Eric Blake (eblake@redhat.com):
[but we still have to fix the hard-coding of gid=5 in the mount() option].
I missed something - why do we have to fix that?
We don't have to fix it now, but we should fix it someday. There's nothing that says a distro has to map 'tty' to gid 5, and while most distros do that, we should instead be portable to compilation on a distro where 'tty' is gid 6 (or some other unusual number).
But that gid should be set according to what the guest expects, right? So we actually can't do it at compilation?
Instead, I'd just do this as:
virAsprintf(ttyName, "/dev/pts/%d", ptyno);
Where virAsprintf will allocate when ttyName starts as null?
Yep - the whole point of virAsprintf is to allocate the string on your behalf, and to gracefully ensure that ttyName is NULL on allocation failure - much more compact than using snprintf yourself, and avoids the waste of a PATH_MAX allocation.
And, while you are at it, what about also fixing src/util/util.c to remove virFileOpenTtyAt (now that no one calls that), by moving its body into virFileOpenTty except for using posix_openpt instead of open("/dev/ptmx").
I was thinking that should be a separate patch for ease of review, but I'll roll it into my next patch. thanks, -serge
New version, compile-tested only tonight. I followed the suggestion about using posix_openpt(), though its manpage worries me - does libvirt need to compile on any platforms that don't have that fn? (In which case we can add the trivial define if we need to, but...) Subject: [PATCH 1/1] lxc: use our own hand-rolled code in place of unlockpt and grantpt The glibc ones cannot handle ptys opened in a devpts not mounted at /dev/pts. Changelog: Oct 17: Per Eli Qiao, use VIR_ALLOC_N when appropriate, make sure to check return values, and follow coding style convention. Change lxcGetTtyGid() to return -1 on error, otherwise return gid in passed-in argument. Oct 18: Per Eric Blake, consolidate virFileOpenTtyAt() into virFileOpenTty(), and use posix_openpt(). Per Eric Blake, don't set gid on opened pty since we ask the kernel to do so with 'gid=5' mount option. Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com> --- src/lxc/lxc_controller.c | 67 +++++++++++++++++++++++++++++++++++++++++++--- src/util/util.c | 24 +---------------- 2 files changed, 64 insertions(+), 27 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 89ce7f5..464bfe8 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -41,6 +41,8 @@ #include <locale.h> #include <linux/loop.h> #include <dirent.h> +#include <grp.h> +#include <sys/stat.h> #if HAVE_CAPNG # include <cap-ng.h> @@ -780,6 +782,62 @@ static int lxcSetPersonality(virDomainDefPtr def) # define MS_SLAVE (1<<19) #endif +#define ACCESSPERMS (S_IRWXU|S_IRWXG|S_IRWXO) /* 0777 */ + +/* heavily borrowed from glibc, but don't assume devpts == "/dev/pts" */ +static int +lxcCreateTty(char *ptmx, int *ttymaster, char **ttyName) +{ + int rc = -1; + int ret; + int ptyno; + uid_t uid; + struct stat st; + int unlock = 0; + + if ((*ttymaster = open(ptmx, O_RDWR|O_NOCTTY|O_NONBLOCK)) < 0) + goto cleanup; + + if (ioctl(*ttymaster, TIOCSPTLCK, &unlock) < 0) + goto cleanup; + + if (ioctl(*ttymaster, TIOCGPTN, &ptyno) < 0) + goto cleanup; + + if (fstat(*ttymaster, &st) < 0) + goto cleanup; + + uid = getuid(); + if (st.st_uid != uid) + if (fchown(*ttymaster, uid, -1) < 0) + goto cleanup; + + if ((st.st_mode & ACCESSPERMS) != (S_IRUSR|S_IWUSR|S_IWGRP)) + if (fchmod(*ttymaster, S_IRUSR|S_IWUSR|S_IWGRP) < 0) + goto cleanup; + + /* + * ptyno shouldn't currently be anything other than 0, but let's + * play it safe + */ + if (virAsprintf(ttyName, "/dev/pts/%d", ptyno) < 0) { + virReportOOMError(); + errno = ENOMEM; + goto cleanup; + } + + + rc = 0; + +cleanup: + if (rc != 0) { + VIR_FORCE_CLOSE(*ttymaster); + VIR_FREE(*ttyName) + } + + return rc; +} + static int lxcControllerRun(virDomainDefPtr def, unsigned int nveths, @@ -877,6 +935,10 @@ lxcControllerRun(virDomainDefPtr def, goto cleanup; } + /* + * todo - should we support gid=X for X!=5 for distros which + * use a different gid for tty? + */ VIR_DEBUG("Mounting 'devpts' on %s", devpts); if (mount("devpts", devpts, "devpts", 0, "newinstance,ptmxmode=0666,mode=0620,gid=5") < 0) { @@ -894,10 +956,7 @@ lxcControllerRun(virDomainDefPtr def, if (devptmx) { VIR_DEBUG("Opening tty on private %s", devptmx); - if (virFileOpenTtyAt(devptmx, - &containerPty, - &containerPtyPath, - 0) < 0) { + if (lxcCreateTty(devptmx, &containerPty, &containerPtyPath) < 0) { virReportSystemError(errno, "%s", _("Failed to allocate tty")); goto cleanup; diff --git a/src/util/util.c b/src/util/util.c index dac616b..1ec36f1 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1104,21 +1104,9 @@ int virFileOpenTty(int *ttymaster, char **ttyName, int rawmode) { - return virFileOpenTtyAt("/dev/ptmx", - ttymaster, - ttyName, - rawmode); -} - -#ifdef __linux__ -int virFileOpenTtyAt(const char *ptmx, - int *ttymaster, - char **ttyName, - int rawmode) -{ int rc = -1; - if ((*ttymaster = open(ptmx, O_RDWR|O_NOCTTY|O_NONBLOCK)) < 0) + if ((*ttymaster = posix_openpt(O_RDWR|O_NOCTTY|O_NONBLOCK)) < 0) goto cleanup; if (unlockpt(*ttymaster) < 0) @@ -1159,16 +1147,6 @@ cleanup: return rc; } -#else -int virFileOpenTtyAt(const char *ptmx ATTRIBUTE_UNUSED, - int *ttymaster ATTRIBUTE_UNUSED, - char **ttyName ATTRIBUTE_UNUSED, - int rawmode ATTRIBUTE_UNUSED) -{ - return -1; -} -#endif - /* * Creates an absolute path for a potentially relative path. -- 1.7.5.4
On 10/18/2011 07:39 PM, Serge E. Hallyn wrote:
New version, compile-tested only tonight. I followed the suggestion about using posix_openpt(), though its manpage worries me - does libvirt need to compile on any platforms that don't have that fn? (In which case we can add the trivial define if we need to, but...)
Libvirt compiles on mingw, which lacks posix_openpt (and in fact, lacks pseudo-terminal altogether). We trivially support functions like this by importing the corresponding gnulib modules (which either implement the workarounds, or gracefully fail with ENOSYS); I recently added posix_openpt() to gnulib, although looking at it further, I'd much rather use gnulib's openpty() function instead of the POSIX sequence of posix_openpt()/grantpt()/unlockpt()/ptsname()/open(), especially since the POSIX sequence is still insufficiently portable (on Solaris and HP-UX, it has to include additional ioctl() calls to enable STREAMS filters before isatty(slave) will return non-zero). But right now, those interfaces are LGPLv3+, so I'm waiting for Bruno to relax their license: https://lists.gnu.org/archive/html/bug-gnulib/2011-10/msg00178.html
@@ -780,6 +782,62 @@ static int lxcSetPersonality(virDomainDefPtr def) # define MS_SLAVE (1<<19) #endif
+#define ACCESSPERMS (S_IRWXU|S_IRWXG|S_IRWXO) /* 0777 */ + +/* heavily borrowed from glibc, but don't assume devpts == "/dev/pts" */ +static int +lxcCreateTty(char *ptmx, int *ttymaster, char **ttyName) +{ + int rc = -1;
We typically use ret for our returns, and rc for temporary variables, although that naming convention doesn't really matter too much.
+ int ret; + int ptyno; + uid_t uid; + struct stat st; + int unlock = 0; + + if ((*ttymaster = open(ptmx, O_RDWR|O_NOCTTY|O_NONBLOCK))< 0) + goto cleanup; + + if (ioctl(*ttymaster, TIOCSPTLCK,&unlock)< 0) + goto cleanup; + + if (ioctl(*ttymaster, TIOCGPTN,&ptyno)< 0) + goto cleanup;
So far, so good.
+ + if (fstat(*ttymaster,&st)< 0) + goto cleanup; + + uid = getuid(); + if (st.st_uid != uid) + if (fchown(*ttymaster, uid, -1)< 0) + goto cleanup;
Not required. 'man mount', under the devpts section, makes it clear that "When nothing is specified, they will be set to the UID and GID of the creating process.", which means st.st_uid will _always_ match getuid() because we just did the open() and there was no uid= mount option.
+ + if ((st.st_mode& ACCESSPERMS) != (S_IRUSR|S_IWUSR|S_IWGRP)) + if (fchmod(*ttymaster, S_IRUSR|S_IWUSR|S_IWGRP)< 0) + goto cleanup;
Not required. Again, the docs state that 'mode=0620' was sufficient to already guarantee this setting.
+ VIR_FORCE_CLOSE(*ttymaster); + VIR_FREE(*ttyName)
How did this ever pass compile-testing without that semicolon?
@@ -877,6 +935,10 @@ lxcControllerRun(virDomainDefPtr def, goto cleanup; }
+ /* + * todo - should we support gid=X for X!=5 for distros which + * use a different gid for tty? + */
No TABs. 'make syntax-check' caught this.
+++ b/src/util/util.c @@ -1104,21 +1104,9 @@ int virFileOpenTty(int *ttymaster, char **ttyName, int rawmode) { - return virFileOpenTtyAt("/dev/ptmx", - ttymaster, - ttyName, - rawmode); -} - -#ifdef __linux__ -int virFileOpenTtyAt(const char *ptmx, - int *ttymaster, - char **ttyName, - int rawmode) -{ int rc = -1;
- if ((*ttymaster = open(ptmx, O_RDWR|O_NOCTTY|O_NONBLOCK))< 0) + if ((*ttymaster = posix_openpt(O_RDWR|O_NOCTTY|O_NONBLOCK))< 0)
Looks good (better would be using openpty(), once gnulib changes that to LGPLv2+, but I'll deal with that in a later patch). So, while waiting on gnulib changes, I'll temporarily #ifdef out this code so that I don't break mingw compilation. ACK to the intent and to incorporating previous review comments. Here's what I squashed in before pushing: diff --git i/src/lxc/lxc_controller.c w/src/lxc/lxc_controller.c index 464bfe8..fad4259 100644 --- i/src/lxc/lxc_controller.c +++ w/src/lxc/lxc_controller.c @@ -784,15 +784,16 @@ static int lxcSetPersonality(virDomainDefPtr def) #define ACCESSPERMS (S_IRWXU|S_IRWXG|S_IRWXO) /* 0777 */ -/* heavily borrowed from glibc, but don't assume devpts == "/dev/pts" */ +/* Create a private tty using the private devpts at PTMX, returning + * the master in *TTYMASTER and the name of the slave, _from the + * perspective of the guest after remounting file systems_, in + * *TTYNAME. Heavily borrowed from glibc, but doesn't require that + * devpts == "/dev/pts" */ static int lxcCreateTty(char *ptmx, int *ttymaster, char **ttyName) { - int rc = -1; - int ret; + int ret = -1; int ptyno; - uid_t uid; - struct stat st; int unlock = 0; if ((*ttymaster = open(ptmx, O_RDWR|O_NOCTTY|O_NONBLOCK)) < 0) @@ -804,38 +805,27 @@ lxcCreateTty(char *ptmx, int *ttymaster, char **ttyName) if (ioctl(*ttymaster, TIOCGPTN, &ptyno) < 0) goto cleanup; - if (fstat(*ttymaster, &st) < 0) - goto cleanup; - - uid = getuid(); - if (st.st_uid != uid) - if (fchown(*ttymaster, uid, -1) < 0) - goto cleanup; - - if ((st.st_mode & ACCESSPERMS) != (S_IRUSR|S_IWUSR|S_IWGRP)) - if (fchmod(*ttymaster, S_IRUSR|S_IWUSR|S_IWGRP) < 0) - goto cleanup; - - /* - * ptyno shouldn't currently be anything other than 0, but let's - * play it safe - */ + /* If mount() succeeded at honoring newinstance, then the kernel + * was new enough to also honor the mode=0620,gid=5 options, which + * guarantee that the new pty already has correct permissions; so + * while glibc has to fstat(), fchmod(), and fchown() for older + * kernels, we can skip those steps. ptyno shouldn't currently be + * anything other than 0, but let's play it safe. */ if (virAsprintf(ttyName, "/dev/pts/%d", ptyno) < 0) { virReportOOMError(); errno = ENOMEM; goto cleanup; } - - rc = 0; + ret = 0; cleanup: - if (rc != 0) { + if (ret != 0) { VIR_FORCE_CLOSE(*ttymaster); - VIR_FREE(*ttyName) + VIR_FREE(*ttyName); } - return rc; + return ret; } static int @@ -935,10 +925,8 @@ lxcControllerRun(virDomainDefPtr def, goto cleanup; } - /* - * todo - should we support gid=X for X!=5 for distros which - * use a different gid for tty? - */ + /* XXX should we support gid=X for X!=5 for distros which use + * a different gid for tty? */ VIR_DEBUG("Mounting 'devpts' on %s", devpts); if (mount("devpts", devpts, "devpts", 0, "newinstance,ptmxmode=0666,mode=0620,gid=5") < 0) { diff --git i/src/util/util.c w/src/util/util.c index 1ec36f1..b2cdf6a 100644 --- i/src/util/util.c +++ w/src/util/util.c @@ -1106,6 +1106,12 @@ int virFileOpenTty(int *ttymaster, { int rc = -1; +#ifdef WIN32 + /* mingw completely lacks pseudo-terminals. */ + errno = ENOSYS; + +#else /* !WIN32 */ + if ((*ttymaster = posix_openpt(O_RDWR|O_NOCTTY|O_NONBLOCK)) < 0) goto cleanup; @@ -1144,8 +1150,8 @@ cleanup: if (rc != 0) VIR_FORCE_CLOSE(*ttymaster); +#endif /* !WIN32 */ return rc; - } /* -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Quoting Eric Blake (eblake@redhat.com):
+ VIR_FORCE_CLOSE(*ttymaster); + VIR_FREE(*ttyName)
How did this ever pass compile-testing without that semicolon?
It didn't. So I fixed it. Then apparently did not do a new git format-patch before sending. Grr. ...
ACK to the intent and to incorporating previous review comments. Here's what I squashed in before pushing:
Great, looks good, thanks. -serge
On Wed, Oct 19, 2011 at 02:47:36PM -0600, Eric Blake wrote:
Looks good (better would be using openpty(), once gnulib changes that to LGPLv2+, but I'll deal with that in a later patch). So, while waiting on gnulib changes, I'll temporarily #ifdef out this code so that I don't break mingw compilation.
Unfortuntely it broke anyway.
ACK to the intent and to incorporating previous review comments. Here's what I squashed in before pushing:
diff --git i/src/util/util.c w/src/util/util.c index 1ec36f1..b2cdf6a 100644 --- i/src/util/util.c +++ w/src/util/util.c @@ -1106,6 +1106,12 @@ int virFileOpenTty(int *ttymaster, { int rc = -1;
+#ifdef WIN32 + /* mingw completely lacks pseudo-terminals. */ + errno = ENOSYS; + +#else /* !WIN32 */ + if ((*ttymaster = posix_openpt(O_RDWR|O_NOCTTY|O_NONBLOCK)) < 0) goto cleanup;
@@ -1144,8 +1150,8 @@ cleanup: if (rc != 0) VIR_FORCE_CLOSE(*ttymaster);
+#endif /* !WIN32 */ return rc; - }
cc1: warnings being treated as errors ../../src/util/util.c: In function 'virFileOpenTty': ../../src/util/util.c:1103:25: error: unused parameter 'ttymaster' [-Wunused-parameter] ../../src/util/util.c:1104:27: error: unused parameter 'ttyName' [-Wunused-parameter] ../../src/util/util.c:1105:24: error: unused parameter 'rawmode' [-Wunused-parameter] Here's what I'm pushing as a build breaker fix diff --git a/src/util/util.c b/src/util/util.c index 01146f5..af27344 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1099,20 +1099,13 @@ virFileBuildPath(const char *dir, const char *name, const char *ext) return path; } - +#ifndef WIN32 int virFileOpenTty(int *ttymaster, char **ttyName, int rawmode) { int rc = -1; -#ifdef WIN32 - /* mingw completely lacks pseudo-terminals, and the gnulib - * replacements are not (yet) license compatible. */ - errno = ENOSYS; - -#else /* !WIN32 */ - if ((*ttymaster = posix_openpt(O_RDWR|O_NOCTTY|O_NONBLOCK)) < 0) goto cleanup; @@ -1151,9 +1144,19 @@ cleanup: if (rc != 0) VIR_FORCE_CLOSE(*ttymaster); -#endif /* !WIN32 */ return rc; } +#else /* WIN32 */ +int virFileOpenTty(int *ttymaster ATTRIBUTE_UNUSED, + char **ttyName ATTRIBUTE_UNUSED, + int rawmode ATTRIBUTE_UNUSED) +{ + /* mingw completely lacks pseudo-terminals, and the gnulib + * replacements are not (yet) license compatible. */ + errno = ENOSYS; + return -1; +} +#endif /* WIN32 */ /* * Creates an absolute path for a potentially relative path. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (5)
-
Daniel P. Berrange -
Eli Qiao -
Eric Blake -
Serge E. Hallyn -
Serge Hallyn