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

From: Bruce Evans (bde_at_zeta.org.au)
Date: 06/19/04

  • Next message: Divacky Roman: "Re: strange message appearing in a fresh current" 
    Date: Sat, 19 Jun 2004 20:59:11 +1000 (EST)
To: Poul-Henning Kamp <phk@phk.freebsd.dk>

    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
    _______________________________________________
    freebsd-current@freebsd.org mailing list
    http://lists.freebsd.org/mailman/listinfo/freebsd-current
    To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

  • Next message: Divacky Roman: "Re: strange message appearing in a fresh current"

    Relevant Pages