Message ID | CALoOobNy2mS=YFF63Hjz_+ewDPtjRa+Jyo_tofUM+vVkrS3bYA@mail.gmail.com |
---|---|
State | New |
Headers | show |
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.
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>