This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2] Fix offset caching for streams and use it for ftell (BZ #16680)
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: Siddhesh Poyarekar <siddhesh at redhat dot com>
- Cc: libc-alpha at sourceware dot org, dalias at aerifal dot cx
- Date: Mon, 17 Mar 2014 11:25:57 -0400
- Subject: Re: [PATCH v2] Fix offset caching for streams and use it for ftell (BZ #16680)
- Authentication-results: sourceware.org; auth=none
- References: <20140311124615 dot GA2182 at spoyarek dot pnq dot redhat dot com> <5326AFAA dot 7000809 at redhat dot com> <20140317113301 dot GL1850 at spoyarek dot pnq dot redhat dot com>
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.