From owner-freebsd-current@FreeBSD.ORG Sat Jun 19 10:59:23 2004 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 0365016A4CE for ; Sat, 19 Jun 2004 10:59:23 +0000 (GMT) Received: from mailout1.pacific.net.au (mailout1.pacific.net.au [61.8.0.84]) by mx1.FreeBSD.org (Postfix) with ESMTP id 5A55A43D4C for ; Sat, 19 Jun 2004 10:59:22 +0000 (GMT) (envelope-from bde@zeta.org.au) Received: from mailproxy1.pacific.net.au (mailproxy1.pacific.net.au [61.8.0.86])i5JAxL4u016792; Sat, 19 Jun 2004 20:59:21 +1000 Received: from gamplex.bde.org (katana.zip.com.au [61.8.7.246]) i5JAxCao004353; Sat, 19 Jun 2004 20:59:19 +1000 Date: Sat, 19 Jun 2004 20:59:11 +1000 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: Poul-Henning Kamp In-Reply-To: <61609.1087632958@critter.freebsd.dk> Message-ID: <20040619193901.W770@gamplex.bde.org> References: <61609.1087632958@critter.freebsd.dk> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII cc: current@freebsd.org Subject: Re: [REVIEW] move tty lock/initial up in the stack X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 19 Jun 2004 10:59:23 -0000 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