From owner-svn-src-head@freebsd.org Mon Mar 6 09:21:24 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id A3EFFCFBD71; Mon, 6 Mar 2017 09:21:24 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail104.syd.optusnet.com.au (mail104.syd.optusnet.com.au [211.29.132.246]) by mx1.freebsd.org (Postfix) with ESMTP id 2B3EA1050; Mon, 6 Mar 2017 09:21:23 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c122-106-153-191.carlnfd1.nsw.optusnet.com.au [122.106.153.191]) by mail104.syd.optusnet.com.au (Postfix) with ESMTPS id 53A21428AAD; Mon, 6 Mar 2017 20:21:13 +1100 (AEDT) Date: Mon, 6 Mar 2017 20:21:12 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Ed Schouten cc: "Conrad E. Meyer" , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r314685 - head/bin/ps In-Reply-To: Message-ID: <20170306183546.E1414@besplex.bde.org> References: <201703042238.v24McAD8008837@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=VbSHBBh9 c=1 sm=1 tr=0 a=Tj3pCpwHnMupdyZSltBt7Q==:117 a=Tj3pCpwHnMupdyZSltBt7Q==:17 a=kj9zAlcOel0A:10 a=6I5d2MoRAAAA:8 a=Mimnt9xOvP8K_R8bOPwA:9 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Mar 2017 09:21:24 -0000 On Sun, 5 Mar 2017, Ed Schouten wrote: > 2017-03-05 0:08 GMT+01:00 Conrad Meyer : >>> - If something is a TTY, then our implementation of the TTY layer >>> guarantees that TIOCGWINSZ always works. >> >> Do you know if it did in 1990 too? It's hard to tell why Marc@ made >> this change way back then. I wasn't sure so I left it alone. > > I can't tell. :-) Actually, it is easy to tell. This has nothing to do with kernel bugs that prevent TIOCGWINSZ working. It is to find the user's terminal so that the correct width can be determined. The correct default width is always that of the user's terminal. The problem is that it isn't clear what the user's terminal is in redirected cases. As described in the log message, the default width was originally hard- coded as 80 (not even 79 to allow for autowrap). (BTW, althought the commit was remarkably diligent in describing the history, it is inconvenient to look up the details on the web, and the web pages are almost unreadable even after downloading them and demangling the formatting a bit.) (The commit was not diligent enough to track down the history of the man pages and whether they were consistent with the code. The original man page should have described the hard-coded default as being 80, and said how wrong this is in the BUGS section, including its off-by-1 error.) (The commit and perhaps the man page were also not diligent enough to describe the details of the overrides for the default. In the original version, one -w gave width 132 (another off-by-1-error), and more than 1 -w gave width BUFSIZ (the worst abuse of stdio's BUFSIZ mistake that I remember noticing).) Kirk changed the default to the terminal width given by TIOCWINSZ on stdin (spelled 0 instead of the current STDIN_FILENO). That is good for most uses. Users normally have all 3 std fd's connected to the same terminal, so TIOCWINSZ on any of them works. Plain ps goes to /dev/tty so any choice works. Normal redirection is ps | less and ps >foo; this doesn't redirect stdin, so TIOCWINSZ on stdin works. Hopefully he changed the man page to describe the new behaviour. The following bugs are visible in Kirk's patch: - the magic numbers 80 and 132 and the full terminal width still have the off-by-1 error - the magic numbers are more broken than before. The magic width of 80 is still abused as a flag to determine if -w has already been applied. This no longer works. If the terminal width is anything except 80, then the first -w gives width BUFSIZ with no intermediate step to 132, although such a step is more needed than before if the terminal width is < 80. This is fixed in -current by counting -w's. Next, marc@ improved the detection of user terminals. TIOCGWINSZ is tried on all 3 std fd's. This fixes the case where stdin is redirected but one of the other std fd's is not redirected (and none of the redirected fd's is to a terminal with unsuitable size). It would be unusual to redirect stdin, and not necessary for ps since ps doesn't read stdin. It might be done for invoking a script that uses ps for output but piped input for other utilities in the script. Not redirecting stderr together with stdin has even more uses. The order of the TIOCGWINSZ calls is subtle. marc@ kept stdin first, perhaps for compatibility. stdout is obviously more relevant than stderr so is checked next, then stderr. But since the output should normally be formatted for stdout, especially if stdout is a terminal, it is better to check stdout first. This was done soon atfer marc@'s change. The diligence in the commit wasn't remarkable enough to uncover the details. The order was improved to stdout first and stdin last by some change beteen marc@ and FreeBSD-1. FreeBSD also used the fancy spellings STD*_FILENO. I thought it was from Net/2 slightly earlier than 1990. ps.c has a 1990 Regents copyright but a 5.43 (Berkelely) 7/1/91 sccsid. marc@'s diff has a 1989 Regents copyright and 1.8 and 5.20 sccsids (obscured by the diff format, but clearly early). Counting for -w was still broken in FreeBSD-1, but was fixed in 4.4Lite1. The next fix in FreeBSD was to add COLUMNS support in 1997 (tjr r97804). According to the commit log, this is for SUSv3. This is now required by POSIX. POSIX also has considerable detail on allowable truncations of fields, but at least in 2001 doesn't have anything about the default total width. After that, all related changes discussed here are regressions. Apple places the !isatty(STDOUT_FILENO) check later than in this commit, so it isn't so clear what it does. When stdout is not a tty, the Apple change blows away the defaults set earlier,including the COLUMNS override specified by POSIX. In FreeBSD, isatty() is implemented using tcgetattr(), and tcgetattr() is implemented using ioctl(...TIOCGETA...). It would take a weird terminal to support TIOCGETA but not TIOCGWINSZ. Weird terminals (including initial and lock state devices?) tend to supply dummy values for both. Perhaps they shouldn't supply either, since they are not really terminals. Assume for simplicity that isatty() is true if TIOCGWINSZ succeeds. Then in Apple's version, the carefully ordered TIOCGWINSZ calls on stderr and stdout are dead code, since the result from them will be canceled if they were reached. The FreeBSD version is similar except it doesn't break COLUMNS or obfuscate the initialization by canceling it much later. After COLUMNS, it checks isatty(STDOUT_FILENO). Then under the simplifing assumption, if isatty(STDOUT_FILENO) is true, TIOCGWINSZ on stdout succeeds and the other 2 carefully ordered TIOCGWINSZ calls are unreachable (Apple reaches them but cancels them). Aother bug in the FreeBSD version is that it doesn't update the documentation. Actually, the documentation is fuzzy enough to not be affected by the change. It mainly says: X -w Use 132 columns to display information, instead of the default X which is your window size. If the -w option is specified more X than once, ps will use as many columns as necessary without X regard for your window size. Note that this option has no effect X if the ``command'' column is not the last column displayed. X X COLUMNS If set, specifies the user's preferred output width in X column positions. By default, ps attempts to automatically X determine the terminal width. The first error is the off-by-1 error in 131 and no mention of auto-wrap. Next, the the default width is fuzzily described, but not fuzzily enough to have no errors, exept for COLUMNS it is described as the "best automatic attempt". That the default when there is no terminal is 79 is not mentioned. In the description of -w, it is stated that the default is "your" window size, but as this mail shows it is unclear where this window is. I now remember noticing the caveat in the last sentence for -w. -w works reasonably for ps l, but not for fancier formats. COLUMNS must be broken for fancier formats too, since COLUMNS affects the width variable like -w. >>> - If we're only interested in testing stdout whether it's a TTY, I >>> think it makes little sense to check TIOCGWINSZ on stdin, stderr. This removes the code that was essentially killed by the commit, but it shouldn't have been killed. >>> I think there would therefore be very little harm to use something like this: >>> >>> | if ((cols = getenv("COLUMNS")) != NULL && *cols != '\0') >>> | termwidth = atoi(cols); >>> | else if ((ioctl(STDOUT_FILENO, TIOCGWINSZ, (char *)&ws) == >>> -1 && ws.ws_row == 0) >> >> Shouldn't this remain || ws.ws_row == 0? > > Err, yes. ||, not &&. When any of the 3 std fd's is a terminal, the fixes in 1990-1991 are clearly right. When none of them is a terminal, the default should have been UNLIMITED, but it has always been 79 or 80. ps users should know this, although it is undocumented. If you break this, then you also should break -w and -ww and the 131-column in-beteen value that they give. 131 is just a convenient value between 79 and UNLIMITED. Changing this would break all scripts that assume the undocumented 79 or use -w to get the documented 131. I use ps in scripts mainly for logging and don't really care about the width. I think your suggestion is to remove the defaulting to 79 too, and default to UNLIMITED. This completes breaking of the undocumented 79, but leaves the documented 131 and -w untouched and almost useless. When the default is not 79, they are only reachable if TIOCWINSZ succeeds on stdout (and COLUMNS is not set -- -w really shouldn't pply if COLUMNS is set, but it does). Then if the terminal width is < 132, ps's width can be increased to 131 with one -w. Not very useful. Two -w's are useful for reaching UNLIMITED. Bruce