mirrors/glibc
1
0
Fork 0
mirror of https://sourceware.org/git/glibc.git synced 2024-11-27 19:53:32 +08:00
Code Issues Actions Packages Projects Releases Wiki Activity

Fix offset caching for streams and use it for ftell (BZ #16680)

Browse Source 
The ftell implementation was made conservative to ensure that
incorrectly cached offsets never affect it.  However, this causes
problems for append mode when a file stream is rewound.  Additionally,
the 'clever' trick of using stat to get position for append mode files
caused more problems than it solved and broke old behavior.  I have
described the various problems that it caused and then finally the
solution.

For a and a+ mode files, rewinding the stream should result in ftell
returning 0 as the offset, but the stat() trick caused it to
(incorrectly) always return the end of file.  Now I couldn't find
anything in POSIX that specifies the stream position after rewind()
for a file opened in 'a' mode, but for 'a+' mode it should be set to
0.  For 'a' mode too, it probably makes sense to keep it set to 0 in
the interest of retaining old behavior.

The initial file position for append mode files is implementation
defined, so the implementation could either retain the current file
position or move the position to the end of file.  The earlier ftell
implementation would move the offset to end of file for append-only
mode, but retain the old offset for a+ mode.  It would also cache the
offset (this detail is important).  My patch broke this and would set
the initial position to end of file for both append modes, thus
breaking old behavior.  I was ignorant enough to write an incorrect
test case for it too.

The Change:

I have now brought back the behavior of seeking to end of file for
append-only streams, but with a slight difference.  I don't cache the
offset though, since we would want ftell to query the current file
position through lseek while the stream is not active.  Since the
offset is moved to the end of file, we can rely on the file position
reported by lseek and we don't need to resort to the stat() nonsense.

Finally, the cache is always reliable, except when there are unflished
writes in an append mode stream (i.e. both a and a+).  In the latter
case, it is safe to just do an lseek to SEEK_END.  The value can be
safely cached too, since the file handle is already active at this
point.  Incidentally, this is the only state change we affect in the
file handle (apart from taking locks of course).

I have also updated the test case to correct my impression of the
initial file position for a+ streams to the initial behavior.  I have
verified that this does not break any existing tests in the testsuite
and also passes with the new tests.
This commit is contained in:
Siddhesh Poyarekar 2014-03-11 17:04:49 +05:30
parent b1dbb426e1
commit ea33158c96
6 changed files with 194 additions and 94 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
ChangeLog
View File

@ -1,5 +1,19 @@
2014-03-17 Siddhesh Poyarekar <siddhesh@redhat.com>
[BZ #16680]
* libio/fileops.c (_IO_file_open): Seek to end of file but
don't cache the offset.
(get_file_offset): Remove function.
(do_ftell): Use cached offset when available.
* libio/iofdopen.c (_IO_new_fdopen): Seek to end of file but
don't cache the offset.
* libio/tst-ftell-active-handler.c (do_rewind_test): New test
case.
(do_one_test): Call it.
(do_ftell_test): Fix up expected old offset for a+ mode.
* libio/wfileops.c (do_ftell_wide): Used cached offset when
available.
* libio/tst-ftell-active-handler.c (do_ftell_test): Don't mix
up test status with function return status.
(do_write_test): Likewise.

4
NEWS
View File

@ -10,8 +10,8 @@ Version 2.20
* The following bugs are resolved with this release:
15347, 15804, 15894, 16447, 16532, 16545, 16574, 16600, 16609, 16610,
16611, 16613, 16623, 16632, 16639, 16642, 16670, 16674, 16677, 16683,
16689, 16695, 16701, 16706, 16707.
16611, 16613, 16623, 16632, 16639, 16642, 16670, 16674, 16677, 16680,
16683, 16689, 16695, 16701, 16706, 16707.
* Running the testsuite no longer terminates as soon as a test fails.
Instead, a file tests.sum (xtests.sum from "make xcheck") is generated,

98
libio/fileops.c
View File

@ -232,13 +232,18 @@ _IO_file_open (fp, filename, posix_mode, prot, read_write, is32not64)
return NULL;
fp->_fileno = fdesc;
_IO_mask_flags (fp, read_write,_IO_NO_READS+_IO_NO_WRITES+_IO_IS_APPENDING);
if ((read_write & _IO_IS_APPENDING) && (read_write & _IO_NO_READS))
if (_IO_SEEKOFF (fp, (_IO_off64_t)0, _IO_seek_end, _IOS_INPUT|_IOS_OUTPUT)
== _IO_pos_BAD && errno != ESPIPE)
/* For append mode, send the file offset to the end of the file. Don't
update the offset cache though, since the file handle is not active. */
if ((read_write & (_IO_IS_APPENDING | _IO_NO_READS))
== (_IO_IS_APPENDING | _IO_NO_READS))
{
_IO_off64_t new_pos = _IO_SYSSEEK (fp, 0, _IO_seek_end);
if (new_pos == _IO_pos_BAD && errno != ESPIPE)
{
close_not_cancel (fdesc);
return NULL;
}
}
_IO_link_in ((struct _IO_FILE_plus *) fp);
return fp;
}
@ -929,43 +934,13 @@ _IO_file_sync_mmap (_IO_FILE *fp)
return 0;
}
/* Get the current file offset using a system call. This is the safest method
to get the current file offset, since we are sure that we get the current
state of the file. Before the stream handle is activated (by using fread,
fwrite, etc.), an application may alter the state of the file descriptor
underlying it by calling read/write/lseek on it. Using a cached offset at
this point will result in returning the incorrect value. Same is the case
when one switches from reading in a+ mode to writing, where the buffer has
not been flushed - the cached offset would reflect the reading position
while the actual write position would be at the end of the file.
do_ftell and do_ftell_wide may resort to using the cached offset in some
special cases instead of calling get_file_offset, but those cases should be
thoroughly described. */
_IO_off64_t
get_file_offset (_IO_FILE *fp)
{
if ((fp->_flags & _IO_IS_APPENDING) == _IO_IS_APPENDING)
{
struct stat64 st;
bool ret = (_IO_SYSSTAT (fp, &st) == 0 && S_ISREG (st.st_mode));
if (ret)
return st.st_size;
else
return EOF;
}
else
return _IO_SYSSEEK (fp, 0, _IO_seek_cur);
}
/* ftell{,o} implementation. Don't modify any state of the file pointer while
we try to get the current state of the stream. */
/* ftell{,o} implementation. The only time we modify the state of the stream
is when we have unflushed writes. In that case we seek to the end and
record that offset in the stream object. */
static _IO_off64_t
do_ftell (_IO_FILE *fp)
{
_IO_off64_t result = 0;
bool use_cached_offset = false;
_IO_off64_t result, offset = 0;
/* No point looking at unflushed data if we haven't allocated buffers
yet. */
@ -974,39 +949,37 @@ do_ftell (_IO_FILE *fp)
bool was_writing = (fp->_IO_write_ptr > fp->_IO_write_base
|| _IO_in_put_mode (fp));
/* Adjust for unflushed data. */
if (!was_writing)
result -= fp->_IO_read_end - fp->_IO_read_ptr;
else
result += fp->_IO_write_ptr - fp->_IO_read_end;
bool append_mode = (fp->_flags & _IO_IS_APPENDING) == _IO_IS_APPENDING;
/* It is safe to use the cached offset when available if there is
unbuffered data (indicating that the file handle is active) and the
handle is not for a file open in a+ mode. The latter condition is
because there could be a scenario where there is a switch from read
mode to write mode using an fseek to an arbitrary position. In this
case, there would be unbuffered data due to be appended to the end of
the file, but the offset may not necessarily be the end of the
file. It is fine to use the cached offset when the a+ stream is in
read mode though, since the offset is maintained correctly in that
case. Note that this is not a comprehensive set of cases when the
offset is reliable. The offset may be reliable even in some cases
where there is no unflushed input and the handle is active, but it's
just that we don't have a way to identify that condition reliably. */
use_cached_offset = (result != 0 && fp->_offset != _IO_pos_BAD
&& ((fp->_flags & (_IO_IS_APPENDING | _IO_NO_READS))
== (_IO_IS_APPENDING | _IO_NO_READS)
&& was_writing));
/* When we have unflushed writes in append mode, seek to the end of the
file and record that offset. This is the only time we change the file
stream state and it is safe since the file handle is active. */
if (was_writing && append_mode)
{
result = _IO_SYSSEEK (fp, 0, _IO_seek_end);
if (result == _IO_pos_BAD)
return EOF;
else
fp->_offset = result;
}
if (use_cached_offset)
result += fp->_offset;
/* Adjust for unflushed data. */
if (!was_writing)
offset -= fp->_IO_read_end - fp->_IO_read_ptr;
else
result += get_file_offset (fp);
offset += fp->_IO_write_ptr - fp->_IO_read_end;
}
if (fp->_offset != _IO_pos_BAD)
result = fp->_offset;
else
result = _IO_SYSSEEK (fp, 0, _IO_seek_cur);
if (result == EOF)
return result;
result += offset;
if (result < 0)
{
__set_errno (EINVAL);
@ -1016,7 +989,6 @@ do_ftell (_IO_FILE *fp)
return result;
}
_IO_off64_t
_IO_new_file_seekoff (fp, offset, dir, mode)
_IO_FILE *fp;

