[v2] Reset cached offset when reading to end of stream (BZ #17653)
diff mbox

Message ID 20141201074537.GT24022@spoyarek.pnq.redhat.com
State New
Headers show

Commit Message

Siddhesh Poyarekar Dec. 1, 2014, 7:45 a.m. UTC
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.

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.

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.

Comments

Andreas Schwab Dec. 1, 2014, 9:25 a.m. UTC | #1
Ok.

Andreas.
Carlos O'Donell Dec. 3, 2014, 7:46 p.m. UTC | #2
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.

Patch
diff mbox

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;
     }