Date: Sat, 19 Jun 2004 20:59:11 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Poul-Henning Kamp <phk@phk.freebsd.dk> Cc: current@freebsd.org Subject: Re: [REVIEW] move tty lock/initial up in the stack Message-ID: <20040619193901.W770@gamplex.bde.org> In-Reply-To: <61609.1087632958@critter.freebsd.dk> References: <61609.1087632958@critter.freebsd.dk>
next in thread | previous in thread | raw e-mail | index | archive | help
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
> @@ -41,6 +41,7 @@
> .Nm
> .Op Fl a | Fl e | Fl g
> .Op Fl f Ar file
> +.Op Fl i | Fl l
> .Op operands
> .Sh DESCRIPTION
> The
> @@ -83,6 +84,12 @@
> .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
> ...
> @@ -91,7 +101,7 @@
> 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");
> @@ -144,7 +154,7 @@
> 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
> ...
> @@ -242,14 +239,6 @@
>
> 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.
> ...
> @@ -387,20 +376,18 @@
> if (com == NULL)
> return (ENXIO);
>
> + tp = com->tp;
Style bug (block comment no longer separated from previous code by a
blank line).
> ...
> @@ -964,28 +952,28 @@
> 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*).
> ...
> @@ -2027,8 +1964,7 @@
> 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
> @@ -769,6 +769,8 @@
> 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:
> @@ -1129,6 +1131,24 @@
> 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
> @@ -94,6 +94,8 @@
> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040619193901.W770>