This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 1/2] Separate ftell from fseek logic and avoid modifying FILE data (#16532)
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: Siddhesh Poyarekar <siddhesh at redhat dot com>, libc-alpha at sourceware dot org
- Date: Mon, 03 Mar 2014 18:15:45 -0500
- Subject: Re: [PATCH 1/2] Separate ftell from fseek logic and avoid modifying FILE data (#16532)
- Authentication-results: sourceware.org; auth=none
- References: <20140220172904 dot GC16378 at spoyarek dot pnq dot redhat dot com>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 02/20/2014 12:29 PM, Siddhesh Poyarekar wrote:
> Hi,
>
> So this is take 3 for this fix:
>
> ftell semantics are distinct from fseek(SEEK_CUR) especially when it
> is called on a file handler that is not yet active. Due to this
> caveat, much care needs to be taken while modifying the handler data
> and hence, this first iteration on separating out ftell focusses on
> maintaining handler data integrity at all times while it figures out
> the current stream offset. The result is that it makes a syscall for
> every offset request.
>
> There is scope for optimizing this by caching offsets when we know
> that the handler is active. A simple way to find out is when the
> buffers have data. It is not so simple to find this out when the
> buffer is empty without adding some kind of flag. I have done this in
> Patch 2/2.
>
> I have verified on x86_64 that there are no regressions in the
> testsuite resulting fomr this patch. The patch also includes a test
> case that tests ftell behaviour at various points before and after the
> streame handle becomes active.
>
> Siddhesh
>
> [BZ #16532]
> * libio/libioP.h (get_file_offset): New function.
> * libio/fileops.c (get_file_offset): Likewise.
> (do_ftell): Likewise.
> (_IO_new_file_seekoff): Split out ftell logic.
> * libio/wfileops.c (do_ftell_wide): Likewise.
> (_IO_wfile_seekoff): Split out ftell logic.
> * libio/tst-ftell-active-handler.c: New test case.
> * libio/Makefile (tests): Add it.
This took me a while to review, sorry about that.
The refactoring and changes look correct to me.
OK to checkin after you fix the nits I have.
Required fixed:
- - Add descriptive comment to get_file_offset
- - Add descriptive comment to do_ftell_wide
- - Add descriptive comment to each test cases explaining
how you computed the expected value and why it's
expected. Future maintainers need this.
Optional fixed:
- - One instance of:
if (ret != 0)
return 1;
could be:
if (ret != 0)
return ret;
Noted in the review comments.
> ---
> libio/Makefile | 2 +-
> libio/fileops.c | 93 ++++++----
> libio/libioP.h | 1 +
> libio/tst-ftell-active-handler.c | 366 +++++++++++++++++++++++++++++++++++++++
> libio/wfileops.c | 224 +++++++++++++-----------
> 5 files changed, 553 insertions(+), 133 deletions(-)
> create mode 100644 libio/tst-ftell-active-handler.c
>
> diff --git a/libio/Makefile b/libio/Makefile
> index 8c333ce..8ccd80f 100644
> --- a/libio/Makefile
> +++ b/libio/Makefile
> @@ -60,7 +60,7 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc \
> tst-wmemstream1 tst-wmemstream2 \
> bug-memstream1 bug-wmemstream1 \
> tst-setvbuf1 tst-popen1 tst-fgetwc bug-wsetpos tst-fseek \
> - tst-fwrite-error tst-ftell-partial-wide
> + tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler
OK.
> ifeq (yes,$(build-shared))
> # Add test-fopenloc only if shared library is enabled since it depends on
> # shared localedata objects.
> diff --git a/libio/fileops.c b/libio/fileops.c
> index a3499be..a177302 100644
> --- a/libio/fileops.c
> +++ b/libio/fileops.c
> @@ -931,6 +931,59 @@ _IO_file_sync_mmap (_IO_FILE *fp)
>
>
Needs a comment describing what the function does.
> _IO_off64_t
> +get_file_offset (_IO_FILE *fp)
> +{
> + if ((fp->_flags & _IO_IS_APPENDING) == _IO_IS_APPENDING)
> + {
> + struct stat64 st;
> + bool ret = (_IO_SYSSTAT (fp, &st) == 0 && S_ISREG (st.st_mode));
OK.
Notes:
I thought we might be able to avoid the stat, but we can't
since as you've pointed out, there could be other users of
the fd and that's valid until you've started using the stream.
Either way the comment is now critical to clarifying this.
> + if (ret)
> + return st.st_size;
> + else
> + return EOF;
OK.
> + }
> + else
> + return _IO_SYSSEEK (fp, 0, _IO_seek_cur);
> +}
OK.
> +
> +
> +/* ftell{,o} implementation. Don't modify any state of the file pointer while
> + we try to get the current state of the stream. */
> +static _IO_off64_t
> +do_ftell (_IO_FILE *fp)
> +{
> + _IO_off64_t result;
> +
> + result = get_file_offset (fp);
OK.
> +
> + if (result == EOF)
> + return result;
OK.
Notes:
I don't like the use of EOF to mean error, and I would rather see
_IO_pos_BAD used instead, but I know that the pattern in the rest
of the code is to use EOF for error, so I'm OK with this. Just
thinking out loud.
> +
> + /* No point looking at unflushed data if we haven't allocated buffers
> + yet. */
> + if (fp->_IO_buf_base != NULL)
> + {
> + bool was_writing = (fp->_IO_write_ptr > fp->_IO_write_base
> + || _IO_in_put_mode (fp));
> +
> + /* Adjust for unflushed data. */
> + if (!was_writing)
> + result -= fp->_IO_read_end - fp->_IO_read_ptr;
> + else
> + result += fp->_IO_write_ptr - fp->_IO_read_end;
> + }
> +
> + if (result < 0)
> + {
> + __set_errno (EINVAL);
> + return EOF;
OK.
> + }
> +
> + return result;
> +}
> +
> +
> +_IO_off64_t
> _IO_new_file_seekoff (fp, offset, dir, mode)
> _IO_FILE *fp;
> _IO_off64_t offset;
> @@ -940,6 +993,13 @@ _IO_new_file_seekoff (fp, offset, dir, mode)
> _IO_off64_t result;
> _IO_off64_t delta, new_offset;
> long count;
> +
> + /* Short-circuit into a separate function. We don't want to mix any
> + functionality and we don't want to touch anything inside the FILE
> + object. */
> + if (mode == 0)
> + return do_ftell (fp);
OK.
Note: This is a great idea, easier to audit too. Nicely done.
> +
> /* POSIX.1 8.2.3.7 says that after a call the fflush() the file
> offset of the underlying file must be exact. */
> int must_be_exact = (fp->_IO_read_base == fp->_IO_read_end
> @@ -948,9 +1008,6 @@ _IO_new_file_seekoff (fp, offset, dir, mode)
> bool was_writing = (fp->_IO_write_ptr > fp->_IO_write_base
> || _IO_in_put_mode (fp));
>
> - if (mode == 0)
> - dir = _IO_seek_cur, offset = 0; /* Don't move any pointers. */
> -
> /* Flush unwritten characters.
> (This may do an unneeded write if we seek within the buffer.
> But to be able to switch to reading, we would need to set
> @@ -958,7 +1015,7 @@ _IO_new_file_seekoff (fp, offset, dir, mode)
> which assumes file_ptr() is eGptr. Anyway, since we probably
> end up flushing when we close(), it doesn't make much difference.)
> FIXME: simulate mem-mapped files. */
> - else if (was_writing && _IO_switch_to_get_mode (fp))
> + if (was_writing && _IO_switch_to_get_mode (fp))
> return EOF;
OK.
Note: Hate the use of EOF here, but don't touch anything that isn't needed.
>
> if (fp->_IO_buf_base == NULL)
> @@ -978,30 +1035,10 @@ _IO_new_file_seekoff (fp, offset, dir, mode)
> {
> case _IO_seek_cur:
> /* Adjust for read-ahead (bytes is buffer). */
> - if (mode != 0 || !was_writing)
> - offset -= fp->_IO_read_end - fp->_IO_read_ptr;
> - else
> - {
> - /* _IO_read_end coincides with fp._offset, so the actual file position
> - is fp._offset - (_IO_read_end - new_write_ptr). This is fine
> - even if fp._offset is not set, since fp->_IO_read_end is then at
> - _IO_buf_base and this adjustment is for unbuffered output. */
> - offset -= fp->_IO_read_end - fp->_IO_write_ptr;
> - }
> + offset -= fp->_IO_read_end - fp->_IO_read_ptr;
OK, simplified by `mode != 0' always being true now.
Note: Really happy to see this simplified.
>
> if (fp->_offset == _IO_pos_BAD)
> - {
> - if (mode != 0)
> - goto dumb;
> - else
> - {
> - result = _IO_SYSSEEK (fp, 0, dir);
> - if (result == EOF)
> - return result;
> -
> - fp->_offset = result;
> - }
> - }
> + goto dumb;
OK.
> /* Make offset absolute, assuming current pointer is file_ptr(). */
> offset += fp->_offset;
> if (offset < 0)
> @@ -1028,10 +1065,6 @@ _IO_new_file_seekoff (fp, offset, dir, mode)
> }
> /* At this point, dir==_IO_seek_set. */
>
> - /* If we are only interested in the current position we've found it now. */
> - if (mode == 0)
> - return offset;
OK.
> -
> /* If destination is within current buffer, optimize: */
> if (fp->_offset != _IO_pos_BAD && fp->_IO_read_base != NULL
> && !_IO_in_backup (fp))
> diff --git a/libio/libioP.h b/libio/libioP.h
> index 4ca723c..8a7b85b 100644
> --- a/libio/libioP.h
> +++ b/libio/libioP.h
> @@ -397,6 +397,7 @@ extern void _IO_wdoallocbuf (_IO_FILE *) __THROW;
> libc_hidden_proto (_IO_wdoallocbuf)
> extern void _IO_unsave_wmarkers (_IO_FILE *) __THROW;
> extern unsigned _IO_adjust_wcolumn (unsigned, const wchar_t *, int) __THROW;
> +extern _IO_off64_t get_file_offset (_IO_FILE *fp);
OK.
>
> /* Marker-related function. */
>
> diff --git a/libio/tst-ftell-active-handler.c b/libio/tst-ftell-active-handler.c
> new file mode 100644
> index 0000000..aac2923
> --- /dev/null
> +++ b/libio/tst-ftell-active-handler.c
> @@ -0,0 +1,366 @@
> +/* Verify that ftell returns the correct value at various points before and
> + after the handler on which it is called becomes active.
OK.
> + Copyright (C) 2014 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <locale.h>
> +#include <wchar.h>
> +
> +static int do_test (void);
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
OK.
> +
> +#define get_handles_fdopen(filename, fd, fp, fd_mode, mode) \
> +({ \
> + int ret = 0; \
> + (fd) = open ((filename), (fd_mode), 0); \
> + if ((fd) == -1) \
> + { \
> + printf ("open failed: %m\n"); \
> + ret = 1;
OK.
\
> + } \
> + else \
> + { \
> + (fp) = fdopen ((fd), (mode)); \
> + if ((fp) == NULL) \
> + { \
> + printf ("fdopen failed: %m\n"); \
> + close (fd); \
> + ret = 1; \
OK.
> + } \
> + } \
> + ret; \
> +})
> +
> +#define get_handles_fopen(filename, fd, fp, mode) \
> +({ \
> + int ret = 0; \
> + (fp) = fopen ((filename), (mode)); \
> + if ((fp) == NULL) \
> + { \
> + printf ("fopen failed: %m\n"); \
> + ret = 1;
OK.
\
> + } \
> + else \
> + { \
> + (fd) = fileno (fp); \
> + if ((fd) == -1) \
> + { \
> + printf ("fileno failed: %m\n"); \
> + ret = 1; \
OK.
> + } \
> + } \
> + ret; \
> +})
OK.
> +
> +static const void *data;
> +static const char *char_data = "abcdef";
> +static const wchar_t *wide_data = L"abcdef";
> +static size_t data_len;
> +/* Maintain the current file length for validation. */
> +static size_t file_len;
> +
> +typedef int (*fputs_func_t) (const void *data, FILE *fp);
> +fputs_func_t fputs_func;
> +
Nit.
The test function needs a detailed comment about how you computed the
expected result. Future maintainers will need to update this value
and without knowing the first principles you applied to arrive at
your result will mean they will need to redo the same work you have
already done.
This is true for each of the do_*_test functions you wrote.
I mention this only once, but it applies to all the test functions.
> +/* Test that the value of ftell is not cached when the stream handle is not
> + active. */
> +static int
> +do_ftell_test (const char *filename)
> +{
> + int ret = 0;
> + struct test
> + {
> + const char *mode;
> + int fd_mode;
> + size_t old_off;
> + size_t new_off;
> + } test_modes[] = {
> + {"w", O_WRONLY, 0, data_len},
> + {"w+", O_RDWR, 0, data_len},
> + {"r+", O_RDWR, 0, data_len},
> + {"a", O_WRONLY, data_len, 2 * data_len},
> + {"a+", O_RDWR, 2 * data_len, 3 * data_len},
> + };
> + for (int j = 0; j < 2; j++)
> + {
> + for (int i = 0; i < sizeof (test_modes) / sizeof (struct test); i++)
> + {
> + FILE *fp;
> + int fd;
> + printf ("\tftell: %s (file, \"%s\"): ", j == 0 ? "fdopen" : "fopen",
> + test_modes[i].mode);
> +
> + if (j == 0)
> + ret = get_handles_fdopen (filename, fd, fp, test_modes[i].fd_mode,
> + test_modes[i].mode);
> + else
> + ret = get_handles_fopen (filename, fd, fp, test_modes[i].mode);
> +
> + if (ret != 0)
> + return 1;
Optional nit:
Why not `return ret;' as in the other parts of the testsuite?
> +
> + long off = ftell (fp);
> + if (off != test_modes[i].old_off)
> + {
> + printf ("Incorrect old offset. Expected %zu but got %ld, ",
> + test_modes[i].old_off, off);
> + ret |= 1;
OK.
> + }
> + else
> + printf ("old offset = %ld, ", off);
> +
> + int ret = write (fd, data, data_len);
> + off = ftell (fp);
> +
> + if (off != test_modes[i].new_off)
> + {
> + printf ("Incorrect new offset. Expected %zu but got %ld\n",
> + test_modes[i].old_off, off);
> + ret |= 1;
> + }
> + else
> + printf ("new offset = %ld\n", off);
> +
> + fclose (fp);
> + }
> + }
> +
> + return ret;
> +}
> +
> +/* This test opens the file for writing, moves the file offset of the
> + underlying file, writes out data and then checks if ftell trips on it. */
> +static int
> +do_write_test (const char *filename)
> +{
> + FILE *fp = NULL;
> + int fd;
> + int ret = 0;
> + struct test
> + {
> + const char *mode;
> + int fd_mode;
> + } test_modes[] = {
> + {"w", O_WRONLY},
> + {"w+", O_RDWR},
> + {"r+", O_RDWR}
> + };
> +
> + for (int j = 0; j < 2; j++)
> + {
> + for (int i = 0; i < sizeof (test_modes) / sizeof (struct test); i++)
> + {
> + printf ("\twrite: %s (file, \"%s\"): ", j == 0 ? "fopen" : "fdopen",
> + test_modes[i].mode);
> +
> + if (j == 0)
> + ret = get_handles_fopen (filename, fd, fp, test_modes[i].mode);
> + else
> + ret = get_handles_fdopen (filename, fd, fp, test_modes[i].fd_mode,
> + test_modes[i].mode);
> +
> + if (ret != 0)
> + return ret;
> +
> + /* Move offset to just before the end of the file. */
> + off_t ret = lseek (fd, file_len - 1, SEEK_SET);
> + if (ret == -1)
> + {
> + printf ("lseek failed: %m\n");
> + ret |= 1;
> + }
> +
> + /* Write some data. */
> + size_t written = fputs_func (data, fp);
> +
> + if (written == EOF)
> + {
> + printf ("fputs[1] failed to write data\n");
> + ret |= 1;
> + }
> +
> + /* Verify that the offset points to the end of the file. */
> + long offset = ftell (fp);
> + file_len = file_len - 1 + data_len;
> +
> + if (offset != file_len)
> + {
> + printf ("Incorrect offset. Expected %zu, but got %ld\n",
> + file_len, offset);
> +
> + ret |= 1;
> + }
> +
> + printf ("offset = %ld\n", offset);
> + fclose (fp);
> + }
> + }
> +
> + return ret;
> +}
OK.
> +
> +/* This test opens a file in append mode, writes some data, and then verifies
> + that ftell does not trip over it. */
> +static int
> +do_append_test (const char *filename)
> +{
> + FILE *fp = NULL;
> + int ret = 0;
> + int fd;
> +
> + struct test
> + {
> + const char *mode;
> + int fd_mode;
> + } test_modes[] = {
> + {"a", O_WRONLY},
> + {"a+", O_RDWR}
> + };
> +
> + for (int j = 0; j < 2; j++)
> + {
> + for (int i = 0; i < sizeof (test_modes) / sizeof (struct test); i++)
> + {
> + printf ("\tappend: %s (file, \"%s\"): ", j == 0 ? "fopen" : "fdopen",
> + test_modes[i].mode);
> +
> + if (j == 0)
> + ret = get_handles_fopen (filename, fd, fp, test_modes[i].mode);
> + else
> + ret = get_handles_fdopen (filename, fd, fp, test_modes[i].fd_mode,
> + test_modes[i].mode);
> +
> + if (ret != 0)
> + return ret;
> +
> + /* Write some data. */
> + size_t written = fputs_func (data, fp);
> +
> + if (written == EOF)
> + {
> + printf ("fputs[1] failed to write all data\n");
> + ret |= 1;
> + }
> +
> + /* Verify that the offset points to the end of the file. */
> + long offset = ftell (fp);
> + file_len += data_len;
> +
> + if (offset != file_len)
> + {
> + printf ("Incorrect offset. Expected %zu, but got %ld\n",
> + file_len, offset);
> +
> + ret |= 1;
> + }
> +
> + printf ("offset = %ld\n", offset);
> + fclose (fp);
> + }
> + }
> +
> + return ret;
> +}
OK.
> +
> +static int
> +do_one_test (const char *filename)
> +{
> + int ret = 0;
> +
> + ret |= do_ftell_test (filename);
> + ret |= do_write_test (filename);
> + ret |= do_append_test (filename);
> +
> + return ret;
> +}
> +
> +static int
> +do_test (void)
> +{
> + int ret = 0;
> + FILE *fp = NULL;
> + char *filename;
> + size_t written;
> + int fd = create_temp_file ("tst-active-handler-tmp.", &filename);
> +
> + if (fd == -1)
> + {
> + printf ("create_temp_file: %m\n");
> + return 1;
> + }
> +
> + fp = fdopen (fd, "w");
> + if (fp == NULL)
> + {
> + printf ("fdopen[0]: %m\n");
> + close (fd);
> + return 1;
> + }
> +
> + data = char_data;
> + data_len = strlen (char_data);
> + file_len = strlen (char_data);
> + written = fputs (data, fp);
> +
> + if (written == EOF)
> + {
> + printf ("fputs[1] failed to write data\n");
> + ret = 1;
> + }
> +
> + fclose (fp);
> + if (ret)
> + return ret;
> +
> + /* Tests for regular files. */
> + puts ("Regular mode:");
> + fputs_func = (fputs_func_t) fputs;
> + data = char_data;
> + data_len = strlen (char_data);
> + ret |= do_one_test (filename);
> +
> + /* Truncate the file before repeating the tests in wide mode. */
> + fp = fopen (filename, "w");
> + if (fp == NULL)
> + {
> + printf ("fopen failed %m\n");
> + return 1;
> + }
> + fclose (fp);
> +
> + /* Tests for wide files. */
> + puts ("Wide mode:");
> + if (setlocale (LC_ALL, "en_US.UTF-8") == NULL)
> + {
> + printf ("Cannot set en_US.UTF-8 locale.\n");
> + return 1;
> + }
> + fputs_func = (fputs_func_t) fputws;
> + data = wide_data;
> + data_len = wcslen (wide_data);
> + ret |= do_one_test (filename);
> +
> + return ret;
OK.
> +}
> diff --git a/libio/wfileops.c b/libio/wfileops.c
> index 9cebe77..eda7828 100644
> --- a/libio/wfileops.c
> +++ b/libio/wfileops.c
> @@ -596,29 +596,22 @@ done:
> return 0;
> }
>
Nit.
Need a detailed comment about what do_ftell_wide does.
> -_IO_off64_t
> -_IO_wfile_seekoff (fp, offset, dir, mode)
> - _IO_FILE *fp;
> - _IO_off64_t offset;
> - int dir;
> - int mode;
> +static _IO_off64_t
> +do_ftell_wide (_IO_FILE *fp)
> {
> - _IO_off64_t result;
> - _IO_off64_t delta, new_offset;
> - long int count;
> - /* POSIX.1 8.2.3.7 says that after a call the fflush() the file
> - offset of the underlying file must be exact. */
> - int must_be_exact = ((fp->_wide_data->_IO_read_base
> - == fp->_wide_data->_IO_read_end)
> - && (fp->_wide_data->_IO_write_base
> - == fp->_wide_data->_IO_write_ptr));
> + _IO_off64_t result, offset = 0;
OK.
>
> - bool was_writing = ((fp->_wide_data->_IO_write_ptr
> - > fp->_wide_data->_IO_write_base)
> - || _IO_in_put_mode (fp));
> -
> - if (mode == 0)
> + /* No point looking for offsets in the buffer if it hasn't even been
> + allocated. */
> + if (fp->_wide_data->_IO_buf_base != NULL)
> {
> + const wchar_t *wide_read_base;
> + const wchar_t *wide_read_ptr;
> + const wchar_t *wide_read_end;
> + bool was_writing = ((fp->_wide_data->_IO_write_ptr
> + > fp->_wide_data->_IO_write_base)
> + || _IO_in_put_mode (fp));
OK.
> +
> /* XXX For wide stream with backup store it is not very
> reasonable to determine the offset. The pushed-back
> character might require a state change and we need not be
> @@ -633,14 +626,117 @@ _IO_wfile_seekoff (fp, offset, dir, mode)
> return -1;
> }
>
> - /* There is no more data in the backup buffer. We can
> - switch back. */
> - _IO_switch_to_main_wget_area (fp);
> + /* Nothing in the backup store, so note the backed up pointers
> + without changing the state. */
> + wide_read_base = fp->_wide_data->_IO_save_base;
> + wide_read_ptr = wide_read_base;
> + wide_read_end = fp->_wide_data->_IO_save_end;
> + }
> + else
> + {
> + wide_read_base = fp->_wide_data->_IO_read_base;
> + wide_read_ptr = fp->_wide_data->_IO_read_ptr;
> + wide_read_end = fp->_wide_data->_IO_read_end;
OK.
> }
>
> - dir = _IO_seek_cur, offset = 0; /* Don't move any pointers. */
> + struct _IO_codecvt *cv = fp->_codecvt;
> + int clen = (*cv->__codecvt_do_encoding) (cv);
> +
> + if (!was_writing)
> + {
> + if (clen > 0)
> + {
> + offset -= (wide_read_end - wide_read_ptr) * clen;
> + offset -= fp->_IO_read_end - fp->_IO_read_ptr;
OK.
> + }
> + else
> + {
> + int nread;
> +
> + size_t delta = wide_read_ptr - wide_read_base;
> + __mbstate_t state = fp->_wide_data->_IO_last_state;
> + nread = (*cv->__codecvt_do_length) (cv, &state,
> + fp->_IO_read_base,
> + fp->_IO_read_end, delta);
> + offset -= fp->_IO_read_end - fp->_IO_read_base - nread;
OK.
> + }
> + }
> + else
> + {
> + if (clen > 0)
> + offset += (fp->_wide_data->_IO_write_ptr
> + - fp->_wide_data->_IO_write_base) * clen;
> + else
> + {
> + size_t delta = (fp->_wide_data->_IO_write_ptr
> + - fp->_wide_data->_IO_write_base);
> +
> + /* Allocate enough space for the conversion. */
> + size_t outsize = delta * sizeof (wchar_t);
> + char *out = malloc (outsize);
> + char *outstop = out;
> + const wchar_t *in = fp->_wide_data->_IO_write_base;
> +
> + enum __codecvt_result status;
> +
> + __mbstate_t state = fp->_wide_data->_IO_last_state;
> + status = (*cv->__codecvt_do_out) (cv, &state,
> + in, in + delta, &in,
> + out, out + outsize, &outstop);
> +
> + /* We don't check for __codecvt_partial because it can be
> + returned on one of two conditions: either the output
> + buffer is full or the input sequence is incomplete. We
> + take care to allocate enough buffer and our input
> + sequences must be complete since they are accepted as
> + wchar_t; if not, then that is an error. */
> + if (__glibc_unlikely (status != __codecvt_ok))
> + return WEOF;
> +
> + offset += outstop - out;
> + }
> +
> + /* _IO_read_end coincides with fp._offset, so the actual file position
> + is fp._offset - (_IO_read_end - new_write_ptr). */
> + offset -= fp->_IO_read_end - fp->_IO_write_ptr;
> + }
> }
>
> + result = get_file_offset (fp);
> +
> + if (result == EOF)
> + return result;
> +
> + result += offset;
> +
> + return result;
OK.
And that ends the refactoring of the `mode == 0' case plus any changes.
> +}
> +
> +_IO_off64_t
> +_IO_wfile_seekoff (fp, offset, dir, mode)
> + _IO_FILE *fp;
> + _IO_off64_t offset;
> + int dir;
> + int mode;
> +{
> + _IO_off64_t result;
> + _IO_off64_t delta, new_offset;
> + long int count;
> +
Nit.
Needs a comment about why we are shunting here.
> + if (mode == 0)
> + return do_ftell_wide (fp);
OK.
> +
> + /* POSIX.1 8.2.3.7 says that after a call the fflush() the file
> + offset of the underlying file must be exact. */
> + int must_be_exact = ((fp->_wide_data->_IO_read_base
> + == fp->_wide_data->_IO_read_end)
> + && (fp->_wide_data->_IO_write_base
> + == fp->_wide_data->_IO_write_ptr));
> +
> + bool was_writing = ((fp->_wide_data->_IO_write_ptr
> + > fp->_wide_data->_IO_write_base)
> + || _IO_in_put_mode (fp));
OK.
> +
> /* Flush unwritten characters.
> (This may do an unneeded write if we seek within the buffer.
> But to be able to switch to reading, we would need to set
> @@ -648,7 +744,7 @@ _IO_wfile_seekoff (fp, offset, dir, mode)
> which assumes file_ptr() is eGptr. Anyway, since we probably
> end up flushing when we close(), it doesn't make much difference.)
> FIXME: simulate mem-mapped files. */
> - else if (was_writing && _IO_switch_to_wget_mode (fp))
> + if (was_writing && _IO_switch_to_wget_mode (fp))
> return WEOF;
OK.
>
> if (fp->_wide_data->_IO_buf_base == NULL)
> @@ -693,7 +789,6 @@ _IO_wfile_seekoff (fp, offset, dir, mode)
> {
> int nread;
>
> - flushed:
> delta = (fp->_wide_data->_IO_read_ptr
> - fp->_wide_data->_IO_read_base);
> fp->_wide_data->_IO_state = fp->_wide_data->_IO_last_state;
> @@ -706,80 +801,9 @@ _IO_wfile_seekoff (fp, offset, dir, mode)
> offset -= fp->_IO_read_end - fp->_IO_read_base - nread;
> }
> }
> - else
> - {
> - char *new_write_ptr = fp->_IO_write_ptr;
> -
> - if (clen > 0)
> - offset += (fp->_wide_data->_IO_write_ptr
> - - fp->_wide_data->_IO_write_base) / clen;
> - else
> - {
> - enum __codecvt_result status = __codecvt_ok;
> - delta = (fp->_wide_data->_IO_write_ptr
> - - fp->_wide_data->_IO_write_base);
> - const wchar_t *write_base = fp->_wide_data->_IO_write_base;
> -
> - /* FIXME: This actually ends up in two iterations of conversion,
> - one here and the next when the buffer actually gets flushed.
> - It may be possible to optimize this in future so that
> - wdo_write identifies already converted content and does not
> - redo it. In any case, this is much better than having to
> - flush buffers for every ftell. */
> - do
> - {
> - /* There is not enough space in the buffer to do the entire
> - conversion, so there is no point trying to avoid the
> - buffer flush. Just do it and go back to how it was with
> - the read mode. */
> - if (status == __codecvt_partial
> - || (delta > 0 && new_write_ptr == fp->_IO_buf_end))
> - {
> - if (_IO_switch_to_wget_mode (fp))
> - return WEOF;
> - goto flushed;
> - }
> -
> - const wchar_t *new_wbase = fp->_wide_data->_IO_write_base;
> - fp->_wide_data->_IO_state = fp->_wide_data->_IO_last_state;
> - status = (*cv->__codecvt_do_out) (cv,
> - &fp->_wide_data->_IO_state,
> - write_base,
> - write_base + delta,
> - &new_wbase,
> - new_write_ptr,
> - fp->_IO_buf_end,
> - &new_write_ptr);
> -
> - delta -= new_wbase - write_base;
> -
> - /* If there was an error, then return WEOF.
> - TODO: set buffer state. */
> - if (__glibc_unlikely (status == __codecvt_error))
> - return WEOF;
> - }
> - while (delta > 0);
> - }
> -
> - /* _IO_read_end coincides with fp._offset, so the actual file position
> - is fp._offset - (_IO_read_end - new_write_ptr). This is fine
> - even if fp._offset is not set, since fp->_IO_read_end is then at
> - _IO_buf_base and this adjustment is for unbuffered output. */
> - offset -= fp->_IO_read_end - new_write_ptr;
> - }
>
> if (fp->_offset == _IO_pos_BAD)
> - {
> - if (mode != 0)
> - goto dumb;
> - else
> - {
> - result = _IO_SYSSEEK (fp, 0, dir);
> - if (result == EOF)
> - return result;
> - fp->_offset = result;
> - }
> - }
> + goto dumb;
OK.
>
> /* Make offset absolute, assuming current pointer is file_ptr(). */
> offset += fp->_offset;
> @@ -802,10 +826,6 @@ _IO_wfile_seekoff (fp, offset, dir, mode)
> }
> /* At this point, dir==_IO_seek_set. */
>
> - /* If we are only interested in the current position we've found it now. */
> - if (mode == 0)
> - return offset;
OK.
> -
> /* If destination is within current buffer, optimize: */
> if (fp->_offset != _IO_pos_BAD && fp->_IO_read_base != NULL
> && !_IO_in_backup (fp))
>
Cheers,
Carlos.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQEcBAEBAgAGBQJTFQ0hAAoJECXvCkNsKkr/2zYIAJ763aRvsLzle8MeZiXFMILr
2Z1/Sy5jUXoRZtdEc/pmFXtghqsI1Li88sXXXXBAlqaZSJt4G4Dnd1fPBV4xpDzi
r+ygt1KRpphRnl3aN7XUiDPpXXPid58SSlLPdBg6wMzlCOGFseUn3KT3hX9yryYF
MVIKidBv/ika3SxJW6YrdDyJKAOpYzrquQ7NXFuRA1STUoDkOYhYN6H8eq6OWtGv
6eDyCUve56YHJLn0bhVX5IkVfhrYIkxKBBlELOjLXXPPJaKqW0iyv72z4MfIHGCb
KoEuUMICZRTL1UHo0+E9L5MXHJIOrlZxbjc6hc6uvFEf4Z+coLBA1SfPCEBK158=
=VsoG
-----END PGP SIGNATURE-----