9
libio/iofdopen.c
View File

@ -167,6 +167,15 @@ _IO_new_fdopen (fd, mode)
_IO_mask_flags (&new_f->fp.file, read_write,
_IO_NO_READS+_IO_NO_WRITES+_IO_IS_APPENDING);
/* For append mode, set the file offset to the end of the file. Don't
update the offset cache though, since the file handle is not active. */
if ((read_write & (_IO_IS_APPENDING | _IO_NO_READS))
== (_IO_IS_APPENDING | _IO_NO_READS))
{
_IO_off64_t new_pos = _IO_SYSSEEK (&new_f->fp.file, 0, _IO_seek_end);
if (new_pos == _IO_pos_BAD && errno != ESPIPE)
return NULL;
}
return &new_f->fp.file;
}
libc_hidden_ver (_IO_new_fdopen, _IO_fdopen)

112
libio/tst-ftell-active-handler.c
View File

@ -88,6 +88,107 @@ static size_t file_len;
typedef int (*fputs_func_t) (const void *data, FILE *fp);
fputs_func_t fputs_func;
/* Test that ftell output after a rewind is correct. */
static int
do_rewind_test (const char *filename)
{
int ret = 0;
struct test
{
const char *mode;
int fd_mode;
size_t old_off;
size_t new_off;
} test_modes[] = {
{"w", O_WRONLY, 0, data_len},
{"w+", O_RDWR, 0, data_len},
{"r+", O_RDWR, 0, data_len},
/* The new offsets for 'a' and 'a+' modes have to factor in the
previous writes since they always append to the end of the
file. */
{"a", O_WRONLY, 0, 3 * data_len},
{"a+", O_RDWR, 0, 4 * data_len},
};
/* Empty the file before the test so that our offsets are simple to
calculate. */
FILE *fp = fopen (filename, "w");
if (fp == NULL)
{
printf ("Failed to open file for emptying\n");
return 1;
}
fclose (fp);
for (int j = 0; j < 2; j++)
{
for (int i = 0; i < sizeof (test_modes) / sizeof (struct test); i++)
{
FILE *fp;
int fd;
int fileret;
printf ("\trewind: %s (file, \"%s\"): ", j == 0 ? "fdopen" : "fopen",
test_modes[i].mode);
if (j == 0)
fileret = get_handles_fdopen (filename, fd, fp,
test_modes[i].fd_mode,
test_modes[i].mode);
else
fileret = get_handles_fopen (filename, fd, fp, test_modes[i].mode);
if (fileret != 0)
return fileret;
/* Write some content to the file, rewind and ensure that the ftell
output after the rewind is 0. POSIX does not specify what the
behavior is when a file is rewound in 'a' mode, so we retain
current behavior, which is to keep the 0 offset. */
size_t written = fputs_func (data, fp);
if (written == EOF)
{
printf ("fputs[1] failed to write data\n");
ret |= 1;
}
rewind (fp);
long offset = ftell (fp);
if (offset != test_modes[i].old_off)
{
printf ("Incorrect old offset. Expected %zu, but got %ld, ",
test_modes[i].old_off, offset);
ret |= 1;
}
else
printf ("old offset = %ld, ", offset);
written = fputs_func (data, fp);
if (written == EOF)
{
printf ("fputs[1] failed to write data\n");
ret |= 1;
}
/* After this write, the offset in append modes should factor in the
implicit lseek to the end of file. */
offset = ftell (fp);
if (offset != test_modes[i].new_off)
{
printf ("Incorrect new offset. Expected %zu, but got %ld\n",
test_modes[i].new_off, offset);
ret |= 1;
}
else
printf ("new offset = %ld\n", offset);
}
}
return ret;
}
/* Test that the value of ftell is not cached when the stream handle is not
active. */
static int
@ -107,11 +208,13 @@ do_ftell_test (const char *filename)
{"w", O_WRONLY, 0, data_len},
{"w+", O_RDWR, 0, data_len},
{"r+", O_RDWR, 0, data_len},
/* For 'a' and 'a+' modes, the initial file position should be the
/* 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. */
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, 2 * data_len, 3 * data_len},
{"a+", O_RDWR, 0, 3 * data_len},
};
for (int j = 0; j < 2; j++)
{
@ -157,7 +260,7 @@ do_ftell_test (const char *filename)
if (off != test_modes[i].new_off)
{
printf ("Incorrect new offset. Expected %zu but got %ld\n",
test_modes[i].old_off, off);
test_modes[i].new_off, off);
ret |= 1;
}
else
@ -322,6 +425,7 @@ do_one_test (const char *filename)
ret |= do_ftell_test (filename);
ret |= do_write_test (filename);
ret |= do_append_test (filename);
ret |= do_rewind_test (filename);
return ret;
}

