Re: [libvirt] [PATCH 1/1] lxc: use our own hand-rolled code in place of unlockpt and grantpt (v2)
Quoting Eric Blake (eblake(a)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(a)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