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

Re: [libvirt] [PATCH 1/1] lxc: use our own hand-rolled code in place of unlockpt and grantpt (v2)



On 10/18/2011 07:39 PM, Serge E. Hallyn wrote:
New version, compile-tested only tonight.  I followed the suggestion
about using posix_openpt(), though its manpage worries me - does libvirt
need to compile on any platforms that don't have that fn?  (In which case
we can add the trivial define if we need to, but...)

Libvirt compiles on mingw, which lacks posix_openpt (and in fact, lacks pseudo-terminal altogether). We trivially support functions like this by importing the corresponding gnulib modules (which either implement the workarounds, or gracefully fail with ENOSYS); I recently added posix_openpt() to gnulib, although looking at it further, I'd much rather use gnulib's openpty() function instead of the POSIX sequence of posix_openpt()/grantpt()/unlockpt()/ptsname()/open(), especially since the POSIX sequence is still insufficiently portable (on Solaris and HP-UX, it has to include additional ioctl() calls to enable STREAMS filters before isatty(slave) will return non-zero). But right now, those interfaces are LGPLv3+, so I'm waiting for Bruno to relax their license:

https://lists.gnu.org/archive/html/bug-gnulib/2011-10/msg00178.html

@@ -780,6 +782,62 @@ static int lxcSetPersonality(virDomainDefPtr def)
  # define MS_SLAVE              (1<<19)
  #endif

