Re: [REVIEW] move tty lock/initial up in the stack

On Sat, 19 Jun 2004, Poul-Henning Kamp wrote:

> This patch moves the "lock/initial" facility known from sio(4) up
> to the generic tty layer.

Just moving them is OK.  Unfortunately, the patch does less than move
them ...

> It adds two new flags to stty(1): -i and -l to manipulate the initial
> and lock states and eliminates the tty[il]d# and cua[il]a# devices.

... starting here.  Control devices in general must be separate so that
they can have different ownership and permissions, and can be opened
without side effects.  For sio devices, the non-control devices can't
even be opened if the complementary non-control device is open.

> Index: bin/stty/stty.1
> ===================================================================
> RCS file: /home/ncvs/src/bin/stty/stty.1,v
> retrieving revision 1.28
> diff -u -r1.28 stty.1
> --- bin/stty/stty.1	6 Apr 2004 20:06:53 -0000	1.28
> +++ bin/stty/stty.1	18 Jun 2004 11:43:40 -0000
> _at__at_ -41,6 +41,7 _at__at_
>  .Nm
>  .Op Fl a | Fl e | Fl g
>  .Op Fl f Ar file
> +.Op Fl i | Fl l
>  .Op operands
>  .Sh DESCRIPTION
>  The
> _at__at_ -83,6 +84,12 _at__at_
>  .Nm
>  to restore the current terminal state as per
>  .St -p1003.2 .
> +.It Fl i
> +Operate on the "initial" settings which apply on first open.
> +.It Fl l
> +Operate on the "lock" settings.

Markup bugs (hard quotes).

> +Please note that the lock settings act as flags, the bits set here
> +indicate which parts of the initial settings it is impossible to change.

Bad grammar (comma splice and a couple of others).

>  .El
>  .Pp
>  The following arguments are available to set the terminal
> Index: bin/stty/stty.c
> ===================================================================
> RCS file: /home/ncvs/src/bin/stty/stty.c,v
> retrieving revision 1.22
> diff -u -r1.22 stty.c
> --- bin/stty/stty.c	6 Apr 2004 20:06:53 -0000	1.22
> +++ bin/stty/stty.c	18 Jun 2004 11:36:57 -0000
> ...
> _at__at_ -91,7 +101,7 _at__at_
>  args:	argc -= optind;
>  	argv += optind;
>
> -	if (tcgetattr(i.fd, &i.t) < 0)
> +	if (ioctl(i.fd, iget, &i.t) < 0)

Too hackish.  Using tcgetattr() was an advance on using ioctl().

>  		errx(1, "stdin isn't a terminal");
>  	if (ioctl(i.fd, TIOCGETD, &i.ldisc) < 0)
>  		err(1, "TIOCGETD");
> _at__at_ -144,7 +154,7 _at__at_
>  		usage();
>  	}
>
> -	if (i.set && tcsetattr(i.fd, 0, &i.t) < 0)
> +	if (i.set && ioctl(i.fd, iset, &i.t) < 0)
>  		err(1, "tcsetattr");

Too hackish as above, and error message no longer matches the code.

>  	if (i.wset && ioctl(i.fd, TIOCSWINSZ, &i.win) < 0)
>  		warn("TIOCSWINSZ");
> Index: sys/dev/sio/sio.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/dev/sio/sio.c,v
> retrieving revision 1.438
> diff -u -r1.438 sio.c
> --- sys/dev/sio/sio.c	16 Jun 2004 09:46:56 -0000	1.438
> +++ sys/dev/sio/sio.c	18 Jun 2004 13:06:42 -0000
> ...
> _at__at_ -242,14 +239,6 _at__at_
>
>  	struct tty	*tp;	/* cross reference */
>
> -	/* Initial state. */
> -	struct termios	it_in;	/* should be in struct tty */
> -	struct termios	it_out;
> -
> -	/* Lock state. */
> -	struct termios	lt_in;	/* should be in struct tty */
> -	struct termios	lt_out;
> -

Note that there are separate initial and lock states for the cua and the
tty devices.  The changes break this.

