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

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