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

2011-10-20 Thread Daniel P. Berrange 
On Wed, Oct 19, 2011 at 02:47:36PM -0600, Eric Blake wrote:
 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.

Unfortuntely it broke anyway.

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

 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;
 -
  }

cc1: warnings being treated as errors
../../src/util/util.c: In function 'virFileOpenTty':
../../src/util/util.c:1103:25: error: unused parameter 'ttymaster' 
[-Wunused-parameter]
../../src/util/util.c:1104:27: error: unused parameter 'ttyName' 
[-Wunused-parameter]
../../src/util/util.c:1105:24: error: unused parameter 'rawmode' 
[-Wunused-parameter]

Here's what I'm pushing as a build breaker fix

diff --git a/src/util/util.c b/src/util/util.c
index 01146f5..af27344 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -1099,20 +1099,13 @@ virFileBuildPath(const char *dir, const char *name, 
const char *ext)
 return path;
 }
 
-
+#ifndef WIN32
 int virFileOpenTty(int *ttymaster,
char **ttyName,
int rawmode)
 {
 int rc = -1;
 
-#ifdef WIN32
-/* mingw completely lacks pseudo-terminals, and the gnulib
- * replacements are not (yet) license compatible.  */
-errno = ENOSYS;
-
-#else /* !WIN32 */
-
 if ((*ttymaster = posix_openpt(O_RDWR|O_NOCTTY|O_NONBLOCK))  0)
 goto cleanup;
 
@@ -1151,9 +1144,19 @@ cleanup:
 if (rc != 0)
 VIR_FORCE_CLOSE(*ttymaster);
 
-#endif /* !WIN32 */
 return rc;
 }
