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. Reported by Justin Clift. --- I compile-tested this on Linux, but haven't yet tested this on MacOS, which is the driving force behind this patch. Meanwhile, it would be nice to run some LXC tests to ensure this all still works. I'm posting it now, to get the review started while I figure out how to run further tests. Gnulib changes are: * .gnulib 2394a60...84c3f9c (78): .gnulib | 2 +- bootstrap.conf | 3 ++ src/util/util.c | 62 ++++++++++++++++++++++++++++++++++++------------------ src/util/util.h | 5 ---- 4 files changed, 45 insertions(+), 27 deletions(-) diff --git a/.gnulib b/.gnulib index 2394a60..84c3f9c 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit 2394a603e7586e671226478e5b15d924c3841f42 +Subproject commit 84c3f9cf54eaa688c5a1a26925886535079a91de 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..5130403 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" @@ -1177,47 +1179,65 @@ int virFileOpenTty(int *ttymaster, char **ttyName, int rawmode) { - int rc = -1; - - if ((*ttymaster = posix_openpt(O_RDWR|O_NOCTTY|O_NONBLOCK)) < 0) - goto cleanup; - - if (unlockpt(*ttymaster) < 0) - goto cleanup; + int ret = -1; + int slave = -1; + char *name = NULL; - if (grantpt(*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; + /* 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

On Wed, Nov 02, 2011 at 02:19:07PM -0600, Eric Blake wrote: This one was actually intentional. It's the build daemon, short buildd. Yes - the analysis was correct, the patch was horribly broken though. New versions attached. I wen't for a tempdir though to make sure the test can succeed. -- Guido

On 11/02/2011 03:30 PM, Guido Günther wrote: ACK. [In fact, I had the exact same patch in my sandbox, but you beat me in posting it!] -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

to avoid exceeding UNIX_PATH_MAX --- tests/virnetsockettest.c | 60 ++++++++++++++++++++++++++++++--------------- 1 files changed, 40 insertions(+), 20 deletions(-) diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index 6320ce0..aeb4f3f 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -202,16 +202,23 @@ static int testSocketUNIXAccept(const void *data ATTRIBUTE_UNUSED) int ret = -1; char *path; - if (progname[0] == '/') { - if (virAsprintf(&path, "%s-test.sock", progname) < 0) { - virReportOOMError(); - goto cleanup; - } - } else { - if (virAsprintf(&path, "%s/%s-test.sock", abs_builddir, progname) < 0) { - virReportOOMError(); - goto cleanup; - } + char *template; + char *tmpdir = NULL; + + template = strdup("/tmp/libvirt_XXXXXX"); + if (template == NULL) { + virReportOOMError(); + goto cleanup; + } + tmpdir = mkdtemp(template); + if (tmpdir == NULL) { + virReportSystemError(errno, "%s", + _("Failed to create temporary directory")); + goto cleanup; + } + if (virAsprintf(&path, "%s/test.sock", tmpdir) < 0) { + virReportOOMError(); + goto cleanup; } if (virNetSocketNewListenUNIX(path, 0700, -1, getgid(), &lsock) < 0) @@ -239,6 +246,9 @@ cleanup: VIR_FREE(path); virNetSocketFree(lsock); virNetSocketFree(ssock); + if (tmpdir) + rmdir(tmpdir); + VIR_FREE(template); return ret; } @@ -251,16 +261,23 @@ static int testSocketUNIXAddrs(const void *data ATTRIBUTE_UNUSED) int ret = -1; char *path; - if (progname[0] == '/') { - if (virAsprintf(&path, "%s-test.sock", progname) < 0) { - virReportOOMError(); - goto cleanup; - } - } else { - if (virAsprintf(&path, "%s/%s-test.sock", abs_builddir, progname) < 0) { - virReportOOMError(); - goto cleanup; - } + char *template; + char *tmpdir = NULL; + + template = strdup("/tmp/libvirt_XXXXXX"); + if (template == NULL) { + virReportOOMError(); + goto cleanup; + } + tmpdir = mkdtemp(template); + if (tmpdir == NULL) { + virReportSystemError(errno, "%s", + _("Failed to create temporary directory")); + goto cleanup; + } + if (virAsprintf(&path, "%s/test.sock", tmpdir) < 0) { + virReportOOMError(); + goto cleanup; } if (virNetSocketNewListenUNIX(path, 0700, -1, getgid(), &lsock) < 0) @@ -317,6 +334,9 @@ cleanup: virNetSocketFree(lsock); virNetSocketFree(ssock); virNetSocketFree(csock); + if (tmpdir) + rmdir(tmpdir); + VIR_FREE(template); return ret; } -- 1.7.7

On 11/02/2011 02:24 PM, Stefan Berger wrote: This relies on the guts of NewListenUNIX to do an unlink() of the file that we still have an open fd on, and create a socket in its place; which is slightly racy, not to mention non-portable to platforms where unlink() fails with open fds (then again, Unix sockets don't exist on those platforms - specifically mingw). But we already have precedence of skipping tests in deep hierarchies: TEST: daemon-conf .................................../../../../../tests/daemon-conf: skipping test: CWD too long SKIP: daemon-conf Maybe we should just skip instead of playing around with /tmp in that case? -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org