[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[libvirt] [PATCHv2] build: fix build on platforms without ptsname_r



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]