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

[Cyrill Gorcunov]

On Thu, Aug 27, 2015 at 06:00:28PM +0300, Vladimir Davydov wrote:
> > +
> > +static struct file_operations vtty_fops;
> > +
> > +#define vtty_match_index(idx)		((idx) >= 0 && (idx) < MAX_NR_VTTY_CONSOLES)
> 
> nit: this line is longer than 80 symbols

yes, but recall that there is no 80 syms limit. The last discussion/flame
over line length I saw was calmed down with agreemen that 90 symbols are
fine. Wonder, do you really still sit on 80 chars terminal?

(sure I'll change this code all over the place if you prefer)

> > +static void vtty_map_set(vtty_map_t *map, struct tty_struct *tty)
> > +{
> > +	lockdep_assert_held(&tty_mutex);
> 
> nit: WARN_ON(map->vttys[tty->index] != NULL)

ok

> > +static void vtty_map_del(vtty_map_t *map, struct tty_struct *tty)
> > +{
> > +	lockdep_assert_held(&tty_mutex);
> > +	if (map) {
> 
> nit: WARN_ON(map->vttys[tty->index] !=
> 	     (tty->driver == ttys_driver ? tty : tty->link))
> 

ok

> > +
> > +static void vtty_set_context(envid_t veid)
> > +{
> 
> nit: WARN_ON(veid)

I believe you meant veid == 0

> > +
> > +static struct tty_struct *vtty_lookup(struct tty_driver *driver,
> > +				      struct inode *inode, int idx)
> > +{
> > +	vtty_map_t *map = vtty_map_lookup(vtty_get_context());
> > +	struct tty_struct *tty;
> > +
> > +	WARN_ON_ONCE((driver != vttym_driver) &&
> > +		     (driver != vttys_driver));
> 
> nit: this check is rather useless

actually not, if we ever end up in this warning
it means some external caller messed with maj/min
match. at first I thought putting BUG_ON here.
otoh, fine, will drop it ;)

> > +
> > +static int vtty_open(struct tty_struct *tty, struct file *filp)
> > +{
> > +	if (test_bit(TTY_OTHER_CLOSED, &tty->flags))
> > +		return -EIO;
> 
> What's the point in this check? You only set TTY_OTHER_CLOSED when both
> ends are closed and hence the pair is dead.
> 
> > +
> > +	clear_bit(TTY_IO_ERROR, &tty->flags);
> > +	clear_bit(TTY_OTHER_CLOSED, &tty->link->flags);
> 
> Again, you only set TTY_IO_ERROR and TTY_OTHER_CLOSED if the pair is
> dead and therefore cannot be reopened, so I think you don't need to
> touch these bits at all.

Seems so. It;s harmless but letme recheck, I've had something
in mind (need either add a comment about it either drop)

> 
> > +	set_bit(TTY_THROTTLED, &tty->flags);
> > +	return 0;
> > +}
> > +
> > +static void vtty_close(struct tty_struct *tty, struct file *filp)
> > +{
> > +	struct tty_struct *peer = tty->link;
> > +	vtty_map_t *map = tty->driver_data;
> > +
> > +	spin_lock(&map->close_lock);
> 
> This spinlock doesn't protect us from concurrent modifications of
> peer->count from e.g. tty_reopen. What about taking tty_mutex instead?
> 
> On the second thought, this wouldn't help us if we patched mainstream
> kernel, because tty_mutex is not held for tty->count modifications there
> anymore. That said, if we simply substitute the spinlock with tty_mutex,
> we will surely have problems sooner or later...
> 
> May be, we'd better move our ->count tweaking to the generic path, i.e.
> tty_release, similarly to pty? We could distinguish vtty from pty by
> checking TTY_EXTRA_REF. What do you think?

you know, i really don't wanna put our code into generic one, its gonna
be hard to support. maybe we should lock the peer manually inside
our close routine, so in this way we would have to take own lock, then
take peer lock under it, which would protect from abba deadlock. Hm?

vtty_close
	spin_lock(&map->close_lock);
	tty_lock(tty->link);
	...
	tty_unlock(tty->link);
	spin_unlock(&map->close_lock);

and instead of spinlock I would use mutex here. Won't this work?

...
> > +
> > +static const struct tty_operations vtty_ops = {
> > +	.lookup		= vtty_lookup,
> > +	.install	= vtty_install,
> > +	.open		= vtty_open,
> > +	.close		= vtty_close,
> > +	.shutdown	= vtty_shutdown,
> > +	.cleanup	= pty_cleanup,
> > +	.write		= vtty_write,
> > +	.write_room	= vtty_write_room,
> > +	.set_termios	= pty_set_termios,
> > +	.unthrottle	= pty_unthrottle,
> 
> > +	.remove		= pty_unix98_remove,
> 
> nit: I wouldn't reuse this one; it's empty, so we it shouldn't be very
> difficult to implement our own vtty_remove; the benefit is that we can
> drop dependency from UNIX98_PTYS

ok

> > +
> > +	tty = vtty_lookup(vttym_driver, NULL, idx);
> > +	if (!tty ||
> > +	    (test_bit(TTY_CLOSING, &tty->flags) ||
> > +	     test_bit(TTY_CLOSING, &tty->link->flags))) {
> > +		/*
> > +		 * The previous connection is about to
> > +		 * be closed so drop it from the map and
> > +		 * allocate a new one.
> > +		 */
> > +		if (tty)
> > +			vtty_map_del(tty->driver_data, tty);
> > +		tty = tty_init_dev(vttys_driver, idx);
> 
> So here you install a new tty slave in place of the closing one, right?

yeah

> If ->shutdown is called for the old tty after this point, you'll get
> empty map. This has to be fixed. Checking that we're actually clearing
> our own tty in vtty_map_del should do the trick.

I simply need to drop the tty->driver_data here, seems this
hunk lost during patch series folding, thanks!