> ...
> _at__at_ -387,20 +376,18 _at__at_
>  	if (com == NULL)
>  		return (ENXIO);
>
> +	tp = com->tp;

Style bug (block comment no longer separated from previous code by a
blank line).

> ...
> _at__at_ -964,28 +952,28 _at__at_
>  		rclk = DEFAULT_RCLK;
>  	com->rclk = rclk;
>
> +	tp = com->tp = ttymalloc(NULL);

Style bug (as above).

Memory leak.  com->tp is not freed if the attach fails.  Neither is com
(*blush*).

> ...
> _at__at_ -2027,8 +1964,7 _at__at_
>  	if (cmd == TIOCSETA || cmd == TIOCSETAW || cmd == TIOCSETAF) {
>  		int	cc;
>  		struct termios *dt = (struct termios *)data;
> -		struct termios *lt = mynor & CALLOUT_MASK
> -				     ? &com->lt_out : &com->lt_in;
> +		struct termios *lt = &tp->t_lock;
>
>  		dt->c_iflag = (tp->t_iflag & lt->c_iflag)
>  			      | (dt->c_iflag & ~lt->c_iflag);

This is missing moving the lock state handling, which comprises about half
of the code that can be moved.

> ...
> Index: sys/kern/tty.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/kern/tty.c,v
> retrieving revision 1.219
> diff -u -r1.219 tty.c
> --- sys/kern/tty.c	16 Jun 2004 09:47:12 -0000	1.219
> +++ sys/kern/tty.c	18 Jun 2004 11:48:41 -0000
> _at__at_ -769,6 +769,8 _at__at_
>  	case  TIOCSTI:
>  	case  TIOCSTOP:
>  	case  TIOCSWINSZ:
> +	case  TIOCSETAI:
> +	case  TIOCSETAL:

Insertion sort errors.

The new cases don't belong in this case statement anyway, since the new
ioctls don't involve modification of the active part of the tty struct.

>  #if defined(COMPAT_43)
>  	case  TIOCLBIC:
>  	case  TIOCLBIS:
> _at__at_ -1129,6 +1131,24 _at__at_
>  	case TIOCGDRAINWAIT:
>  		*(int *)data = tp->t_timeout / hz;
>  		break;
> +	case TIOCSETAI:
> +		error = suser(td);
> +		if (error)
> +			return (error);
> +		tp->t_init = *(struct termios *)data;
> +		break;
> +	case TIOCGETAI:
> +		*(struct termios *)data = tp->t_init;
> +		break;
> +	case TIOCSETAL:
> +		error = suser(td);
> +		if (error)
> +			return (error);
> +		tp->t_lock = *(struct termios *)data;
> +		break;
> +	case TIOCGETAL:
> +		*(struct termios *)data = tp->t_lock;
> +		break;

Insertion sort errors.

>  	default:
>  #if defined(COMPAT_43)
>  		return (ttcompat(tp, cmd, data, flag));
> Index: sys/sys/tty.h
> ===================================================================
> RCS file: /home/ncvs/src/sys/sys/tty.h,v
> retrieving revision 1.81
> diff -u -r1.81 tty.h
> --- sys/sys/tty.h	17 Jun 2004 17:16:53 -0000	1.81
> +++ sys/sys/tty.h	18 Jun 2004 11:42:43 -0000
> _at__at_ -94,6 +94,8 _at__at_
>  	struct	selinfo t_rsel;		/* Tty read/oob select. */
>  	struct	selinfo t_wsel;		/* Tty write select. */
>  	struct	termios t_termios;	/* Termios state. */
> +	struct	termios t_init;		/* Initial termios state. */
> +	struct	termios t_lock;		/* Locked termios state. */

Needs to be doubled.  See above.

The new fields could be better named.  There is nothing in their name to
suggests that they are for termios.  The sio names it_* and lt_* are
abbreviations of initial_termios_* and lock_termios_*.

>  	struct	winsize t_winsize;	/* Window size. */
>  					/* Start output. */
>  	void	(*t_oproc)(struct tty *);

Bruce