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

[Serge E. Hallyn]

Quoting Eric Blake (eblake at 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 Hallyn<serge.hallyn at 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