On 16/01/2017 11:53 AM, Bruce Evans wrote:
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:
review it in the review page and just send a link in the email..
that's what it's for.
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
_______________________________________________
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"