9:06 p.m.
MacOS lacks ptsname_r, and gnulib doesn't (yet) provide it.
But we can avoid it altogether, by using gnulib openpty()
instead. Note that we do _not_ want the pt_chown module;
all systems that we currently port to can either properly do
openpty() and/or grantpt(), or lack ptys altogether; we are
not porting to any system that requires us to deal with the
hassle of installing a setuid pt_chown helper just to satisfy
gnulib's ability to provide openpty() on even more platforms.
* .gnulib: Update to latest, for openpty fixes
* bootstrap.conf (gnulib_modules): Add openpty, ttyname_r.
(gnulib_tool_option_extras): Exclude pt_chown module.
* src/util/util.c (virFileOpenTty): Rewrite in terms of openpty
and ttyname_r.
* src/util/util.h (virFileOpenTtyAt): Delete dead prototype.
---
Alas, this is just complicated enough that I don't feel comfortable
pushing it under the build-breaker rule, even though I have verified
that it fixes builds on both FreeBSD and Cygwin, as well as still
behaves correctly with LXC containers on Linux. Anyone willing to
give this a review before 0.9.7 gets released?
.gnulib | 2 +-
bootstrap.conf | 3 ++
src/util/util.c | 73 ++++++++++++++++++++++++++++++++++++++++--------------
src/util/util.h | 5 ----
4 files changed, 58 insertions(+), 25 deletions(-)
diff --git a/.gnulib b/.gnulib
index 2394a60..0031e4f 160000
--- a/.gnulib
+++ b/.gnulib
@@ -1 +1 @@
-Subproject commit 2394a603e7586e671226478e5b15d924c3841f42
+Subproject commit 0031e4f6353cc7077a9d0dad0c793bd6e3dc7aaa
diff --git a/bootstrap.conf b/bootstrap.conf
index 0faa2e2..4557d2d 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -68,6 +68,7 @@ mkstemps
mktempd
netdb
nonblocking
+openpty
passfd
perror
physmem
@@ -101,6 +102,7 @@ sys_wait
termios
time_r
timegm
+ttyname_r
uname
useless-if-before-free
usleep
@@ -168,6 +170,7 @@ tests_base=gnulib/tests
gnulib_tool_option_extras="\
--lgpl=2\
--with-tests\
+ --avoid=pt_chown\
"
# Convince bootstrap to use multiple m4 directories.
diff --git a/src/util/util.c b/src/util/util.c
index bd52b21..959c224 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -45,10 +45,11 @@
#include <string.h>
#include <signal.h>
#include <termios.h>
+#include <pty.h>
+
#if HAVE_LIBDEVMAPPER_H
# include <libdevmapper.h>
#endif
-#include "c-ctype.h"
#ifdef HAVE_PATHS_H
# include <paths.h>
@@ -65,6 +66,7 @@
# include <mntent.h>
#endif
+#include "c-ctype.h"
#include "dirname.h"
#include "virterror_internal.h"
#include "logging.h"
@@ -1172,52 +1174,85 @@ virFileBuildPath(const char *dir, const char *name, const char
*ext)
return path;
}
+/* Open a non-blocking master side of a pty. If ttyName is not NULL,
+ * then populate it with the name of the slave. If rawmode is set,
+ * also put the master side into raw mode before returning. */
#ifndef WIN32
int virFileOpenTty(int *ttymaster,
char **ttyName,
int rawmode)
{
- int rc = -1;
-
- if ((*ttymaster = posix_openpt(O_RDWR|O_NOCTTY|O_NONBLOCK)) < 0)
- goto cleanup;
+ /* XXX A word of caution - on some platforms (Solaris and HP-UX),
+ * additional ioctl() calls are needs after opening the slave
+ * before it will cause isatty() to return true. Should we make
+ * virFileOpenTty also return the opened slave fd, so the caller
+ * doesn't have to worry about that mess? */
+ int ret = -1;
+ int slave = -1;
+ char *name = NULL;
- if (unlockpt(*ttymaster) < 0)
- goto cleanup;
+ /* Unfortunately, we can't use the name argument of openpty, since
+ * there is no guarantee on how large the buffer has to be.
+ * Likewise, we can't use the termios argument: we have to use
+ * read-modify-write since there is no portable way to initialize
+ * a struct termios without use of tcgetattr. */
+ if (openpty(ttymaster, &slave, NULL, NULL, NULL) < 0)
+ return -1;
- if (grantpt(*ttymaster) < 0)
+ /* What a shame that openpty cannot atomically set FD_CLOEXEC, but
+ * that using posix_openpt/grantpt/unlockpt/ptsname is not
+ * thread-safe, and that ptsname_r is not portable. */
+ if (virSetNonBlock(*ttymaster) < 0 ||
+ virSetCloseExec(*ttymaster) < 0)
goto cleanup;
+ /* While Linux supports tcgetattr on either the master or the
+ * slave, Solaris requires it to be on the slave. */
if (rawmode) {
struct termios ttyAttr;
- if (tcgetattr(*ttymaster, &ttyAttr) < 0)
+ if (tcgetattr(slave, &ttyAttr) < 0)
goto cleanup;
cfmakeraw(&ttyAttr);
- if (tcsetattr(*ttymaster, TCSADRAIN, &ttyAttr) < 0)
+ if (tcsetattr(slave, TCSADRAIN, &ttyAttr) < 0)
goto cleanup;
}
+ /* ttyname_r on the slave is required by POSIX, while ptsname_r on
+ * the master is a glibc extension, and the POSIX ptsname is not
+ * thread-safe. Since openpty gave us both descriptors, guess
+ * which way we will determine the name? :) */
if (ttyName) {
- if (VIR_ALLOC_N(*ttyName, PATH_MAX) < 0) {
- errno = ENOMEM;
+ /* Initial guess of 64 is generally sufficient; rely on ERANGE
+ * to tell us if we need to grow. */
+ size_t len = 64;
+ int rc;
+
+ if (VIR_ALLOC_N(name, len) < 0)
goto cleanup;
- }
- if (ptsname_r(*ttymaster, *ttyName, PATH_MAX) != 0) {
- VIR_FREE(*ttyName);
+ while ((rc = ttyname_r(slave, name, len)) == ERANGE) {
+ if (VIR_RESIZE_N(name, len, len, len) < 0)
+ goto cleanup;
+ }
+ if (rc != 0) {
+ errno = rc;
goto cleanup;
}
+ *ttyName = name;
+ name = NULL;
}
- rc = 0;
+ ret = 0;
cleanup:
- if (rc != 0)
+ if (ret != 0)
VIR_FORCE_CLOSE(*ttymaster);
+ VIR_FORCE_CLOSE(slave);
+ VIR_FREE(name);
- return rc;
+ return ret;
}
#else /* WIN32 */
int virFileOpenTty(int *ttymaster ATTRIBUTE_UNUSED,
diff --git a/src/util/util.h b/src/util/util.h
index e869f1b..d8176a8 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -121,11 +121,6 @@ int virFileAbsPath(const char *path,
int virFileOpenTty(int *ttymaster,
char **ttyName,
int rawmode);
-int virFileOpenTtyAt(const char *ptmx,
- int *ttymaster,
- char **ttyName,
- int rawmode);
-
char *virArgvToString(const char *const *argv);
--
1.7.4.4