Date: Mon, 16 Jan 2017 13:29:24 +0800 From: Julian Elischer <julian@freebsd.org> To: Bruce Evans <brde@optusnet.com.au>, Adrian Chadd <adrian.chadd@gmail.com> Cc: Slawa Olhovchenkov <slw@zxy.spb.ru>, Julian Elischer <julian@elischer.org>, Mark Johnston <markj@freebsd.org>, "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: <9810b254-7d68-cb93-3766-9d943afae632@freebsd.org> In-Reply-To: <20170116130020.B1170@besplex.bde.org> 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> <20170116130020.B1170@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?9810b254-7d68-cb93-3766-9d943afae632>