+#else /* WIN32 */
+int virFileOpenTty(int *ttymaster ATTRIBUTE_UNUSED,
+   char **ttyName ATTRIBUTE_UNUSED,
+   int rawmode ATTRIBUTE_UNUSED)
+{
+/* mingw completely lacks pseudo-terminals, and the gnulib
+ * replacements are not (yet) license compatible.  */
+errno = ENOSYS;
+return -1;
+}
+#endif /* WIN32 */
 
 /*
  * Creates an absolute path for a potentially relative path.


Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2011-10-19 Thread Eric Blake 

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  (119)
  #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;

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

2011-10-19 Thread Serge E. Hallyn 
Quoting Eric Blake (ebl...@redhat.com):
 +VIR_FORCE_CLOSE(*ttymaster);
 +VIR_FREE(*ttyName)
 
 How did this ever pass compile-testing without that semicolon?

It didn't.  So I fixed it.  Then apparently did not do a new
git format-patch before sending.  Grr.

...

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

Great, looks good, thanks.

-serge

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2011-10-18 Thread Serge E. Hallyn 
Quoting Eric Blake (ebl...@redhat.com):
 [but we still have to fix the hard-coding of
 gid=5 in the mount() option].
 
 I missed something - why do we have to fix that?
 
 We don't have to fix it now, but we should fix it someday.  There's
 nothing that says a distro has to map 'tty' to gid 5, and while most
 distros do that, we should instead be portable to compilation on a
 distro where 'tty' is gid 6 (or some other unusual number).

But that gid should be set according to what the guest expects, right?
So we actually can't do it at compilation?

 Instead, I'd just do this as:
 
 virAsprintf(ttyName, /dev/pts/%d, ptyno);
 
 Where virAsprintf will allocate when ttyName starts as null?
 
 Yep - the whole point of virAsprintf is to allocate the string on
 your behalf, and to gracefully ensure that ttyName is NULL on
 allocation failure - much more compact than using snprintf yourself,
 and avoids the waste of a PATH_MAX allocation.
 
 And, while you are at it, what about also fixing src/util/util.c to
 remove virFileOpenTtyAt (now that no one calls that), by moving its
 body into virFileOpenTty except for using posix_openpt instead of
 open(/dev/ptmx).

I was thinking that should be a separate patch for ease of review, but
I'll roll it into my next patch.

thanks,
-serge

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2011-10-18 Thread Serge E. Hallyn 
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...)

Subject: [PATCH 1/1] lxc: use our own hand-rolled code in place of unlockpt
 and grantpt

The glibc ones cannot handle ptys opened in a devpts not mounted at
/dev/pts.

Changelog:
  Oct 17: Per Eli Qiao, use VIR_ALLOC_N when appropriate, make
  sure to check return values, and follow coding style
  convention.
  Change lxcGetTtyGid() to return -1 on error, otherwise
  return gid in passed-in argument.
  Oct 18: Per Eric Blake, consolidate virFileOpenTtyAt() into
  virFileOpenTty(), and use posix_openpt().
  Per Eric Blake, don't set gid on opened pty since we
  ask the kernel to do so with 'gid=5' mount option.

Signed-off-by: Serge Hallyn serge.hal...@canonical.com
---
 src/lxc/lxc_controller.c |   67 +++---
 src/util/util.c  |   24 +
 2 files changed, 64 insertions(+), 27 deletions(-)

diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 89ce7f5..464bfe8 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -41,6 +41,8 @@
 #include locale.h
 #include linux/loop.h
 #include dirent.h
+#include grp.h
+#include sys/stat.h
 
 #if HAVE_CAPNG
 # include cap-ng.h
@@ -780,6 +782,62 @@ static int lxcSetPersonality(virDomainDefPtr def)
 # define MS_SLAVE  (119)
 #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;
+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;
+
+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 (virAsprintf(ttyName, /dev/pts/%d, ptyno)  0) {
+virReportOOMError();
+errno = ENOMEM;
+goto cleanup;
+}
+
+
+rc = 0;
+
+cleanup:
+if (rc != 0) {
+VIR_FORCE_CLOSE(*ttymaster);
+VIR_FREE(*ttyName)
+}
+
+return rc;
+}
+
 static int
 lxcControllerRun(virDomainDefPtr def,
  unsigned int nveths,
@@ -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?
+*/
 VIR_DEBUG(Mounting 'devpts' on %s, devpts);
 if (mount(devpts, devpts, devpts, 0,
   newinstance,ptmxmode=0666,mode=0620,gid=5)  0) {
@@ -894,10 +956,7 @@ lxcControllerRun(virDomainDefPtr def,
 
 if (devptmx) {
 VIR_DEBUG(Opening tty on private %s, devptmx);
-if (virFileOpenTtyAt(devptmx,
- containerPty,
- containerPtyPath,
- 0)  0) {
+if (lxcCreateTty(devptmx, containerPty, containerPtyPath)  0) {
 virReportSystemError(errno, %s,
  _(Failed to allocate tty));
 goto cleanup;
diff --git a/src/util/util.c b/src/util/util.c
index dac616b..1ec36f1 100644
--- a/src/util/util.c
+++ 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)
 goto cleanup;
 
 if (unlockpt(*ttymaster)  0)
@@ -1159,16 +1147,6 @@ cleanup:
 return rc;
 
 }
-#else
-int virFileOpenTtyAt(const char *ptmx ATTRIBUTE_UNUSED,
- int *ttymaster ATTRIBUTE_UNUSED,
- char **ttyName ATTRIBUTE_UNUSED,
- int rawmode ATTRIBUTE_UNUSED)
-{
-

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

2011-10-17 Thread Serge E. Hallyn 
Quoting Eli Qiao (ta...@linux.vnet.ibm.com):
 hi Serge :

Thanks for taking a look.

 I checked the code ,  only in lxc_controller.c call virFileOpenTtyAt().
 I didn't test the path, but my suggestion is that modify the
 utility function in /src/util/util.c instead of adding a new
 function.

But virFileOpenTtyAt() is called by virFileOpenTty() in the same
file.  So we really do want a new function using its own versions of
grantpt+unlockpt, because I think that, when we can, we want to continue
using the glibc versions.

So the safe approach seemed to me to be: put the container-specific
code into src/lxc/lxc_controller.c, then (in a separate patch) just fold
virFileOpenTtyAt(), straight into virFileOpenTty().

If you agree, I'll post a new version incorporating your other feedback,
especially the garish use of alloca :)

(If you disagree, please feel free to send your own patch - I'm in no
way attached to having my version included, I mainly wanted to point out
the bug)

thanks,
-serge

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2011-10-17 Thread Eric Blake 

On 10/17/2011 07:26 AM, Serge E. Hallyn wrote:

Quoting Eli Qiao (ta...@linux.vnet.ibm.com):

hi Serge :


Thanks for taking a look.


 I checked the code ,  only in lxc_controller.c call virFileOpenTtyAt().
 I didn't test the path, but my suggestion is that modify the
utility function in /src/util/util.c instead of adding a new
function.


But virFileOpenTtyAt() is called by virFileOpenTty() in the same
file.  So we really do want a new function using its own versions of
grantpt+unlockpt, because I think that, when we can, we want to continue
using the glibc versions.

So the safe approach seemed to me to be: put the container-specific
code into src/lxc/lxc_controller.c, then (in a separate patch) just fold
virFileOpenTtyAt(), straight into virFileOpenTty().


Correct - my intent was that we have:

src/util/util.c: virFileOpenTty() - generally useful by any driver, uses 
glibc, and also should use posix_openpt rather than than 
open(/dev/ptmx) for portability (gnulib provides posix_openpt). 
Completely ditch virFileOpenTtyAt() by inlining it back to 
virFileOpenTty(), and since no one could use what virFileOpenTtyAt() 
claimed to provide except in the portable case where virFileOpenTty() 
was sufficient.


src/lxc/lxc_controller.c - LXC-specific function that uses ioctl() 
instead of glibc for opening a private tty.


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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2011-10-17 Thread Serge E. Hallyn 
The glibc ones cannot handle ptys opened in a devpts not mounted at
/dev/pts.

Changelog:
  Oct 17: Per Eli Qiao, use VIR_ALLOC_N when appropriate, make
  sure to check return values, and follow coding style
  convention.
  Change lxcGetTtyGid() to return -1 on error, otherwise
  return gid in passed-in argument.

Signed-off-by: Serge Hallyn serge.hal...@canonical.com
---
 src/lxc/lxc_controller.c |   89 +++--
 1 files changed, 85 insertions(+), 4 deletions(-)

diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 89ce7f5..8c9caee 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -41,6 +41,8 @@
 #include locale.h
 #include linux/loop.h
 #include dirent.h
+#include grp.h
+#include sys/stat.h
 
 #if HAVE_CAPNG
 # include cap-ng.h
@@ -781,6 +783,88 @@ static int lxcSetPersonality(virDomainDefPtr def)
 #endif
 
 static int
+lxcGetTtyGid(int *gid) {
+char *grtmpbuf;
+struct group grbuf;
+size_t grbuflen = sysconf(_SC_GETGR_R_SIZE_MAX);
+struct group *p;
+int ret;
+gid_t tty_gid = -1;
+
+/* Get the group ID of the special `tty' group.  */
+if (grbuflen == (size_t) -1L)
+/* `sysconf' does not support _SC_GETGR_R_SIZE_MAX.
+   Try a moderate value.  */
+grbuflen = 1024;
+
+if ((VIR_ALLOC_N(grtmpbuf, grbuflen))  0)
+   return -1;
+
+ret = getgrnam_r(tty, grbuf, grtmpbuf, grbuflen, p);
+if (ret || p != NULL)
+tty_gid = p-gr_gid;
+
+VIR_FREE(grtmpbuf);
+
+*gid = tty_gid == -1 ? getgid() : tty_gid;
+return 0;
+}
+
+#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;
+int ret;
+int ptyno;
+uid_t uid;
+gid_t gid;
+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;
+
+if (fstat(*ttymaster, st)  0)
+goto cleanup;
+
+if (lxcGetTtyGid(gid)  0)
+goto cleanup;
+
+uid = getuid();
+if (st.st_uid != uid || st.st_gid != gid)
+if (fchown(*ttymaster, uid, gid)  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;
+
+if (VIR_ALLOC_N(*ttyName, PATH_MAX)  0) {
+errno = ENOMEM;
+goto cleanup;
+}
+
+snprintf(*ttyName, PATH_MAX, /dev/pts/%d, ptyno);
+
+rc = 0;
+
+cleanup:
+if (rc != 0)
+VIR_FORCE_CLOSE(*ttymaster);
+
+return rc;
+}
+
+static int
 lxcControllerRun(virDomainDefPtr def,
  unsigned int nveths,
  char **veths,
@@ -894,10 +978,7 @@ lxcControllerRun(virDomainDefPtr def,
 
 if (devptmx) {
 VIR_DEBUG(Opening tty on private %s, devptmx);
-if (virFileOpenTtyAt(devptmx,
- containerPty,
- containerPtyPath,
- 0)  0) {
+if (lxcCreateTty(devptmx, containerPty, containerPtyPath)  0) {
 virReportSystemError(errno, %s,
  _(Failed to allocate tty));
 goto cleanup;
-- 
1.7.5.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2011-10-17 Thread Eric Blake 

On 10/17/2011 01:04 PM, Serge E. Hallyn wrote:

The glibc ones cannot handle ptys opened in a devpts not mounted at
/dev/pts.

Changelog:
   Oct 17: Per Eli Qiao, use VIR_ALLOC_N when appropriate, make
   sure to check return values, and follow coding style
  convention.
  Change lxcGetTtyGid() to return -1 on error, otherwise
  return gid in passed-in argument.

Signed-off-by: Serge Hallynserge.hal...@canonical.com
---
  src/lxc/lxc_controller.c |   89 +++--
  1 files changed, 85 insertions(+), 4 deletions(-)




@@ -781,6 +783,88 @@ static int lxcSetPersonality(virDomainDefPtr def)
  #endif

  static int
+lxcGetTtyGid(int *gid) {
+char *grtmpbuf;
+struct group grbuf;
+size_t grbuflen = sysconf(_SC_GETGR_R_SIZE_MAX);
+struct group *p;
+int ret;
+gid_t tty_gid = -1;


Hmm.  This gets called once per lxc guest, instead of once per libvirtd 
process, even though the gid won't change in the meantime.


Furthermore, we have _already_ hardcoded this to 5, based on the options 
we gave to mount(devpts) - either we need to fix that mount call 
(better), or we can skip this function altogether (assuming that our 
hard-coding of 5 is correct, there's no need to requery it, although 
that does sound like a worse solution).  For that matter, the whole 
point of the mount(devpts,...,gid=5) designation is that the new pty 
will be owned by gid 5, without needing to fchown() it.  Are there 
kernels that are new enough to support newinstance mounting, yet old 
enough to not honor gid=5?  That would be the only case where we would 
have to chown in the first place.  But if all kernels new enough to 
support newinstance mounting correctly honor the gid=5 option, then we 
don't even have to do chown() calls [but we still have to fix the 
hard-coding of gid=5 in the mount() option].




+if (fstat(*ttymaster,st)  0)
+goto cleanup;
+
+if (lxcGetTtyGid(gid)  0)
+goto cleanup;
+
+uid = getuid();
+if (st.st_uid != uid || st.st_gid != gid)
+if (fchown(*ttymaster, uid, gid)  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;


Likewise, I think this fchmod() is useless; the mode=0620 in the mount 
option should have already set this up.



+
+if (VIR_ALLOC_N(*ttyName, PATH_MAX)  0) {
+errno = ENOMEM;
+goto cleanup;
+}


Wasteful.  PATH_MAX is MUCH bigger than we need.


+
+snprintf(*ttyName, PATH_MAX, /dev/pts/%d, ptyno);


Instead, I'd just do this as:

virAsprintf(ttyName, /dev/pts/%d, ptyno);

Also, do we want this to be the name of the pty, _as seen from the guest 
after the fs pivot is complete_, or do we want this to be the name of 
the pty, as seen from the host prior to the pivot, in which case it 
would be some derivative of %s/dev/pts/%d, root-src, ptyno?


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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2011-10-17 Thread Serge E. Hallyn 
Quoting Eric Blake (ebl...@redhat.com):
 On 10/17/2011 01:04 PM, Serge E. Hallyn wrote:
 The glibc ones cannot handle ptys opened in a devpts not mounted at
 /dev/pts.
 
 Changelog:
Oct 17: Per Eli Qiao, use VIR_ALLOC_N when appropriate, make
sure to check return values, and follow coding style
convention.
Change lxcGetTtyGid() to return -1 on error, otherwise
return gid in passed-in argument.
 
 Signed-off-by: Serge Hallynserge.hal...@canonical.com
 ---
   src/lxc/lxc_controller.c |   89 
  +++--
   1 files changed, 85 insertions(+), 4 deletions(-)
 
 
 @@ -781,6 +783,88 @@ static int lxcSetPersonality(virDomainDefPtr def)
   #endif
 
   static int
 +lxcGetTtyGid(int *gid) {
 +char *grtmpbuf;
 +struct group grbuf;
 +size_t grbuflen = sysconf(_SC_GETGR_R_SIZE_MAX);
 +struct group *p;
 +int ret;
 +gid_t tty_gid = -1;
 
 Hmm.  This gets called once per lxc guest, instead of once per
 libvirtd process, even though the gid won't change in the meantime.
 
 Furthermore, we have _already_ hardcoded this to 5, based on the
 options we gave to mount(devpts) - either we need to fix that
 mount call (better), or we can skip this function altogether
 (assuming that our hard-coding of 5 is correct, there's no need to
 requery it, although that does sound like a worse solution).  For
 that matter, the whole point of the mount(devpts,...,gid=5)
 designation is that the new pty will be owned by gid 5, without
 needing to fchown() it.  Are there kernels that are new enough to
 support newinstance mounting, yet old enough to not honor gid=5?

No.  Mode and gid precede multiple devpts instances.

 That would be the only case where we would have to chown in the
 first place.  But if all kernels new enough to support newinstance
 mounting correctly honor the gid=5 option, then we don't even have
 to do chown() calls [but we still have to fix the hard-coding of
 gid=5 in the mount() option].

I missed something - why do we have to fix that?

It seems to me you're right - this is a linux-specific fn and
we are setting gid to 5 and the mode, we can skip this whole lxcGetTtyGid
function and the corresponding fchown and fchmod calls.  Nice!

 +if (fstat(*ttymaster,st)  0)
 +goto cleanup;
 +
 +if (lxcGetTtyGid(gid)  0)
 +goto cleanup;
 +
 +uid = getuid();
 +if (st.st_uid != uid || st.st_gid != gid)
 +if (fchown(*ttymaster, uid, gid)  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;
 
 Likewise, I think this fchmod() is useless; the mode=0620 in the
 mount option should have already set this up.

Yup.

 +
 +if (VIR_ALLOC_N(*ttyName, PATH_MAX)  0) {
 +errno = ENOMEM;
 +goto cleanup;
 +}
 
 Wasteful.  PATH_MAX is MUCH bigger than we need.

I thought so, but it was being done that way before, and didn't
seem all that important.

 +
 +snprintf(*ttyName, PATH_MAX, /dev/pts/%d, ptyno);
 
 Instead, I'd just do this as:
 
 virAsprintf(ttyName, /dev/pts/%d, ptyno);

Where virAsprintf will allocate when ttyName starts as null?

 Also, do we want this to be the name of the pty, _as seen from the
 guest after the fs pivot is complete_, or do we want this to be the
 name of the pty, as seen from the host prior to the pivot, in which
 case it would be some derivative of %s/dev/pts/%d, root-src,
 ptyno?

This gets passed into lxc_container which then prepends the chroot
location.  Don't know if there is any reason why we can't just
set it to the full name here, haven't looked further.

-serge

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2011-10-17 Thread Eric Blake 

On 10/17/2011 03:29 PM, Serge E. Hallyn wrote:

that matter, the whole point of the mount(devpts,...,gid=5)
designation is that the new pty will be owned by gid 5, without
needing to fchown() it.  Are there kernels that are new enough to
support newinstance mounting, yet old enough to not honor gid=5?


No.  Mode and gid precede multiple devpts instances.


Good to hear.  In other words, glibc has to worry about it when dealing 
with /dev/pts, because it caters to older kernels that lacked multiple 
devpts, but lxc can take shortcuts based on guarantees present by virtue 
of the existence of multiple instances.  But definitely some comments in 
the code are called for, explaining why we are taking these shortcuts.



[but we still have to fix the hard-coding of
gid=5 in the mount() option].


I missed something - why do we have to fix that?


We don't have to fix it now, but we should fix it someday.  There's 
nothing that says a distro has to map 'tty' to gid 5, and while most 
distros do that, we should instead be portable to compilation on a 
distro where 'tty' is gid 6 (or some other unusual number).



Instead, I'd just do this as:

virAsprintf(ttyName, /dev/pts/%d, ptyno);


Where virAsprintf will allocate when ttyName starts as null?


Yep - the whole point of virAsprintf is to allocate the string on your 
behalf, and to gracefully ensure that ttyName is NULL on allocation 
failure - much more compact than using snprintf yourself, and avoids the 
waste of a PATH_MAX allocation.


And, while you are at it, what about also fixing src/util/util.c to 
remove virFileOpenTtyAt (now that no one calls that), by moving its body 
into virFileOpenTty except for using posix_openpt instead of 
open(/dev/ptmx).


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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2011-10-14 Thread Serge Hallyn 
The glibc ones cannot handle ptys opened in a devpts not mounted at
/dev/pts.

Signed-off-by: Serge Hallyn serge.hal...@canonical.com
---
 src/lxc/lxc_controller.c |   82 +++--
 1 files changed, 78 insertions(+), 4 deletions(-)

diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
index 89ce7f5..d79f5ba 100644
--- a/src/lxc/lxc_controller.c
+++ b/src/lxc/lxc_controller.c
@@ -41,6 +41,8 @@
 #include locale.h
 #include linux/loop.h
 #include dirent.h
+#include grp.h
+#include sys/stat.h
 
 #if HAVE_CAPNG
 # include cap-ng.h
@@ -781,6 +783,81 @@ static int lxcSetPersonality(virDomainDefPtr def)
 #endif
 
 static int
+lxcGetTtyGid(void) {
+char *grtmpbuf;
+struct group grbuf;
+size_t grbuflen = sysconf(_SC_GETGR_R_SIZE_MAX);
+struct group *p;
+int ret;
+gid_t tty_gid = -1;
+
+/* Get the group ID of the special `tty' group.  */
+if (grbuflen == (size_t) -1L)
+/* `sysconf' does not support _SC_GETGR_R_SIZE_MAX.
+   Try a moderate value.  */
+grbuflen = 1024;
+
+grtmpbuf = (char *) alloca (grbuflen);
+ret = getgrnam_r(tty, grbuf, grtmpbuf, grbuflen, p);
+if (ret || p != NULL)
+tty_gid = p-gr_gid;
+
+return tty_gid == -1 ? getgid() : tty_gid;
+}
+
+#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;
+int ret;
+int ptyno;
+uid_t uid;
+gid_t gid;
+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;
+
+ret = ioctl(*ttymaster, TIOCGPTN, ptyno);
+if (ret  0)
+goto cleanup;
+
+ret = fstat(*ttymaster, st);
+
+uid = getuid();
+gid = lxcGetTtyGid();
+if (st.st_uid != uid || st.st_gid != gid)
+if (fchown(*ttymaster, uid, gid)  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;
+
+if (VIR_ALLOC_N(*ttyName, PATH_MAX)  0) {
+errno = ENOMEM;
+goto cleanup;
+}
+
+snprintf(*ttyName, PATH_MAX, /dev/pts/%d, ptyno);
+
+rc = 0;
+
+cleanup:
+if (rc != 0)
+VIR_FORCE_CLOSE(*ttymaster);
+
+return rc;
+}
+
+static int
 lxcControllerRun(virDomainDefPtr def,
  unsigned int nveths,
  char **veths,
@@ -894,10 +971,7 @@ lxcControllerRun(virDomainDefPtr def,
 
 if (devptmx) {
 VIR_DEBUG(Opening tty on private %s, devptmx);
-if (virFileOpenTtyAt(devptmx,
- containerPty,
- containerPtyPath,
- 0)  0) {
+if (lxcCreateTty(devptmx, containerPty, containerPtyPath)  0) {
 virReportSystemError(errno, %s,
  _(Failed to allocate tty));
 goto cleanup;
-- 
1.7.5.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list