Date: Mon, 16 Jan 2017 14:53:26 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Adrian Chadd <adrian.chadd@gmail.com> Cc: Slawa Olhovchenkov <slw@zxy.spb.ru>, Julian Elischer <julian@elischer.org>, Mark Johnston <markj@freebsd.org>, Bruce Evans <brde@optusnet.com.au>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>, rang@acm.org, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org> Subject: Re: svn commit: r311952 - head/sys/ddb Message-ID: <20170116130020.B1170@besplex.bde.org> In-Reply-To: <CAJ-Vmon%2B9atqnnD0d%2Bg%2BRQBhE8jKqwteadPidzUGHu_dwdoFJQ@mail.gmail.com> References: <201701120022.v0C0MaHq053076@repo.freebsd.org> <20170113131948.P1465@besplex.bde.org> <20170114220629.GB18065@raichu> <1755552c-a5e2-848a-38e2-3bacfbecfb23@elischer.org> <20170115144531.GB58505@zxy.spb.ru> <CAJ-VmoniRcFEO8QzCd97=3bCzoiKdRgQFT6oNuNR_K=TPFm-VQ@mail.gmail.com> <20170115201758.GA78888@zxy.spb.ru> <CAJ-Vmon%2B9atqnnD0d%2Bg%2BRQBhE8jKqwteadPidzUGHu_dwdoFJQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170116130020.B1170>