9:31 p.m.
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(a)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
9:32 p.m.
New subject: [libvirt] [PATCH 2/2] Fix type in lxc_controller
s/Mouting/Mounting.
Signed-off-by: Serge Hallyn <serge.hallyn(a)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
4:07 a.m.
New subject: [libvirt] [PATCH 2/2] Fix type in lxc_controller
On Wed, Oct 12, 2011 at 09:32:03PM -0500, Serge E. Hallyn wrote:
s/Mouting/Mounting.
Signed-off-by: Serge Hallyn <serge.hallyn(a)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 :|
10:45 a.m.
New subject: [libvirt] [PATCH 2/2] Fix type in lxc_controller
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(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
8:20 a.m.
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 :|
10:47 a.m.
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(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
11:53 a.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
8:59 a.m.
Quoting Eric Blake (eblake(a)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
11:29 a.m.
New subject: [libvirt] [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.
Signed-off-by: Serge Hallyn <serge.hallyn(a)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
10:54 p.m.
New subject: [libvirt] [PATCH 1/1] lxc: use our own hand-rolled code in place of unlockpt and grantpt
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
best regards
eli
8:26 a.m.
New subject: [libvirt] [PATCH 1/1] lxc: use our own hand-rolled code in place of unlockpt and grantpt
Quoting Eli Qiao (taget(a)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
9:32 a.m.
New subject: [libvirt] [PATCH 1/1] lxc: use our own hand-rolled code in place of unlockpt and grantpt
On 10/17/2011 07:26 AM, Serge E. Hallyn wrote:
Quoting Eli Qiao (taget(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
2:04 p.m.
New subject: [libvirt] [PATCH 1/1] lxc: use our own hand-rolled code in place of unlockpt and grantpt (v2)
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(a)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
3:57 p.m.
New subject: [libvirt] [PATCH 1/1] lxc: use our own hand-rolled code in place of unlockpt and grantpt (v2)
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(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4:29 p.m.
New subject: [libvirt] [PATCH 1/1] lxc: use our own hand-rolled code in place of unlockpt and grantpt (v2)
Quoting Eric Blake (eblake(a)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(a)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
4:40 p.m.
New subject: [libvirt] [PATCH 1/1] lxc: use our own hand-rolled code in place of unlockpt and grantpt (v2)
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
7:58 p.m.
New subject: [libvirt] [PATCH 1/1] lxc: use our own hand-rolled code in place of unlockpt and grantpt (v2)
Quoting Eric Blake (eblake(a)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
8:39 p.m.
New subject: [libvirt] [PATCH 1/1] lxc: use our own hand-rolled code in place of unlockpt and grantpt (v2)
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(a)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
3:47 p.m.
New subject: [libvirt] [PATCH 1/1] lxc: use our own hand-rolled code in place of unlockpt and grantpt (v2)
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:52 p.m.
New subject: [libvirt] [PATCH 1/1] lxc: use our own hand-rolled code in place of unlockpt and grantpt (v2)
Quoting Eric Blake (eblake(a)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
4:03 a.m.
New subject: [libvirt] [PATCH 1/1] lxc: use our own hand-rolled code in place of unlockpt and grantpt (v2)
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 :|
4784
days inactive
4791
days old
20 comments
5 participants
participants (5)
-
Daniel P. Berrange -
Eli Qiao -
Eric Blake -
Serge E. Hallyn -
Serge Hallyn