svn commit: r358152 - head/bin/sh

[Pedro Giffuni]

For the record ...

On 21/02/2020 22:31, Kyle Evans wrote:
> On Fri, Feb 21, 2020 at 3:53 PM Li-Wen Hsu <lwhsu at freebsd.org> wrote:
>> On Sat, Feb 22, 2020 at 4:58 AM Antoine Brodin <antoine at freebsd.org> wrote:
>>> On Thu, Feb 20, 2020 at 4:01 AM Hiroki Sato <hrs at freebsd.org> wrote:
>>>> Author: hrs
>>>> Date: Thu Feb 20 03:01:27 2020
>>>> New Revision: 358152
>>>> URL: https://svnweb.freebsd.org/changeset/base/358152
>>>>
>>>> Log:
>>>>    Improve performance of "read" built-in command when using a seekable
>>>>    fd.
>>>>
>>>>    The read built-in command calls read(2) with a 1-byte buffer because
>>>>    newline characters need to be detected even on a byte stream which
>>>>    comes from a non-seekable file descriptor.  Because of this, the
>>>>    following script calls >6,000 read(2) to show a 6KiB file:
>>>>
>>>>     while read IN; do echo "$IN"; done < /COPYRIGHT
>>>>
>>>>    When the input byte stream is seekable, it is possible to read a data
>>>>    block and then reposition the file pointer to where a newline
>>>>    character found.  This change adds a small buffer to do this and
>>>>    reduces the number of read(2) calls.
>>>>
>>>>    Theoretically, multiple built-in commands reading the same seekable
>>>>    byte stream in a single pipe chain can share the buffer.  However,
>>>>    this change just makes a single invocation of the read built-in
>>>>    allocate a buffer and deallocate it every time for simplicity.
>>>>    Although this causes read(2) to read the same regions multiple times,
>>>>    the performance penalty should be small compared to the reduction of
>>>>    read(2) calls.
>>>>
>>>>    Reviewed by:          jilles
>>>>    MFC after:            1 week
>>>>    Differential Revision:        https://reviews.freebsd.org/D23747
>>> This seems to be broken on at least i386.
>>> Please either fix or revert.
>>>
>>> Antoine (with hat: portmgr)
>> Could you provide more detail?  I'm worried because I didn't see
>> related regression from the recent test results. We may need to add
>> more test against the breakage you mentioned.
>>
> This trivially failed with the example in the commit message; only the
> first line would be output. It also triggered a failure of
> functional_test:read2 in /usr/tests/bin/sh/builtins on i386 (and all
> of the other platforms with a 32-bit size_t), which would exit with a
> non-zero status code.
>
> I tested and deployed the fix suggested by cem@ as r358235 by just
> making residue an off_t,

This is an example case of why it is important to keep the i386 port 
building and running. If we don't have a 32 bit port that is easy to 
build and test many bugs like these can easily go through.

Cheers,

Pedro.