This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2] Reset cached offset when reading to end of stream (BZ #17653)
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: Siddhesh Poyarekar <siddhesh at redhat dot com>, Andreas Schwab <schwab at linux-m68k dot org>
- Cc: libc-alpha at sourceware dot org
- Date: Wed, 03 Dec 2014 14:46:11 -0500
- Subject: Re: [PATCH v2] Reset cached offset when reading to end of stream (BZ #17653)
- Authentication-results: sourceware.org; auth=none
- References: <20141127122839 dot GA31809 at spoyarek dot pnq dot redhat dot com> <87sih4vkpg dot fsf at igel dot home> <20141127150731 dot GB24022 at spoyarek dot pnq dot redhat dot com> <87h9xkveb7 dot fsf at igel dot home> <20141201074537 dot GT24022 at spoyarek dot pnq dot redhat dot com>
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.