Re: [RFC][PATCH 6/6]: /dev/tty tweak in init_dev()

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org]

Serge E. Hallyn [serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org] wrote:
| Quoting sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org (sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
| > 
| > From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
| > Subject: [RFC][PATCH 6/6]: /dev/tty tweak in init_dev()
| > 
| > When opening /dev/tty, __tty_open() finds the tty using get_current_tty().
| > When __tty_open() calls init_dev(), init_dev() tries to 'find' the tty
| > again from devpts.  Is that really necessary ?

How about I change the second sentence to:

	When __tty_open() later calls init_dev() it passes in this
	'current-tty' to _tty_open() in *ret_tty. init_dev() ignores
	this and calls devpts again to find the tty.
| > 
| > The problem with asking devpts again is that with multiple mounts, devpts
| > cannot find the tty without knowing the specific mount instance. We can't
| > find the mount instance of devpts, since the inode of /dev/tty is in a
| > different filesystem.
| > 
| > ---
| >  drivers/char/tty_io.c |    5 ++++-
| >  1 file changed, 4 insertions(+), 1 deletion(-)
| > 
| > Index: linux-2.6.26-rc8-mm1/drivers/char/tty_io.c
| > ===================================================================
| > --- linux-2.6.26-rc8-mm1.orig/drivers/char/tty_io.c	2008-08-04 17:25:20.000000000 -0700
| > +++ linux-2.6.26-rc8-mm1/drivers/char/tty_io.c	2008-08-04 17:26:34.000000000 -0700
| > @@ -2066,7 +2066,10 @@ static int init_dev(struct tty_driver *d
| > 
| >  	/* check whether we're reopening an existing tty */
| >  	if (driver->flags & TTY_DRIVER_DEVPTS_MEM) {
| > -		tty = devpts_get_tty(inode, idx);
| > +		if (inode->i_rdev == MKDEV(TTYAUX_MAJOR, 0))
| > +			tty = *ret_tty;
| 
| I don't understand this.  ret_tty is for returning tty.  For instance in
| _ptmx_open() it's passed in completely uninitialized.  Isn't it
| dereferenced later in init_dev?

Yes, it is a quick/dirty fix.  When called from _ptmx_open() inode->i_rdev
would be [5,2] so, the uninitialized *ret_tty would not be used. 

Yes, it should be cleaner. I have been thinking of calling get_current_ty()
again here or renaming the parameter.

| 
| And what you're doing here doesn't really match your patch intro, but
| the patch intro doesn't really say what the patch does, it just lists a
| problem.  So I'm 100% unclear on what your intent here is.

I can update the intro as pointed above.  And will add a comment here.

The point is devpts does not have sufficient information to find the tty
struct for /dev/tty.  Fortunately, we already have the tty struct in this
case and don't need to ask devpts




| 
| > +		else
| > +			tty = devpts_get_tty(inode, idx);
| >  		/*
| >  		 * If we don't have a tty here on a slave open, it's because
| >  		 * the master already started the close process and there's