47
libio/wfileops.c
View File

@ -597,12 +597,12 @@ done:
}
/* ftell{,o} implementation for wide mode. Don't modify any state of the file
pointer while we try to get the current state of the stream. */
pointer while we try to get the current state of the stream except in one
case, which is when we have unflushed writes in append mode. */
static _IO_off64_t
do_ftell_wide (_IO_FILE *fp)
{
_IO_off64_t result, offset = 0;
bool use_cached_offset = false;
/* No point looking for offsets in the buffer if it hasn't even been
allocated. */
@ -615,6 +615,20 @@ do_ftell_wide (_IO_FILE *fp)
> fp->_wide_data->_IO_write_base)
|| _IO_in_put_mode (fp));
bool append_mode = (fp->_flags & _IO_IS_APPENDING) == _IO_IS_APPENDING;
/* When we have unflushed writes in append mode, seek to the end of the
file and record that offset. This is the only time we change the file
stream state and it is safe since the file handle is active. */
if (was_writing && append_mode)
{
result = _IO_SYSSEEK (fp, 0, _IO_seek_end);
if (result == _IO_pos_BAD)
return EOF;
else
fp->_offset = result;
}
/* XXX For wide stream with backup store it is not very
reasonable to determine the offset. The pushed-back
character might require a state change and we need not be
@ -703,37 +717,24 @@ do_ftell_wide (_IO_FILE *fp)
position is fp._offset - (_IO_read_end - new_write_ptr). */
offset -= fp->_IO_read_end - fp->_IO_write_ptr;
}
/* It is safe to use the cached offset when available if there is
unbuffered data (indicating that the file handle is active) and
the handle is not for a file open in a+ mode. The latter
condition is because there could be a scenario where there is a
switch from read mode to write mode using an fseek to an arbitrary
position. In this case, there would be unbuffered data due to be
appended to the end of the file, but the offset may not
necessarily be the end of the file. It is fine to use the cached
offset when the a+ stream is in read mode though, since the offset
is maintained correctly in that case. Note that this is not a
comprehensive set of cases when the offset is reliable. The
offset may be reliable even in some cases where there is no
unflushed input and the handle is active, but it's just that we
don't have a way to identify that condition reliably. */
use_cached_offset = (offset != 0 && fp->_offset != _IO_pos_BAD
&& ((fp->_flags & (_IO_IS_APPENDING | _IO_NO_READS))
== (_IO_IS_APPENDING | _IO_NO_READS)
&& was_writing));
}
if (use_cached_offset)
if (fp->_offset != _IO_pos_BAD)
result = fp->_offset;
else
result = get_file_offset (fp);
result = _IO_SYSSEEK (fp, 0, _IO_seek_cur);
if (result == EOF)
return result;
result += offset;
if (result < 0)
{
__set_errno (EINVAL);
return EOF;
}
return result;
}