| Message ID | 20141201074537.GT24022@spoyarek.pnq.redhat.com |
|---|---|
| State | New |
| Headers | show |
Ok. Andreas.
On 12/01/2014 02:45 AM, Siddhesh Poyarekar wrote: > On Thu, Nov 27, 2014 at 04:44:12PM +0100, Andreas Schwab wrote: >> For the case of naccbuf != 0 this isn't really EOF, but an error. No >> other error case is resetting _offset. > > Thanks, updated patch below: > >> Also, what about _IO_wfile_underflow_mmap? > > All of the _mmap files operate on read-only files, so the only way > another active handle can change state (from EOF) is by calling > {f,l}seek. In that case, the standard requires that the other handle, > upon activation, does an fseek as well, so resetting the cache is not > necessary. Agreed. > Siddhesh > > [BZ #17653] > * libio/fileops.c (_IO_new_file_underflow): Unset cached > offset on EOF. > * libio/wfileops.c (_IO_wfile_underflow): Likewise. > * libio/tst-ftell-active-handler.c (fgets_func_t): New type. > (fgets_func): Function pointer to fgets and fgetws. > (do_ftell_test): Add test to verify ftell value after read > EOF. > (do_test): Set fgets_func. Looks good to me. > commit 45bdf7cf9456e69e956f6398a3506d0dc9853338 > Author: Siddhesh Poyarekar <siddhesh@redhat.com> > Date: Thu Nov 27 20:32:57 2014 +0530 > > Reset cached offset when reading to end of stream (BZ #17653) > > POSIX allows applications to switch file handles when a read results > in an end of file. Unset the cached offset at this point so that it > is queried again. > > diff --git a/libio/fileops.c b/libio/fileops.c > index 1fc5719..3ae9682 100644 > --- a/libio/fileops.c > +++ b/libio/fileops.c > @@ -615,7 +615,13 @@ _IO_new_file_underflow (fp) > } > fp->_IO_read_end += count; > if (count == 0) > - return EOF; > + { > + /* If a stream is read to EOF, the calling application may switch active > + handles. As a result, our offset cache would no longer be valid, so > + unset it. */ > + fp->_offset = _IO_pos_BAD; > + return EOF; OK. > + } > if (fp->_offset != _IO_pos_BAD) > _IO_pos_adjust (fp->_offset, count); > return *(unsigned char *) fp->_IO_read_ptr; > diff --git a/libio/tst-ftell-active-handler.c b/libio/tst-ftell-active-handler.c > index 72066b4..f69e169 100644 > --- a/libio/tst-ftell-active-handler.c > +++ b/libio/tst-ftell-active-handler.c > @@ -86,7 +86,9 @@ static size_t data_len; > static size_t file_len; > > typedef int (*fputs_func_t) (const void *data, FILE *fp); > +typedef void *(*fgets_func_t) (void *ws, int n, FILE *fp); > fputs_func_t fputs_func; > +fgets_func_t fgets_func; > > /* This test verifies that the offset reported by ftell is correct after the > file is truncated using ftruncate. ftruncate does not change the file > @@ -290,20 +292,22 @@ do_ftell_test (const char *filename) > int fd_mode; > size_t old_off; > size_t new_off; > + size_t eof_off; > } test_modes[] = { > /* In w, w+ and r+ modes, the file position should be at the > beginning of the file. After the write, the offset should be > - updated to data_len. */ > - {"w", O_WRONLY | O_TRUNC, 0, data_len}, > - {"w+", O_RDWR | O_TRUNC, 0, data_len}, > - {"r+", O_RDWR, 0, data_len}, > + updated to data_len. We don't use eof_off in w and a modes since > + they don't allow reading. */ > + {"w", O_WRONLY | O_TRUNC, 0, data_len, 0}, > + {"w+", O_RDWR | O_TRUNC, 0, data_len, 2 * data_len}, > + {"r+", O_RDWR, 0, data_len, 3 * data_len}, > /* For the 'a' mode, the initial file position should be the > current end of file. After the write, the offset has data_len > added to the old value. For a+ mode however, the initial file > position is the file position of the underlying file descriptor, > since it is initially assumed to be in read mode. */ > - {"a", O_WRONLY, data_len, 2 * data_len}, > - {"a+", O_RDWR, 0, 3 * data_len}, > + {"a", O_WRONLY, 3 * data_len, 4 * data_len, 5 * data_len}, > + {"a+", O_RDWR, 0, 5 * data_len, 6 * data_len}, > }; > for (int j = 0; j < 2; j++) > { > @@ -348,12 +352,44 @@ do_ftell_test (const char *filename) > > if (off != test_modes[i].new_off) > { > - printf ("Incorrect new offset. Expected %zu but got %ld\n", > + printf ("Incorrect new offset. Expected %zu but got %ld", > test_modes[i].new_off, off); > ret |= 1; > } > else > - printf ("new offset = %ld\n", off); > + printf ("new offset = %ld", off); > + > + /* Read to the end, write some data to the fd and check if ftell can > + see the new ofset. Do this test only for files that allow > + reading. */ > + if (test_modes[i].fd_mode != O_WRONLY) > + { > + char tmpbuf[data_len]; > + > + rewind (fp); > + > + while (fgets_func (tmpbuf, sizeof (tmpbuf), fp) && !feof (fp)); > + > + write_ret = write (fd, data, data_len); > + if (write_ret != data_len) > + { > + printf ("write failed (%m)\n"); > + ret |= 1; > + } > + off = ftell (fp); > + > + if (off != test_modes[i].eof_off) > + { > + printf (", Incorrect offset after read EOF. " > + "Expected %zu but got %ld\n", > + test_modes[i].eof_off, off); > + ret |= 1; > + } > + else > + printf (", offset after EOF = %ld\n", off); > + } > + else > + putc ('\n', stdout); OK, good this is the test for the above change to reset the file-position to unknown if we read up to EOF. > > fclose (fp); > } > @@ -617,6 +653,7 @@ do_test (void) > /* Tests for regular files. */ > puts ("Regular mode:"); > fputs_func = (fputs_func_t) fputs; > + fgets_func = (fgets_func_t) fgets; > data = char_data; > data_len = strlen (char_data); > ret |= do_one_test (filename); > @@ -638,6 +675,7 @@ do_test (void) > return 1; > } > fputs_func = (fputs_func_t) fputws; > + fgets_func = (fgets_func_t) fgetws; > data = wide_data; > data_len = wcslen (wide_data); > ret |= do_one_test (filename); > diff --git a/libio/wfileops.c b/libio/wfileops.c > index 71281c1..2a003b3 100644 > --- a/libio/wfileops.c > +++ b/libio/wfileops.c > @@ -257,7 +257,10 @@ _IO_wfile_underflow (fp) > if (count <= 0) > { > if (count == 0 && naccbuf == 0) > - fp->_flags |= _IO_EOF_SEEN; > + { > + fp->_flags |= _IO_EOF_SEEN; > + fp->_offset = _IO_pos_BAD; OK, this is the wide-mode fix. > + } > else > fp->_flags |= _IO_ERR_SEEN, count = 0; > } > Cheers, Carlos.
diff --git a/libio/fileops.c b/libio/fileops.c index 1fc5719..3ae9682 100644 --- a/libio/fileops.c +++ b/libio/fileops.c @@ -615,7 +615,13 @@ _IO_new_file_underflow (fp) } fp->_IO_read_end += count; if (count == 0) - return EOF; + { + /* If a stream is read to EOF, the calling application may switch active + handles. As a result, our offset cache would no longer be valid, so + unset it. */ + fp->_offset = _IO_pos_BAD; + return EOF; + } if (fp->_offset != _IO_pos_BAD) _IO_pos_adjust (fp->_offset, count); return *(unsigned char *) fp->_IO_read_ptr; diff --git a/libio/tst-ftell-active-handler.c b/libio/tst-ftell-active-handler.c index 72066b4..f69e169 100644 --- a/libio/tst-ftell-active-handler.c +++ b/libio/tst-ftell-active-handler.c @@ -86,7 +86,9 @@ static size_t data_len; static size_t file_len; typedef int (*fputs_func_t) (const void *data, FILE *fp); +typedef void *(*fgets_func_t) (void *ws, int n, FILE *fp); fputs_func_t fputs_func; +fgets_func_t fgets_func; /* This test verifies that the offset reported by ftell is correct after the file is truncated using ftruncate. ftruncate does not change the file @@ -290,20 +292,22 @@ do_ftell_test (const char *filename) int fd_mode; size_t old_off; size_t new_off; + size_t eof_off; } test_modes[] = { /* In w, w+ and r+ modes, the file position should be at the beginning of the file. After the write, the offset should be - updated to data_len. */ - {"w", O_WRONLY | O_TRUNC, 0, data_len}, - {"w+", O_RDWR | O_TRUNC, 0, data_len}, - {"r+", O_RDWR, 0, data_len}, + updated to data_len. We don't use eof_off in w and a modes since + they don't allow reading. */ + {"w", O_WRONLY | O_TRUNC, 0, data_len, 0}, + {"w+", O_RDWR | O_TRUNC, 0, data_len, 2 * data_len}, + {"r+", O_RDWR, 0, data_len, 3 * data_len}, /* For the 'a' mode, the initial file position should be the current end of file. After the write, the offset has data_len added to the old value. For a+ mode however, the initial file position is the file position of the underlying file descriptor, since it is initially assumed to be in read mode. */ - {"a", O_WRONLY, data_len, 2 * data_len}, - {"a+", O_RDWR, 0, 3 * data_len}, + {"a", O_WRONLY, 3 * data_len, 4 * data_len, 5 * data_len}, + {"a+", O_RDWR, 0, 5 * data_len, 6 * data_len}, }; for (int j = 0; j < 2; j++) { @@ -348,12 +352,44 @@ do_ftell_test (const char *filename) if (off != test_modes[i].new_off) { - printf ("Incorrect new offset. Expected %zu but got %ld\n", + printf ("Incorrect new offset. Expected %zu but got %ld", test_modes[i].new_off, off); ret |= 1; } else - printf ("new offset = %ld\n", off); + printf ("new offset = %ld", off); + + /* Read to the end, write some data to the fd and check if ftell can + see the new ofset. Do this test only for files that allow + reading. */ + if (test_modes[i].fd_mode != O_WRONLY) + { + char tmpbuf[data_len]; + + rewind (fp); + + while (fgets_func (tmpbuf, sizeof (tmpbuf), fp) && !feof (fp)); + + write_ret = write (fd, data, data_len); + if (write_ret != data_len) + { + printf ("write failed (%m)\n"); + ret |= 1; + } + off = ftell (fp); + + if (off != test_modes[i].eof_off) + { + printf (", Incorrect offset after read EOF. " + "Expected %zu but got %ld\n", + test_modes[i].eof_off, off); + ret |= 1; + } + else + printf (", offset after EOF = %ld\n", off); + } + else + putc ('\n', stdout); fclose (fp); } @@ -617,6 +653,7 @@ do_test (void) /* Tests for regular files. */ puts ("Regular mode:"); fputs_func = (fputs_func_t) fputs; + fgets_func = (fgets_func_t) fgets; data = char_data; data_len = strlen (char_data); ret |= do_one_test (filename); @@ -638,6 +675,7 @@ do_test (void) return 1; } fputs_func = (fputs_func_t) fputws; + fgets_func = (fgets_func_t) fgetws; data = wide_data; data_len = wcslen (wide_data); ret |= do_one_test (filename); diff --git a/libio/wfileops.c b/libio/wfileops.c index 71281c1..2a003b3 100644 --- a/libio/wfileops.c +++ b/libio/wfileops.c @@ -257,7 +257,10 @@ _IO_wfile_underflow (fp) if (count <= 0) { if (count == 0 && naccbuf == 0) - fp->_flags |= _IO_EOF_SEEN; + { + fp->_flags |= _IO_EOF_SEEN; + fp->_offset = _IO_pos_BAD; + } else fp->_flags |= _IO_ERR_SEEN, count = 0; }