+#define ACCESSPERMS (S_IRWXU|S_IRWXG|S_IRWXO) /* 0777 */
+
+/* heavily borrowed from glibc, but don't assume devpts == "/dev/pts" */
+static int
+lxcCreateTty(char *ptmx, int *ttymaster, char **ttyName)
+{
+    int rc = -1;

We typically use ret for our returns, and rc for temporary variables, although that naming convention doesn't really matter too much.

+    int ret;
+    int ptyno;
+    uid_t uid;
+    struct stat st;
+    int unlock = 0;
+
+    if ((*ttymaster = open(ptmx, O_RDWR|O_NOCTTY|O_NONBLOCK))<  0)
+        goto cleanup;
+
+    if (ioctl(*ttymaster, TIOCSPTLCK,&unlock)<  0)
+        goto cleanup;
+
+    if (ioctl(*ttymaster, TIOCGPTN,&ptyno)<  0)
+        goto cleanup;

So far, so good.

+
+    if (fstat(*ttymaster,&st)<  0)
+        goto cleanup;
+
+    uid = getuid();
+    if (st.st_uid != uid)
+        if (fchown(*ttymaster, uid, -1)<  0)
+            goto cleanup;

Not required. 'man mount', under the devpts section, makes it clear that "When nothing is specified, they will be set to the UID and GID of the creating process.", which means st.st_uid will _always_ match getuid() because we just did the open() and there was no uid= mount option.

+
+    if ((st.st_mode&  ACCESSPERMS) != (S_IRUSR|S_IWUSR|S_IWGRP))
+        if (fchmod(*ttymaster, S_IRUSR|S_IWUSR|S_IWGRP)<  0)
+            goto cleanup;

Not required. Again, the docs state that 'mode=0620' was sufficient to already guarantee this setting.

+        VIR_FORCE_CLOSE(*ttymaster);
+        VIR_FREE(*ttyName)

How did this ever pass compile-testing without that semicolon?

@@ -877,6 +935,10 @@ lxcControllerRun(virDomainDefPtr def,
              goto cleanup;
          }

+	/*
+	 * todo - should we support gid=X for X!=5 for distros which
+	 * use a different gid for tty?
+	 */

No TABs.  'make syntax-check' caught this.

+++ b/src/util/util.c
@@ -1104,21 +1104,9 @@ int virFileOpenTty(int *ttymaster,
                     char **ttyName,
                     int rawmode)
  {
-    return virFileOpenTtyAt("/dev/ptmx",
-                            ttymaster,
-                            ttyName,
-                            rawmode);
-}
-
-#ifdef __linux__
-int virFileOpenTtyAt(const char *ptmx,
-                     int *ttymaster,
-                     char **ttyName,
-                     int rawmode)
-{
      int rc = -1;

-    if ((*ttymaster = open(ptmx, O_RDWR|O_NOCTTY|O_NONBLOCK))<  0)
+    if ((*ttymaster = posix_openpt(O_RDWR|O_NOCTTY|O_NONBLOCK))<  0)

Looks good (better would be using openpty(), once gnulib changes that to LGPLv2+, but I'll deal with that in a later patch). So, while waiting on gnulib changes, I'll temporarily #ifdef out this code so that I don't break mingw compilation.

ACK to the intent and to incorporating previous review comments. Here's what I squashed in before pushing:


diff --git i/src/lxc/lxc_controller.c w/src/lxc/lxc_controller.c
index 464bfe8..fad4259 100644
--- i/src/lxc/lxc_controller.c
+++ w/src/lxc/lxc_controller.c
@@ -784,15 +784,16 @@ static int lxcSetPersonality(virDomainDefPtr def)

 #define ACCESSPERMS (S_IRWXU|S_IRWXG|S_IRWXO) /* 0777 */

-/* heavily borrowed from glibc, but don't assume devpts == "/dev/pts" */
+/* Create a private tty using the private devpts at PTMX, returning
+ * the master in *TTYMASTER and the name of the slave, _from the
+ * perspective of the guest after remounting file systems_, in
+ * *TTYNAME.  Heavily borrowed from glibc, but doesn't require that
+ * devpts == "/dev/pts" */
 static int
 lxcCreateTty(char *ptmx, int *ttymaster, char **ttyName)
 {
-    int rc = -1;
-    int ret;
+    int ret = -1;
     int ptyno;
-    uid_t uid;
-    struct stat st;
     int unlock = 0;

     if ((*ttymaster = open(ptmx, O_RDWR|O_NOCTTY|O_NONBLOCK)) < 0)
@@ -804,38 +805,27 @@ lxcCreateTty(char *ptmx, int *ttymaster, char **ttyName)
     if (ioctl(*ttymaster, TIOCGPTN, &ptyno) < 0)
         goto cleanup;

-    if (fstat(*ttymaster, &st) < 0)
-        goto cleanup;
-
-    uid = getuid();
-    if (st.st_uid != uid)
-        if (fchown(*ttymaster, uid, -1) < 0)
-            goto cleanup;
-
-    if ((st.st_mode & ACCESSPERMS) != (S_IRUSR|S_IWUSR|S_IWGRP))
-        if (fchmod(*ttymaster, S_IRUSR|S_IWUSR|S_IWGRP) < 0)
-            goto cleanup;
-
-    /*
-     * ptyno shouldn't currently be anything other than 0, but let's
-     * play it safe
-     */
+    /* If mount() succeeded at honoring newinstance, then the kernel
+     * was new enough to also honor the mode=0620,gid=5 options, which
+     * guarantee that the new pty already has correct permissions; so
+     * while glibc has to fstat(), fchmod(), and fchown() for older
+     * kernels, we can skip those steps.  ptyno shouldn't currently be
+     * anything other than 0, but let's play it safe.  */
     if (virAsprintf(ttyName, "/dev/pts/%d", ptyno) < 0) {
         virReportOOMError();
         errno = ENOMEM;
         goto cleanup;
     }

-
-    rc = 0;
+    ret = 0;

 cleanup:
-    if (rc != 0) {
+    if (ret != 0) {
         VIR_FORCE_CLOSE(*ttymaster);
-        VIR_FREE(*ttyName)
+        VIR_FREE(*ttyName);
     }

-    return rc;
+    return ret;
 }

 static int
@@ -935,10 +925,8 @@ lxcControllerRun(virDomainDefPtr def,
             goto cleanup;
         }

-	/*
-	 * todo - should we support gid=X for X!=5 for distros which
-	 * use a different gid for tty?
-	 */
+        /* XXX should we support gid=X for X!=5 for distros which use
+         * a different gid for tty?  */
         VIR_DEBUG("Mounting 'devpts' on %s", devpts);
         if (mount("devpts", devpts, "devpts", 0,
                   "newinstance,ptmxmode=0666,mode=0620,gid=5") < 0) {
diff --git i/src/util/util.c w/src/util/util.c
index 1ec36f1..b2cdf6a 100644
--- i/src/util/util.c
+++ w/src/util/util.c
@@ -1106,6 +1106,12 @@ int virFileOpenTty(int *ttymaster,
 {
     int rc = -1;

+#ifdef WIN32
+    /* mingw completely lacks pseudo-terminals.  */
+    errno = ENOSYS;
+
+#else /* !WIN32 */
+
     if ((*ttymaster = posix_openpt(O_RDWR|O_NOCTTY|O_NONBLOCK)) < 0)
         goto cleanup;

@@ -1144,8 +1150,8 @@ cleanup:
     if (rc != 0)
         VIR_FORCE_CLOSE(*ttymaster);

+#endif /* !WIN32 */
     return rc;
-
 }

 /*

--
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


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