svn commit: r311952 - head/sys/ddb

[Julian Elischer]

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
>