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):

New module 'unlinkat', split off from module 'openat'. New module 'fchmodat', split off from module 'openat'. putenv: indent #definition of "environ" to placate cppi gitlog-to-changelog: provide a ChangeLog-repair mechanism gitlog-to-changelog: avoid an infloop * MODULES.html.sh: Fix sed-script shell quoting and locale issues. fchownat: Improve description. * tests/test-stdalign.c (TEST_ALIGNMENT): Shrink back to 8. Fix my old ChangeLog entry to properly cite Bruno's email. alignof: Avoid collision with stdalign module. New module 'fchownat', split off from module 'openat'. stdalign: port better to MSVC and to Sun C 5.11 doc about some IRIX 5.3 problems. gitlog-to-changelog: fix git-log invocation gitlog-to-changelog: new option --append-dot ffsl, ffsll: Avoid compilation error due to 'restrict'. GNUmakefile: reenable "make syntax-check" for most projects gitlog-to-changelog: treat a message with only blank lines as empty test-parse-datetime.c: avoid new DST-related false positive test failure autoupdate fstat: Tweak documentation. Update documentation regarding 'largefile' module. maint.mk: don't maintain a second build-aux variable. Adjust to Bruno's comments. sys_socket: use stdalign, not alignof crypto libraries: use stdalign argp: use stdalign stdalign-tests: new module stdalign: new module raise test: Avoid a test failure on Linux/MIPS. nonblocking tests: Fix test failure on Linux/MIPS. utimensat: Work around problem on Linux/hppa. maint.mk: fix a bug in sc_prohibit_stddef_without_use maint.mk: exempt ENODATA from a syntax-check rule fts: close parent dir FD before returning from post-traversal fts_read autoupdate readme-release: improve safety of release prep instructions. errno, strerror-override: Support for MSVC 10. perror: Recognize when test program crashes. perror: Fix indentation. isfinite, isinf, isnan, signbit: Don't define as a macro in C++. relocatable-prog-wrapper: Don't leave object files behind. openpty, posix_openpt: Remove code duplication. unlockpt: Detect invalid argument. openpty: Avoid compilation error on AIX 6.1. autoupdate posix_openpt: Support for OpenBSD. posix_openpt test: Coding style. grantpt: Support --avoid=pt_chown. posix_openpt: Fix autoconf macro. openpty: Update comments. openpty: relax license pt_chown: use configmake to simplify build ptsname and others: relax license update from texinfo posix_openpt: remove spurious #endif maint.mk: Respect $(build_aux) in web-manual rule. posix_openpt: Fix compilation error. Support for old NeXTstep 3.3 frexp(). Support for old NeXTstep 3.3 sed. Support for old NeXTstep 3.3 gcc. posix_openpt: new module xstrtoll: Fix compilation failure. vasnprintf: Optimize bit search operation. vasnprintf: Fix comments. Tests for module 'integer_length_ll'. New module 'integer_length_ll'. Tests for module 'integer_length_l'. New module 'integer_length_l'. Tests for module 'integer_length'. New module 'integer_length'. popen: Fix dependency conditions. perror: Fix autoconf test. ffsl: Optimize on 64-bit platforms. autoupdate ffsl: Optimize on 32-bit platforms. ffsl, ffsll: Optimize for GCC. ffs, bcopy, memset: Support symbol renaming via config.h.

.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:

On 11/02/2011 12:57 PM, Guido Günther wrote:

...

Built fine on most Debian architectures:

https://buildd.debian.org/status/package.php?p=libvirt&suite=experimental

The built failure on amd64 is due to virnetsockettest failing with:

5) Socket UNIX Accept... libvir: RPC error : Path /build/buildd-libvirt_0.9.7~rc1-1-amd64-EGXZTE/libvirt-0.9.7~rc1/debian/build/tests/virnetsockettest-test.sock too long for unix socket: Cannot allocate memory

since the socket path doesn't fit in UNIX_PATH_MAX. Since exceeding the path shouldn't't be fatal I'm using the attached patch.

...

From: =?UTF-8?q?Guido=20G=C3=BCnther?=<agx@sigxcpu.org> Date: Wed, 2 Nov 2011 19:02:42 +0100 Subject: Skip socket test if we exceed UNIX_PATH_MAX.

As seen on the amd64 buildd with:

s/buildd/build/

This one was actually intentional. It's the build daemon, short buildd.

...

+++ b/tests/virnetsockettest.c @@ -205,11 +205,13 @@ static int testSocketUNIXAccept(const void *data ATTRIBUTE_UNUSED) if (progname[0] == '/') { if (virAsprintf(&path, "%s-test.sock", progname)< 0) { virReportOOMError(); + ret = EXIT_AM_SKIP; goto cleanup; }

Wrong place to be checking - virAsprintf() only fails on OOM (malloc failure), not on size fit, so your skip is unlikely to be hit. I agree that it's okay to skip the test if run in a subdirectory so deep that a Unix socket cannot be created with a name that long, but that should be done strlen() check just before virNetSocketNewListenUNIX, not by looking for malloc failure. And since we're most likely not out of memory, that may mean we also have a bug to fix in our error reporting quality within virNetSocketNewListenUNIX.

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:

--- src/rpc/virnetsocket.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index ab88e19..d832c53 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -327,7 +327,8 @@ int virNetSocketNewListenUNIX(const char *path,

addr.data.un.sun_family = AF_UNIX; if (virStrcpyStatic(addr.data.un.sun_path, path) == NULL) { - virReportSystemError(ENOMEM, _("Path %s too long for unix socket"), path); + virReportSystemError(ENAMETOOLONG, + _("Path %s too long for unix socket"), path);

ACK. [In fact, I had the exact same patch in my sandbox, but you beat me in posting it!] -- Eric Blake eblake@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:

@@ -214,6 +219,19 @@ static int testSocketUNIXAccept(const void *data ATTRIBUTE_UNUSED) } }

+ if (strlen(path) >= sizeof(sun.sun_path)) { + if (!virStrcpy(path, "/tmp/test.sock.XXXXXX", sizeof(sun.sun_path))) { + VIR_DEBUG("Unexpected error using virStrcpyStatic"); + goto cleanup; + } + tmpfd = mkostemp(path, 0700); + if (tmpfd < 0) { + virReportSystemError(errno, "%s", + _("Failed to create temporary file")); + goto cleanup; + } + } + if (virNetSocketNewListenUNIX(path, 0700, -1, getgid(), &lsock) < 0)

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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org