Quoting Eric Blake (eblake@xxxxxxxxxx): > 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@xxxxxxxxxxxxx> > >--- > > 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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list