[PATCH v2] Fix offset caching for streams and use it for ftell (BZ #16680)
Siddhesh Poyarekar
siddhesh@redhat.com
Mon Mar 17 11:32:00 GMT 2014
On Mon, Mar 17, 2014 at 04:17:46AM -0400, Carlos O'Donell wrote:
> Right, so I was thinking along the same lines. If you didn't switch
> from write to read then there is no reason why the offset can't
> be the end of the file after rewind. However, that subtle change
> is a change in our implementation and I agree that it would be best
> if we retained the old behaviour.
>
> Does a `write; rewind; read' work now with the patch? Was the current
> patch causing this to fail also or did the `read' start reading at
> offset 0?
That's not a concern since the read would fail for a file opened in
'a' mode. Only ftell behaviour was relevant here. For 'a+' it reads
at offset 0 and that is correct.
On 17 March 2014 16:02, Rich Felker <dalias@aerifal.cx> wrote:
>> Does this reinstitution of lseek cause any problems for the use of ftell
>> on inactive streams? For example is it really correct to have _IO_file_open
>> seek to the end of the fully flushed file in append mode? What about
>> other users of the fd that might expect the underlying offset to remain
>> the same?
>
> It's perfectly correct for fopen to perform the seek. For fdopen I'm
> not so clear; I think it's wrong for fdopen to change the position
> except in the case where O_APPEND wasn't set and fdopen adds it (in
> this case the implementation can do whatever it wants, no?).
I had missed the detail about fdopen not modifying the file descriptor
if O_APPEND is already set, thanks for pointing out. I've attached a
patch that applies on top of the earlier patch to ensure that fdopen
seeks to the end only when O_APPEND is set by fdopen and not when
O_APPEND is already set. I have also added a test case to verify this
behaviour. None of the old behaviour is affected by this.
Siddhesh
* libio/iofdopen.c (_IO_new_fdopen): Seek to end only if
setting O_APPEND.
* libio/tst-ftell-active-handler.c (do_append_test): Add a
test case.
diff --git a/libio/iofdopen.c b/libio/iofdopen.c
index 843a4fa..77a61f1 100644
--- a/libio/iofdopen.c
+++ b/libio/iofdopen.c
@@ -58,6 +58,7 @@ _IO_new_fdopen (fd, mode)
int fd_flags;
int i;
int use_mmap = 0;
+ bool do_seek = false;
switch (*mode)
{
@@ -128,6 +129,7 @@ _IO_new_fdopen (fd, mode)
*/
if ((posix_mode & O_APPEND) && !(fd_flags & O_APPEND))
{
+ do_seek = true;
#ifdef F_SETFL
if (_IO_fcntl (fd, F_SETFL, fd_flags | O_APPEND) == -1)
#endif
@@ -169,8 +171,8 @@ _IO_new_fdopen (fd, mode)
/* 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))
+ if (do_seek && ((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)
diff --git a/libio/tst-ftell-active-handler.c b/libio/tst-ftell-active-handler.c
index 40ca58c..e9dc7b3 100644
--- a/libio/tst-ftell-active-handler.c
+++ b/libio/tst-ftell-active-handler.c
@@ -414,6 +414,61 @@ do_append_test (const char *filename)
}
}
+ /* For fdopen in 'a' mode, the file descriptor should not change if the file
+ is already open with the O_APPEND flag set. */
+ fd = open (filename, O_WRONLY | O_APPEND, 0);
+ if (fd == -1)
+ {
+ printf ("open(O_APPEND) failed: %m\n");
+ return 1;
+ }
+
+ off_t seek_ret = lseek (fd, file_len - 1, SEEK_SET);
+ if (seek_ret == -1)
+ {
+ printf ("lseek[O_APPEND][0] failed: %m\n");
+ ret |= 1;
+ }
+
+ fp = fdopen (fd, "a");
+ if (fp == NULL)
+ {
+ printf ("fdopen(O_APPEND) failed: %m\n");
+ close (fd);
+ return 1;
+ }
+
+ off_t new_seek_ret = lseek (fd, 0, SEEK_CUR);
+ if (seek_ret == -1)
+ {
+ printf ("lseek[O_APPEND][1] failed: %m\n");
+ ret |= 1;
+ }
+
+ printf ("\tappend: fdopen (file, \"a\"): O_APPEND: ");
+
+ if (seek_ret != new_seek_ret)
+ {
+ printf ("incorrectly modified file offset to %ld, should be %ld",
+ new_seek_ret, seek_ret);
+ ret |= 1;
+ }
+ else
+ printf ("retained current file offset %ld", seek_ret);
+
+ new_seek_ret = ftello (fp);
+
+ if (seek_ret != new_seek_ret)
+ {
+ printf (", ftello reported incorrect offset %ld, should be %ld\n",
+ new_seek_ret, seek_ret);
+ ret |= 1;
+ }
+ else
+ printf (", ftello reported correct offset %ld\n", seek_ret);
+
+ fclose (fp);
+
return ret;
}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20140317/ebfb5438/attachment.sig>
More information about the Libc-alpha
mailing list