Date: Mon, 6 Mar 2017 20:21:12 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ed Schouten <ed@nuxi.nl> Cc: "Conrad E. Meyer" <cem@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r314685 - head/bin/ps Message-ID: <20170306183546.E1414@besplex.bde.org> In-Reply-To: <CABh_MKkjZdOWm05Hi%2BkcNh_UNRoK=JF_MZdkzOhC912%2B=1aONA@mail.gmail.com> References: <201703042238.v24McAD8008837@repo.freebsd.org> <CABh_MKnux-yfKhFeuApsZSNgyO1sktA_%2BDphhHQXaZezx92Cpw@mail.gmail.com> <CAG6CVpX_Q9JiW35Po7c4=TDSKy94%2BorFRSk_6u093T%2BbY2NL1Q@mail.gmail.com> <CABh_MKkjZdOWm05Hi%2BkcNh_UNRoK=JF_MZdkzOhC912%2B=1aONA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 5 Mar 2017, Ed Schouten wrote:
> 2017-03-05 0:08 GMT+01:00 Conrad Meyer <cem@freebsd.org>:
>>> - 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170306183546.E1414>