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