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

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.

On 03/17/2014 07:33 AM, Siddhesh Poyarekar wrote:
> 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.

Awesome, I'm glad something useful came out of my review ;-)
 
> 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.

This looks good to me and removes the only concern I had with your
patch that we were modifying the underlying fd's offset on f*open.

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

OK.

>  
>    switch (*mode)
>      {
> @@ -128,6 +129,7 @@ _IO_new_fdopen (fd, mode)
>       */
>    if ((posix_mode & O_APPEND) && !(fd_flags & O_APPEND))
>      {
> +      do_seek = true;

OK.

>  #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)))

OK.

>      {
>        _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;
> +    }
> +

OK.

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

OK.

Cheers,
Carlos.