[v2] Fix offset caching for streams and use it for ftell (BZ #16680)
Commit Message
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.
Comments
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.
@@ -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)
@@ -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;
}