Fix failure to cleanup input-only stream buffers in preparation for BZ#16734 fix

  Greetings,

In preparation for BZ#16734 fix, I've build libc with (not intended to commit):

 # include <fcntl.h>
... etc.

This results in 3 mtrace failures:

FAIL: misc/tst-error1-mem
FAIL: posix/bug-regex31-mem
FAIL: posix/tst-fnmatch-mem

The last two of them are because input-only FILE streams need the same
buffer cleanup as writable streams.

Attached patch fixes these two failures.


2015-02-16  Paul Pluzhnikov  <ppluzhnikov@google.com>

        * libio/genops.c (_IO_unbuffer_write): Cleanup read-only
streams as well.
  

On Mon, Feb 16, 2015 at 5:07 PM, Paul Pluzhnikov <ppluzhnikov@gmail.com> wrote:

> In preparation for BZ#16734 fix, I've build libc with (not intended to commit):
>
> diff --git a/libio/libioP.h b/libio/libioP.h
> index d8604ca..d1699de 100644
> --- a/libio/libioP.h
> +++ b/libio/libioP.h
> @@ -719,7 +719,7 @@ extern _IO_off64_t _IO_seekpos_unlocked (_IO_FILE
> *, _IO_off64_t, int)
>  # endif
>  #endif
>
> -#if _G_HAVE_MMAP
> +#if _G_HAVE_MMAP && 0
>
>  # include <unistd.h>
>  # include <fcntl.h>
> ... etc.
>
> This results in 3 mtrace failures:
>
> FAIL: misc/tst-error1-mem
> FAIL: posix/bug-regex31-mem
> FAIL: posix/tst-fnmatch-mem
>
> The last two of them are because input-only FILE streams need the same
> buffer cleanup as writable streams.
>
> Attached patch fixes these two failures.
>
>
> 2015-02-16  Paul Pluzhnikov  <ppluzhnikov@google.com>
>
>         * libio/genops.c (_IO_unbuffer_write): Cleanup read-only
> streams as well.

Ping?
  

On Mon, Feb 16, 2015 at 5:07 PM, Paul Pluzhnikov <ppluzhnikov@gmail.com> wrote:
> Greetings,
>
> In preparation for BZ#16734 fix, I've build libc with (not intended to commit):
>
> diff --git a/libio/libioP.h b/libio/libioP.h
> index d8604ca..d1699de 100644
> --- a/libio/libioP.h
> +++ b/libio/libioP.h
> @@ -719,7 +719,7 @@ extern _IO_off64_t _IO_seekpos_unlocked (_IO_FILE
> *, _IO_off64_t, int)
>  # endif
>  #endif
>
> -#if _G_HAVE_MMAP
> +#if _G_HAVE_MMAP && 0
>
>  # include <unistd.h>
>  # include <fcntl.h>
> ... etc.
>
> This results in 3 mtrace failures:
>
> FAIL: misc/tst-error1-mem
> FAIL: posix/bug-regex31-mem
> FAIL: posix/tst-fnmatch-mem
>
> The last two of them are because input-only FILE streams need the same
> buffer cleanup as writable streams.
>
> Attached patch fixes these two failures.
>
>
> 2015-02-16  Paul Pluzhnikov  <ppluzhnikov@google.com>
>
>         * libio/genops.c (_IO_unbuffer_write): Cleanup read-only
> streams as well.

It looks good to me, except for the name of the function,
_IO_unbuffer_write, which was changed by

1998-11-29 1998  H.J. Lu  <hjl@gnu.org>

        * libio/genops.c (_IO_unbuffer_write): Renamed from
        _IO_unbuffer_all.
        (_IO_cleanup): Call _IO_unbuffer_write instead of
        _IO_unbuffer_all.

With this patch,  the original name makes more senses now.
OK with the original function name.

Thanks.
  

On Sun, Mar 8, 2015 at 7:44 AM, H.J. Lu <hjl.tools@gmail.com> wrote:

> OK with the original function name.

Thanks. Committed as 18d2675
  

On 02/16/2015 08:07 PM, Paul Pluzhnikov wrote:
> bz16734-cleanup.patch2.txt
> 
> 
> diff --git a/libio/genops.c b/libio/genops.c
> index 6612997..094598f 100644
> --- a/libio/genops.c
> +++ b/libio/genops.c
> @@ -950,8 +950,6 @@ _IO_unbuffer_write (void)
>    for (fp = (_IO_FILE *) _IO_list_all; fp; fp = fp->_chain)
>      {
>        if (! (fp->_flags & _IO_UNBUFFERED)
> -	  && (! (fp->_flags & _IO_NO_WRITES)
> -	      || (fp->_flags & _IO_IS_APPENDING))
>  	  /* Iff stream is un-orientated, it wasn't used. */
>  	  && fp->_mode != 0)
>  	{

I know you already committed this, but I wanted to mention that
I just reviewed this and it looks OK to me.

It would have been nice to have added a specific test case to
catch this instead of relying on other orthogonal test cases.
Please still consider adding a test case for this specific
failure.

I had a lot of difficulty untying the POSIX stream, and fd
requirements the last time I touched this code, and Siddhesh
helped write several more key regression tests to encode
the expectations we had as seen in [1][2]. While that is not
relevant here, what is relevant is making sure we have good
and clear regression test coverage.

Cheers,
Carlos.

[1] https://sourceware.org/glibc/wiki/File%20offsets%20in%20a%20stdio%20stream%20and%20ftell
[2] "2.5.1 Interaction of File Descriptors and Standard I/O Streams"
    http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html
  

On Sun, Mar 8, 2015 at 11:47 AM, Carlos O'Donell <carlos@redhat.com> wrote:

> Please still consider adding a test case for this specific
> failure.

Before the mmap->malloc switch, this failure manifests in missing
munmap, but mmap/munmap from stdio is not interposable, and I am not
sure there *is* a way to write a test for this.

After the switch, sure: I can add a specific read-only fopen test.
  

On 03/08/2015 02:57 PM, Paul Pluzhnikov wrote:
> On Sun, Mar 8, 2015 at 11:47 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> 
>> Please still consider adding a test case for this specific
>> failure.
> 
> Before the mmap->malloc switch, this failure manifests in missing
> munmap, but mmap/munmap from stdio is not interposable, and I am not
> sure there *is* a way to write a test for this.

Yes, you won't be able to test that.

> After the switch, sure: I can add a specific read-only fopen test.

Exactly.

Cheers,
Carlos.