svn commit: r311952 - head/sys/ddb

[Bruce Evans]

On Sun, 15 Jan 2017, Adrian Chadd wrote:

> hah, i took this, then life got in the way. :)
>
> Bruce - what do you think of
> https://bz-attachments.freebsd.org/attachment.cgi?id=138881 ?

Also https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=184987 .

I think it is hard to read and review since it is not in the mail.
After fetching it and quoting it:

X diff -r 91820d9948e0 -r 13f40d577646 sys/kern/tty_ttydisc.c
X --- a/sys/kern/tty_ttydisc.c	Fri Feb 22 09:45:32 2013 +0100
X +++ b/sys/kern/tty_ttydisc.c	Tue Dec 17 23:03:17 2013 +0100
X @@ -448,6 +448,9 @@
X  	if (tp->t_flags & TF_ZOMBIE)
X  		return (EIO);
X 
X +	if (tp->t_termios.c_lflag & FLUSHO)
X +		return (0);
X +

This seems to be far too simple.  As pointed out in the PR thread, it
is missing at least the clearing of uio_resid.

    (BTW, returns for TF_ZOMBIE and IO_NDELAY tend to have the opposite
    problem.  The tend to reduce uio_resid to count i/o that they have
    not done.  They should just return an error like the above return
    for TF_ZOMBIE does.  Upper layers are supposed to translate this
    error to success if any i/o has been done.  Upper layers often get
    this wrong, and my fixes currently do too much compensation in lower
    layers.)

The old tty driver has squillions of FLUSHO checks for output not involving
uio.  Mostly for echo of input.  However, any input (and other operations?)
normally turns of FLUSHO, so this is hard to see.

To see if this works compatibly now, try typing 111^O2.  ^O should be
echoed as ^O^R followed by reprinting the line according to the implicit
VREPRINT (^R).  This should cancel FLUSHO, so the 2 is just echoed on
the new line.  However ^O instead of 2 should just cancel FLUSHO.  So
^O^O should be equivalent to ^R except it prints ^O before ^R.

It is wrong for FLUSHO to have any effect except in modes where VDISCARD
has an effect.  VDISCARD seems to be conditional on precisely
!EXTPROC && IEXTEN.  Its inactivity in other modes should be implemented
by forcing it off on certain mode changes.  The old tty driver forces it
off in squillions of places, perhaps including all mode changes for
simplicity.  There would be a problem if stty(1) could actually set it,
since then invalid settings like "raw flusho" would be allowed.  Clearing
it after all mode changes would prevent stty actually setting it.


X  	/*
X  	 * We don't need to check whether the process is the foreground
X  	 * process group or if we have a carrier. This is already done
X @@ -881,6 +884,14 @@
X 
X  	/* Special control characters that are implementation dependent. */
X  	if (CMP_FLAG(l, IEXTEN)) {
X +		/* Discard (^O) */

Style bug.  This comment is missing a "." and does less than echo the code.
The code doesn't hard-code VDISCARD as ^O, and it is unclear whether the
comment applies to the previous or the next line (except it is further
from echoing the previous line).

X +		if (CMP_CC(VDISCARD, c)) {
X +			if (!(tp->t_termios.c_lflag & FLUSHO))

Style bug.  This file elsewhere always uses the funky style of explicit
comparison of (sets of ) flags with 0.

X +				ttyoutq_write_nofrag(&tp->t_outq, "^O", 2);

This also hard-codes ^O.  The old driver echos the actual discard character
using ttyecho(c, tp).  I don't know if the funky rules for quoting control
characters apply in this case, but ttyecho() has to be quite complicated to
handle general cases.

X +			tp->t_termios.c_lflag ^= FLUSHO;

The old driver adds an implicit VREPRINT char here if the input queue is
nonempty.  This usually results in echoing ^O^R and reprinting the line.

X +			return(0);
X +		}
X +

Otherwise, the action here is identical with the old driver.

X  		/* Accept the next character as literal. */
X  		if (CMP_CC(VLNEXT, c)) {
X  			if (CMP_FLAG(l, ECHO)) {

The old driver needs 23 lines mentioning FLUSHO to keep it mostly clear.
These are:
- 3 in compat code (still there)
- 3 here
- 1 clear whenever restarting output at the end of interrupt input (most
   cases interrupt input)
- 4 checks in ttyoutput() which is used manly for echoing.  1 at the
   beginning would be simpler, but the checks are more or less before
   each putc() and there are many inconsistences for counting characters
   and the column position from the complications
- 1 clear in ttioctl() in 1 case of turning 1 type of flow control back on.
   Buggy, since FLUSHO should not be affected by flow control except by the
   xon char being treated as an ordinary char for canceling VDISCARD.
- 1 clear in ttioctl() for the same flow control done by TIOCSTART
- 1 check for ordinary writes as in this patch
- 2 in comments
- 2 more checks in the loop for ordinary writes.  Some checks are necessary,
   but these are misplaced.  FLUSHO can change underneath when we unlock to
   do the i/o, so it must be re-checked when we wake up.  It is not checked
   in quite the right places in the old code, but at least the main check
   is placed at the top of the loop so it mostly works.  In the patch, the
   check is outside of the loop, so it is never checked after waking up.
- 1 clear at the start of ttyrub()
- 1 set and 1 clear in ttyrub() for TAB processing
- 1 clear in ttyecho() related to the TAB processing
- 1 other case.

So the main missing details are:
- clear FLUSHO for almost any input
- check FLUSHO in echo processing? (should the new input clear the flag
   before the echo?)
- missing uio handling for writing
- missing race handling for writing
- clear FLUSHO for most ioctls more than the old code did.  Clear it at
   least when changing the mode to one where VDISCARD doesn't apply.

Bruce