[libvirt] [PATCH 1/1] lxc: use our own hand-rolled code in place of unlockpt and grantpt (v2)
Eric Blake
eblake at redhat.com
Wed Oct 19 20:47:36 UTC 2011
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 at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
More information about the libvir-list
mailing list