[Devel] [RFC rh7 v2] ve/tty: vt -- Implement per VE support for console and terminals

[Vladimir Davydov]

On Wed, Aug 05, 2015 at 02:59:50PM +0300, Cyrill Gorcunov wrote:
> On Wed, Aug 05, 2015 at 12:51:11PM +0300, Vladimir Davydov wrote:
> > > > Why do you add this functionality if it is not supposed to be used? IMO,
> > > > if you only allowed opening vtty-slave using ioctl, the implementation
> > > > would be much simpler.
> > > 
> > > The ability of accessing slave peers from inside of VE using cgroups
> > > mechanism has been an initial idea of all this code. It happened later
> > > when we deside to provide ioctls instead.
> > 
> > I'm a bit lost :-/ Whose was the idea? I thought the initial idea was to
> > port PCS6 stuff as is, but I started to review this code only after
> > several iterations had been done, so I might be completely wrong.
> 
> At the very first we wanted to move pcs6 code into pcs7 codebase
> but overall idea, as far as I remember, was to not use any specific
> code (ie our ioctl interfaces and such) until really needed. Thus
> in first iterations I didn't use ioctls at all, so any client
> can be accessing container's console, not only our prctl/vzctl tool.
> One simply may move client/reader into the VE cgroup and open vtty slave
> peer to fetch console data. It may be some loggin agent or whatever.

Moving a process to a VE cgroup just to read its tty... This makes me
sad :-(

> But instead now we using ioctl to open container's console and
> I think this is not the best option but let it be.

Why are you so opposed to ioctls? Why did we start to move every piece
of our user API we could to ve cgroup in the first place? What's the
point in substituting one private (not mainstream) API with another? Why
couldn't we left ve.iptables_mask, ve.features, ve.os_release, etc, etc
as ioctls? We wouldn't have to rewrite libvzctl as much as we have done
then. It's not a question solely to you. Everybody's done that (me too
:-), but may be there is a reason I don't know. If you know why ioctls
suck and ve cgroup rules, I'd really appreciate if you shared.

> 
> > > And no, implementation won't be anyhow simplier because you have to
> > > find out _which_ VE you're installing terminal from is currently
> > > active.
> > 
> > But you wouldn't have to implement the case when vtty slave (master in
> > PCS6) is looked up or installed then. Less branches, easier to read.
> 
> Hm? pcs6 does lookup over both masters and slave, see vtty_lookup in pcs6,
> it's not anyhow easier.

: static struct tty_struct *vtty_lookup(struct tty_driver *driver,
: 				      struct inode *inode, int idx)
: {
: 	struct tty_struct *tty;
: 	struct ve_struct *ve = get_exec_env();
: 
: 	BUG_ON(driver != vtty_driver);

        ^^^^^^
this

: static int vtty_install(struct tty_driver *driver, struct tty_struct *tty)
: {
: 	struct tty_struct *o_tty;
: 	int idx = tty->index;
: 	struct ve_struct *ve = get_exec_env();
: 
: 	BUG_ON(driver != vtty_driver);
        ^^^^^^

and this make code easier IMO. You have to (and do) handle both master
and slave cases here.

> 
> > > New terminal allocation is happening from inside of lookup/install helpers
> > > which have no other data under the hands than driver+index. Since we are
> > > going to rip off driver::ve, we can't longer rely on this.
> > > 
> > > Moreover, currently I'm testing vzctl console CTID index functionality
> > > and as you might imagine this routine is called from inside of ve0 context
> > > so I added additional helpers which allow to lookup into specified VEs
> > > map, instead of get_exec_env.
> > > 
> > > > BTW, a side question. In PCS6 /dev/console = vtty slave, while in your
> > > > code it is master. Why did you decide to reverse things? Why not just
> > > > port what we have in PCS6?
> > > 
> > > Actually I don't understand why it's a slave peers assigned to /dev/console
> > > and /dev/tty's. Maybe it was done for some historical reason? I don't know
> > > the background. The vtty code is close to pty code in pcs6 and I presume
> > > some ideas have been taken: in particular several "readers/writers" might
> > > be opening console and ttys, but in terms of pty code only one "master"
> > > is allowed (iow to make a pair one have to open ptmx device, next open
> > > of the same device cause new pair to generate). So I assume to be somehow
> > > close to the ideas of unix98 terminals we provided "slave" peers to emulate
> > > console and ttys, because slave peers may be opened several times.
> > > 
> > > But I decided to make it straight -- there is many to many mapping:
> > 
> > Sorry, but it is not straight, it is screwed up IMO. If we used Unix98
> > ptys (which is what we would do if we were not so lazy), we would have
> > pty slave open by a container (/dev/console) and the master open and
> > read/flushed by a dispatcher thread. This is how it works in LXC AFAIK.
> > Now, we don't want to patch the dispatcher, so we want to make the
> > master throw out data automatically if it is not open.
> 
> I was about to write a long description why I think it's not
> that good, but I see what you mean. Letme try to reverse the
> approach and implement pcs6 1:1 behaviour with as much pcs6
> code fetched as I could (I already tried but didn't success
> can't remember which exactly problems I met).

It'd be great *IMO*, but I might be wrong, you know. Unfortunately
nobody except two of us seem to care about this :-(

> > > >> +static int __init vtty_init(void)
> > > >> +{
> > > >> +#define TTY_DRIVER_ALLOC_FLAGS			\
> > > > 
> > > > Please zap this macro and pass the flags as is.
> > > 
> > > I pass them into two place, so no, not going to do that:
> > 
> > Please submit a patch upstream that would do the same in
> > unix98_pty_init() and legacy_pty_init().
> 
> Sole cleanup patches on its own are not welcome
> and you know this.

Yeah, I occasionally act as a fat troll, sorry :-) If you do feel a
strong affection to this macro, then please rename it to
VTTY_DRIVER_ALLOC_FLAGS, so that it won't occasionally conflict with
something tty-related.

Thanks.

> 
> > > if I need a new flag I'll update one macro instead of
> > > walking the code and update two places.
>