Use cached offset in ftell when reliable

The cached offset is reliable to use in ftell when the stream handle
is active.  We can consider a stream as being active when there is
unflushed data.  However, even in this case, we can use the cached
offset only when the stream is not being written to in a+ mode,
because this case may have unflushed data and a stale offset; the
previous read could have sent it off somewhere other than the end of
the file.

There were a couple of adjustments necessary to get this to work.
Firstly, fdopen now ceases to use _IO_attach_fd because it sets the
offset cache to the current file position.  This is not correct
because there could be changes to the file descriptor before the
stream handle is activated, which would not get reflected.

A similar offset caching action is done in _IO_fwide, claiming that
wide streams have 'problems' with the file offsets.  There don't seem
to be any obvious problems with not having the offset cache available,
other than that it will have to be queried in a subsequent
read/write/seek.  I have removed this as well.

The testsuite passes successfully with these changes on x86_64.

diff --git a/ChangeLog b/ChangeLog
index d8d87d7856e7891033a8f5ce93dab4b2ddb2fc26..b1ccca410149fc5fb12422ccacc996ccc622a16c 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,12 @@
 2014-03-04  Siddhesh Poyarekar  <siddhesh@redhat.com>
 
+       * libio/fileops.c (do_ftell): Use cached offset when
+       available.
+       * libio/iofwide.c (do_ftell_wide): Likewise.
+       * libio/iofdopen.c (_IO_new_fdopen): Don't use
+       _IO_file_attach.
+       * libio/wfileops.c (_IO_fwide): Don't cache offset.
+
        [BZ #16532]
        * libio/libioP.h (get_file_offset): New function.
        * libio/fileops.c (get_file_offset): Likewise.

diff --git a/libio/fileops.c b/libio/fileops.c
index 500629564ae86061f777487e9f00d8d617fd89a1..2e7bc8dad9ad6ddffc7ad2d7ac86559636a720da 100644 (file)
--- a/libio/fileops.c
+++ b/libio/fileops.c
@@ -964,12 +964,8 @@ get_file_offset (_IO_FILE *fp)
 static _IO_off64_t
 do_ftell (_IO_FILE *fp)
 {
-  _IO_off64_t result;
-
-  result = get_file_offset (fp);
-
-  if (result == EOF)
-    return result;
+  _IO_off64_t result = 0;
+  bool use_cached_offset = false;
 
   /* No point looking at unflushed data if we haven't allocated buffers
      yet.  */
@@ -983,8 +979,34 @@ do_ftell (_IO_FILE *fp)
        result -= fp->_IO_read_end - fp->_IO_read_ptr;
       else
        result += fp->_IO_write_ptr - fp->_IO_read_end;
+
+      /* 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));
     }
 
+  if (use_cached_offset)
+    result += fp->_offset;
+  else
+    result += get_file_offset (fp);
+
+  if (result == EOF)
+    return result;
+
   if (result < 0)
     {
       __set_errno (EINVAL);

diff --git a/libio/iofdopen.c b/libio/iofdopen.c
index 066ff19000bbf3ec1b02d662aa925d6205656301..3f266f728818ecfecff48fc71942ac94f4d085bd 100644 (file)
--- a/libio/iofdopen.c
+++ b/libio/iofdopen.c
@@ -141,9 +141,6 @@ _IO_new_fdopen (fd, mode)
 #ifdef _IO_MTSAFE_IO
   new_f->fp.file._lock = &new_f->lock;
 #endif
-  /* Set up initially to use the `maybe_mmap' jump tables rather than using
-     __fopen_maybe_mmap to do it, because we need them in place before we
-     call _IO_file_attach or else it will allocate a buffer immediately.  */
   _IO_no_init (&new_f->fp.file, 0, 0, &new_f->wd,
 #ifdef _G_HAVE_MMAP
               (use_mmap && (read_write & _IO_NO_WRITES))
@@ -159,13 +156,12 @@ _IO_new_fdopen (fd, mode)
 #if  !_IO_UNIFIED_JUMPTABLES
   new_f->fp.vtable = NULL;
 #endif
-  if (_IO_file_attach ((_IO_FILE *) &new_f->fp, fd) == NULL)
-    {
-      _IO_setb (&new_f->fp.file, NULL, NULL, 0);
-      _IO_un_link (&new_f->fp);
-      free (new_f);
-      return NULL;
-    }
+  /* We only need to record the fd because _IO_file_init will have unset the
+     offset.  It is important to unset the cached offset because the real
+     offset in the file could change between now and when the handle is
+     activated and we would then mislead ftell into believing that we have a
+     valid offset.  */
+  new_f->fp.file._fileno = fd;
   new_f->fp.file._flags &= ~_IO_DELETE_DONT_CLOSE;
 
   _IO_mask_flags (&new_f->fp.file, read_write,

diff --git a/libio/iofwide.c b/libio/iofwide.c
index 5cff6325f10579c3049e3f3a6a35c9f1c2848910..64187e401b2d09c68972cd28f8cf94f4e8a979fc 100644 (file)
--- a/libio/iofwide.c
+++ b/libio/iofwide.c
@@ -199,12 +199,6 @@ _IO_fwide (fp, mode)
 
       /* From now on use the wide character callback functions.  */
       ((struct _IO_FILE_plus *) fp)->vtable = fp->_wide_data->_wide_vtable;
-
-      /* One last twist: we get the current stream position.  The wide
-        char streams have much more problems with not knowing the
-        current position and so we should disable the optimization
-        which allows the functions without knowing the position.  */
-      fp->_offset = _IO_SYSSEEK (fp, 0, _IO_seek_cur);
     }
 
   /* Set the mode now.  */

diff --git a/libio/wfileops.c b/libio/wfileops.c
index 7f4c92399faf741af41cd61305b0e3bb401592f8..776bb4a4bac89b7f886618fb9aa64585d9acd027 100644 (file)
--- a/libio/wfileops.c
+++ b/libio/wfileops.c
@@ -602,6 +602,7 @@ 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.  */
@@ -702,9 +703,31 @@ do_ftell_wide (_IO_FILE *fp)
             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));
     }
 
-  result = get_file_offset (fp);
+  if (use_cached_offset)
+    result = fp->_offset;
+  else
+    result = get_file_offset (fp);
 
   if (result == EOF)
     return result;