| Message ID | 20140319070755.GA25315@spoyarek.pnq.redhat.com |
|---|---|
| State | New |
| Headers | show |
Ping! On Wed, Mar 19, 2014 at 12:37:55PM +0530, Siddhesh Poyarekar wrote: > The test case had a stupid error that got caught when I tested on > powerpc. Updated patch. > > The offset computation in write mode uses the fact that _IO_read_end > is kept in sync with the external file offset. This however is not > true when O_APPEND is in effect since switching to write mode ought to > send the external file offset to the end of file without making the > necessary adjustment to _IO_read_end. > > Hence in append mode, offset computation when writing should only > consider the effect of unflushed writes, i.e. from _IO_write_base to > _IO_write_ptr. > > Tested on x86_64 (and not x86_66 like I mentioned earlier ;)), i686, > s390 and ppc. > > Siddhesh > > [BZ #16724] > * libio/tst-ftell-append.c: New test case. > * libio/Makefile (tests): Add test case. > * libio/fileops.c (do_ftell): Don't trust _IO_read_end when in > append mode. > * libio/wfileops.c (do_ftell_wide): Likewise. > > --- > libio/Makefile | 3 +- > libio/fileops.c | 12 +++- > libio/tst-ftell-append.c | 169 +++++++++++++++++++++++++++++++++++++++++++++++ > libio/wfileops.c | 13 +++- > 4 files changed, 192 insertions(+), 5 deletions(-) > create mode 100644 libio/tst-ftell-append.c > > diff --git a/libio/Makefile b/libio/Makefile > index 69c25c0..4bedfad 100644 > --- a/libio/Makefile > +++ b/libio/Makefile > @@ -60,7 +60,8 @@ 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-ftell-active-handler > + tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \ > + tst-ftell-append > 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 cf68dbf..204cfea 100644 > --- a/libio/fileops.c > +++ b/libio/fileops.c > @@ -91,7 +91,9 @@ extern struct __gconv_trans_data __libio_translit attribute_hidden; > > The position in the buffer that corresponds to the position > in external file system is normally _IO_read_end, except in putback > - mode, when it is _IO_save_end. > + mode, when it is _IO_save_end and also when the file is in append mode, > + since switching from read to write mode automatically sends the position in > + the external file system to the end of file. > If the field _fb._offset is >= 0, it gives the offset in > the file as a whole corresponding to eGptr(). (?) > > @@ -966,6 +968,14 @@ do_ftell (_IO_FILE *fp) > /* Adjust for unflushed data. */ > if (!was_writing) > offset -= fp->_IO_read_end - fp->_IO_read_ptr; > + /* We don't trust _IO_read_end to represent the current file offset when > + writing in append mode because the value would have to be shifted to > + the end of the file during a flush. Use the write base instead, along > + with the new offset we got above when we did a seek to the end of the > + file. */ > + else if (append_mode) > + offset += fp->_IO_write_ptr - fp->_IO_write_base; > + /* For all other modes, _IO_read_end represents the file offset. */ > else > offset += fp->_IO_write_ptr - fp->_IO_read_end; > } > diff --git a/libio/tst-ftell-append.c b/libio/tst-ftell-append.c > new file mode 100644 > index 0000000..604dc03 > --- /dev/null > +++ b/libio/tst-ftell-append.c > @@ -0,0 +1,169 @@ > +/* Verify that ftell returns the correct value after a read and a write on a > + file opened in a+ mode. > + 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 <locale.h> > +#include <wchar.h> > + > +/* data points to either char_data or wide_data, depending on whether we're > + testing regular file mode or wide mode respectively. Similarly, > + fputs_func points to either fputs or fputws. data_len keeps track of the > + length of the current data and file_len maintains the current file > + length. */ > +#define BUF_LEN 4 > +static void *buf; > +static char char_buf[BUF_LEN]; > +static wchar_t wide_buf[BUF_LEN]; > +static const void *data; > +static const char *char_data = "abcdefghijklmnopqrstuvwxyz"; > +static const wchar_t *wide_data = L"abcdefghijklmnopqrstuvwxyz"; > +static size_t data_len; > +static size_t file_len; > + > +typedef int (*fputs_func_t) (const void *data, FILE *fp); > +fputs_func_t fputs_func; > + > +typedef void *(*fgets_func_t) (void *s, int size, FILE *stream); > +fgets_func_t fgets_func; > + > +static int do_test (void); > + > +#define TEST_FUNCTION do_test () > +#include "../test-skeleton.c" > + > +static FILE * > +init_file (const char *filename) > +{ > + FILE *fp = fopen (filename, "w"); > + if (fp == NULL) > + { > + printf ("fopen: %m\n"); > + return NULL; > + } > + > + int written = fputs_func (data, fp); > + > + if (written == EOF) > + { > + printf ("fputs failed to write data\n"); > + fclose (fp); > + return NULL; > + } > + > + file_len = data_len; > + > + fclose (fp); > + > + fp = fopen (filename, "a+"); > + if (fp == NULL) > + { > + printf ("fopen(a+): %m\n"); > + return NULL; > + } > + > + return fp; > +} > + > +static int > +do_one_test (const char *filename) > +{ > + FILE *fp = init_file (filename); > + > + if (fp == NULL) > + return 1; > + > + void *ret = fgets_func (buf, BUF_LEN, fp); > + > + if (ret == NULL) > + { > + printf ("read failed: %m\n"); > + fclose (fp); > + return 1; > + } > + > + int written = fputs_func (data, fp); > + > + if (written == EOF) > + { > + printf ("fputs failed to write data\n"); > + fclose (fp); > + return 1; > + } > + > + file_len += data_len; > + > + long off = ftell (fp); > + > + if (off != file_len) > + { > + printf ("Incorrect offset %ld, expected %zu\n", off, file_len); > + fclose (fp); > + return 1; > + } > + else > + printf ("Correct offset %ld after write.\n", off); > + > + return 0; > +} > + > +/* Run the tests for regular files and wide mode files. */ > +static int > +do_test (void) > +{ > + int ret = 0; > + char *filename; > + int fd = create_temp_file ("tst-ftell-append-tmp.", &filename); > + > + if (fd == -1) > + { > + printf ("create_temp_file: %m\n"); > + return 1; > + } > + > + close (fd); > + > + /* Tests for regular files. */ > + puts ("Regular mode:"); > + fputs_func = (fputs_func_t) fputs; > + fgets_func = (fgets_func_t) fgets; > + data = char_data; > + buf = char_buf; > + data_len = strlen (char_data); > + ret |= do_one_test (filename); > + > + /* 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; > + fgets_func = (fgets_func_t) fgetws; > + data = wide_data; > + buf = wide_buf; > + data_len = wcslen (wide_data); > + ret |= do_one_test (filename); > + > + return ret; > +} > diff --git a/libio/wfileops.c b/libio/wfileops.c > index 3199861..f123add 100644 > --- a/libio/wfileops.c > +++ b/libio/wfileops.c > @@ -713,9 +713,16 @@ do_ftell_wide (_IO_FILE *fp) > 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; > + /* We don't trust _IO_read_end to represent the current file offset > + when writing in append mode because the value would have to be > + shifted to the end of the file during a flush. Use the write base > + instead, along with the new offset we got above when we did a seek > + to the end of the file. */ > + if (append_mode) > + offset += fp->_IO_write_ptr - fp->_IO_write_base; > + /* For all other modes, _IO_read_end represents the file offset. */ > + else > + offset += fp->_IO_write_ptr - fp->_IO_read_end; > } > } > > -- > 1.8.3.1 >
This fell off my ping radar, so... Ping! On Mon, Mar 24, 2014 at 09:21:38PM +0530, Siddhesh Poyarekar wrote: > Ping! > > On Wed, Mar 19, 2014 at 12:37:55PM +0530, Siddhesh Poyarekar wrote: > > The test case had a stupid error that got caught when I tested on > > powerpc. Updated patch. > > > > The offset computation in write mode uses the fact that _IO_read_end > > is kept in sync with the external file offset. This however is not > > true when O_APPEND is in effect since switching to write mode ought to > > send the external file offset to the end of file without making the > > necessary adjustment to _IO_read_end. > > > > Hence in append mode, offset computation when writing should only > > consider the effect of unflushed writes, i.e. from _IO_write_base to > > _IO_write_ptr. > > > > Tested on x86_64 (and not x86_66 like I mentioned earlier ;)), i686, > > s390 and ppc. > > > > Siddhesh > > > > [BZ #16724] > > * libio/tst-ftell-append.c: New test case. > > * libio/Makefile (tests): Add test case. > > * libio/fileops.c (do_ftell): Don't trust _IO_read_end when in > > append mode. > > * libio/wfileops.c (do_ftell_wide): Likewise. > > > > --- > > libio/Makefile | 3 +- > > libio/fileops.c | 12 +++- > > libio/tst-ftell-append.c | 169 +++++++++++++++++++++++++++++++++++++++++++++++ > > libio/wfileops.c | 13 +++- > > 4 files changed, 192 insertions(+), 5 deletions(-) > > create mode 100644 libio/tst-ftell-append.c > > > > diff --git a/libio/Makefile b/libio/Makefile > > index 69c25c0..4bedfad 100644 > > --- a/libio/Makefile > > +++ b/libio/Makefile > > @@ -60,7 +60,8 @@ 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-ftell-active-handler > > + tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \ > > + tst-ftell-append > > 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 cf68dbf..204cfea 100644 > > --- a/libio/fileops.c > > +++ b/libio/fileops.c > > @@ -91,7 +91,9 @@ extern struct __gconv_trans_data __libio_translit attribute_hidden; > > > > The position in the buffer that corresponds to the position > > in external file system is normally _IO_read_end, except in putback > > - mode, when it is _IO_save_end. > > + mode, when it is _IO_save_end and also when the file is in append mode, > > + since switching from read to write mode automatically sends the position in > > + the external file system to the end of file. > > If the field _fb._offset is >= 0, it gives the offset in > > the file as a whole corresponding to eGptr(). (?) > > > > @@ -966,6 +968,14 @@ do_ftell (_IO_FILE *fp) > > /* Adjust for unflushed data. */ > > if (!was_writing) > > offset -= fp->_IO_read_end - fp->_IO_read_ptr; > > + /* We don't trust _IO_read_end to represent the current file offset when > > + writing in append mode because the value would have to be shifted to > > + the end of the file during a flush. Use the write base instead, along > > + with the new offset we got above when we did a seek to the end of the > > + file. */ > > + else if (append_mode) > > + offset += fp->_IO_write_ptr - fp->_IO_write_base; > > + /* For all other modes, _IO_read_end represents the file offset. */ > > else > > offset += fp->_IO_write_ptr - fp->_IO_read_end; > > } > > diff --git a/libio/tst-ftell-append.c b/libio/tst-ftell-append.c > > new file mode 100644 > > index 0000000..604dc03 > > --- /dev/null > > +++ b/libio/tst-ftell-append.c > > @@ -0,0 +1,169 @@ > > +/* Verify that ftell returns the correct value after a read and a write on a > > + file opened in a+ mode. > > + 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 <locale.h> > > +#include <wchar.h> > > + > > +/* data points to either char_data or wide_data, depending on whether we're > > + testing regular file mode or wide mode respectively. Similarly, > > + fputs_func points to either fputs or fputws. data_len keeps track of the > > + length of the current data and file_len maintains the current file > > + length. */ > > +#define BUF_LEN 4 > > +static void *buf; > > +static char char_buf[BUF_LEN]; > > +static wchar_t wide_buf[BUF_LEN]; > > +static const void *data; > > +static const char *char_data = "abcdefghijklmnopqrstuvwxyz"; > > +static const wchar_t *wide_data = L"abcdefghijklmnopqrstuvwxyz"; > > +static size_t data_len; > > +static size_t file_len; > > + > > +typedef int (*fputs_func_t) (const void *data, FILE *fp); > > +fputs_func_t fputs_func; > > + > > +typedef void *(*fgets_func_t) (void *s, int size, FILE *stream); > > +fgets_func_t fgets_func; > > + > > +static int do_test (void); > > + > > +#define TEST_FUNCTION do_test () > > +#include "../test-skeleton.c" > > + > > +static FILE * > > +init_file (const char *filename) > > +{ > > + FILE *fp = fopen (filename, "w"); > > + if (fp == NULL) > > + { > > + printf ("fopen: %m\n"); > > + return NULL; > > + } > > + > > + int written = fputs_func (data, fp); > > + > > + if (written == EOF) > > + { > > + printf ("fputs failed to write data\n"); > > + fclose (fp); > > + return NULL; > > + } > > + > > + file_len = data_len; > > + > > + fclose (fp); > > + > > + fp = fopen (filename, "a+"); > > + if (fp == NULL) > > + { > > + printf ("fopen(a+): %m\n"); > > + return NULL; > > + } > > + > > + return fp; > > +} > > + > > +static int > > +do_one_test (const char *filename) > > +{ > > + FILE *fp = init_file (filename); > > + > > + if (fp == NULL) > > + return 1; > > + > > + void *ret = fgets_func (buf, BUF_LEN, fp); > > + > > + if (ret == NULL) > > + { > > + printf ("read failed: %m\n"); > > + fclose (fp); > > + return 1; > > + } > > + > > + int written = fputs_func (data, fp); > > + > > + if (written == EOF) > > + { > > + printf ("fputs failed to write data\n"); > > + fclose (fp); > > + return 1; > > + } > > + > > + file_len += data_len; > > + > > + long off = ftell (fp); > > + > > + if (off != file_len) > > + { > > + printf ("Incorrect offset %ld, expected %zu\n", off, file_len); > > + fclose (fp); > > + return 1; > > + } > > + else > > + printf ("Correct offset %ld after write.\n", off); > > + > > + return 0; > > +} > > + > > +/* Run the tests for regular files and wide mode files. */ > > +static int > > +do_test (void) > > +{ > > + int ret = 0; > > + char *filename; > > + int fd = create_temp_file ("tst-ftell-append-tmp.", &filename); > > + > > + if (fd == -1) > > + { > > + printf ("create_temp_file: %m\n"); > > + return 1; > > + } > > + > > + close (fd); > > + > > + /* Tests for regular files. */ > > + puts ("Regular mode:"); > > + fputs_func = (fputs_func_t) fputs; > > + fgets_func = (fgets_func_t) fgets; > > + data = char_data; > > + buf = char_buf; > > + data_len = strlen (char_data); > > + ret |= do_one_test (filename); > > + > > + /* 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; > > + fgets_func = (fgets_func_t) fgetws; > > + data = wide_data; > > + buf = wide_buf; > > + data_len = wcslen (wide_data); > > + ret |= do_one_test (filename); > > + > > + return ret; > > +} > > diff --git a/libio/wfileops.c b/libio/wfileops.c > > index 3199861..f123add 100644 > > --- a/libio/wfileops.c > > +++ b/libio/wfileops.c > > @@ -713,9 +713,16 @@ do_ftell_wide (_IO_FILE *fp) > > 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; > > + /* We don't trust _IO_read_end to represent the current file offset > > + when writing in append mode because the value would have to be > > + shifted to the end of the file during a flush. Use the write base > > + instead, along with the new offset we got above when we did a seek > > + to the end of the file. */ > > + if (append_mode) > > + offset += fp->_IO_write_ptr - fp->_IO_write_base; > > + /* For all other modes, _IO_read_end represents the file offset. */ > > + else > > + offset += fp->_IO_write_ptr - fp->_IO_read_end; > > } > > } > > > > -- > > 1.8.3.1 > > > >
Ping! On Fri, Apr 11, 2014 at 03:11:12PM +0530, Siddhesh Poyarekar wrote: > This fell off my ping radar, so... > > Ping! > > On Mon, Mar 24, 2014 at 09:21:38PM +0530, Siddhesh Poyarekar wrote: > > Ping! > > > > On Wed, Mar 19, 2014 at 12:37:55PM +0530, Siddhesh Poyarekar wrote: > > > The test case had a stupid error that got caught when I tested on > > > powerpc. Updated patch. > > > > > > The offset computation in write mode uses the fact that _IO_read_end > > > is kept in sync with the external file offset. This however is not > > > true when O_APPEND is in effect since switching to write mode ought to > > > send the external file offset to the end of file without making the > > > necessary adjustment to _IO_read_end. > > > > > > Hence in append mode, offset computation when writing should only > > > consider the effect of unflushed writes, i.e. from _IO_write_base to > > > _IO_write_ptr. > > > > > > Tested on x86_64 (and not x86_66 like I mentioned earlier ;)), i686, > > > s390 and ppc. > > > > > > Siddhesh > > > > > > [BZ #16724] > > > * libio/tst-ftell-append.c: New test case. > > > * libio/Makefile (tests): Add test case. > > > * libio/fileops.c (do_ftell): Don't trust _IO_read_end when in > > > append mode. > > > * libio/wfileops.c (do_ftell_wide): Likewise. > > > > > > --- > > > libio/Makefile | 3 +- > > > libio/fileops.c | 12 +++- > > > libio/tst-ftell-append.c | 169 +++++++++++++++++++++++++++++++++++++++++++++++ > > > libio/wfileops.c | 13 +++- > > > 4 files changed, 192 insertions(+), 5 deletions(-) > > > create mode 100644 libio/tst-ftell-append.c > > > > > > diff --git a/libio/Makefile b/libio/Makefile > > > index 69c25c0..4bedfad 100644 > > > --- a/libio/Makefile > > > +++ b/libio/Makefile > > > @@ -60,7 +60,8 @@ 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-ftell-active-handler > > > + tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \ > > > + tst-ftell-append > > > 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 cf68dbf..204cfea 100644 > > > --- a/libio/fileops.c > > > +++ b/libio/fileops.c > > > @@ -91,7 +91,9 @@ extern struct __gconv_trans_data __libio_translit attribute_hidden; > > > > > > The position in the buffer that corresponds to the position > > > in external file system is normally _IO_read_end, except in putback > > > - mode, when it is _IO_save_end. > > > + mode, when it is _IO_save_end and also when the file is in append mode, > > > + since switching from read to write mode automatically sends the position in > > > + the external file system to the end of file. > > > If the field _fb._offset is >= 0, it gives the offset in > > > the file as a whole corresponding to eGptr(). (?) > > > > > > @@ -966,6 +968,14 @@ do_ftell (_IO_FILE *fp) > > > /* Adjust for unflushed data. */ > > > if (!was_writing) > > > offset -= fp->_IO_read_end - fp->_IO_read_ptr; > > > + /* We don't trust _IO_read_end to represent the current file offset when > > > + writing in append mode because the value would have to be shifted to > > > + the end of the file during a flush. Use the write base instead, along > > > + with the new offset we got above when we did a seek to the end of the > > > + file. */ > > > + else if (append_mode) > > > + offset += fp->_IO_write_ptr - fp->_IO_write_base; > > > + /* For all other modes, _IO_read_end represents the file offset. */ > > > else > > > offset += fp->_IO_write_ptr - fp->_IO_read_end; > > > } > > > diff --git a/libio/tst-ftell-append.c b/libio/tst-ftell-append.c > > > new file mode 100644 > > > index 0000000..604dc03 > > > --- /dev/null > > > +++ b/libio/tst-ftell-append.c > > > @@ -0,0 +1,169 @@ > > > +/* Verify that ftell returns the correct value after a read and a write on a > > > + file opened in a+ mode. > > > + 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 <locale.h> > > > +#include <wchar.h> > > > + > > > +/* data points to either char_data or wide_data, depending on whether we're > > > + testing regular file mode or wide mode respectively. Similarly, > > > + fputs_func points to either fputs or fputws. data_len keeps track of the > > > + length of the current data and file_len maintains the current file > > > + length. */ > > > +#define BUF_LEN 4 > > > +static void *buf; > > > +static char char_buf[BUF_LEN]; > > > +static wchar_t wide_buf[BUF_LEN]; > > > +static const void *data; > > > +static const char *char_data = "abcdefghijklmnopqrstuvwxyz"; > > > +static const wchar_t *wide_data = L"abcdefghijklmnopqrstuvwxyz"; > > > +static size_t data_len; > > > +static size_t file_len; > > > + > > > +typedef int (*fputs_func_t) (const void *data, FILE *fp); > > > +fputs_func_t fputs_func; > > > + > > > +typedef void *(*fgets_func_t) (void *s, int size, FILE *stream); > > > +fgets_func_t fgets_func; > > > + > > > +static int do_test (void); > > > + > > > +#define TEST_FUNCTION do_test () > > > +#include "../test-skeleton.c" > > > + > > > +static FILE * > > > +init_file (const char *filename) > > > +{ > > > + FILE *fp = fopen (filename, "w"); > > > + if (fp == NULL) > > > + { > > > + printf ("fopen: %m\n"); > > > + return NULL; > > > + } > > > + > > > + int written = fputs_func (data, fp); > > > + > > > + if (written == EOF) > > > + { > > > + printf ("fputs failed to write data\n"); > > > + fclose (fp); > > > + return NULL; > > > + } > > > + > > > + file_len = data_len; > > > + > > > + fclose (fp); > > > + > > > + fp = fopen (filename, "a+"); > > > + if (fp == NULL) > > > + { > > > + printf ("fopen(a+): %m\n"); > > > + return NULL; > > > + } > > > + > > > + return fp; > > > +} > > > + > > > +static int > > > +do_one_test (const char *filename) > > > +{ > > > + FILE *fp = init_file (filename); > > > + > > > + if (fp == NULL) > > > + return 1; > > > + > > > + void *ret = fgets_func (buf, BUF_LEN, fp); > > > + > > > + if (ret == NULL) > > > + { > > > + printf ("read failed: %m\n"); > > > + fclose (fp); > > > + return 1; > > > + } > > > + > > > + int written = fputs_func (data, fp); > > > + > > > + if (written == EOF) > > > + { > > > + printf ("fputs failed to write data\n"); > > > + fclose (fp); > > > + return 1; > > > + } > > > + > > > + file_len += data_len; > > > + > > > + long off = ftell (fp); > > > + > > > + if (off != file_len) > > > + { > > > + printf ("Incorrect offset %ld, expected %zu\n", off, file_len); > > > + fclose (fp); > > > + return 1; > > > + } > > > + else > > > + printf ("Correct offset %ld after write.\n", off); > > > + > > > + return 0; > > > +} > > > + > > > +/* Run the tests for regular files and wide mode files. */ > > > +static int > > > +do_test (void) > > > +{ > > > + int ret = 0; > > > + char *filename; > > > + int fd = create_temp_file ("tst-ftell-append-tmp.", &filename); > > > + > > > + if (fd == -1) > > > + { > > > + printf ("create_temp_file: %m\n"); > > > + return 1; > > > + } > > > + > > > + close (fd); > > > + > > > + /* Tests for regular files. */ > > > + puts ("Regular mode:"); > > > + fputs_func = (fputs_func_t) fputs; > > > + fgets_func = (fgets_func_t) fgets; > > > + data = char_data; > > > + buf = char_buf; > > > + data_len = strlen (char_data); > > > + ret |= do_one_test (filename); > > > + > > > + /* 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; > > > + fgets_func = (fgets_func_t) fgetws; > > > + data = wide_data; > > > + buf = wide_buf; > > > + data_len = wcslen (wide_data); > > > + ret |= do_one_test (filename); > > > + > > > + return ret; > > > +} > > > diff --git a/libio/wfileops.c b/libio/wfileops.c > > > index 3199861..f123add 100644 > > > --- a/libio/wfileops.c > > > +++ b/libio/wfileops.c > > > @@ -713,9 +713,16 @@ do_ftell_wide (_IO_FILE *fp) > > > 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; > > > + /* We don't trust _IO_read_end to represent the current file offset > > > + when writing in append mode because the value would have to be > > > + shifted to the end of the file during a flush. Use the write base > > > + instead, along with the new offset we got above when we did a seek > > > + to the end of the file. */ > > > + if (append_mode) > > > + offset += fp->_IO_write_ptr - fp->_IO_write_base; > > > + /* For all other modes, _IO_read_end represents the file offset. */ > > > + else > > > + offset += fp->_IO_write_ptr - fp->_IO_read_end; > > > } > > > } > > > > > > -- > > > 1.8.3.1 > > > > > > > > >
Ping! On Fri, Apr 18, 2014 at 01:02:23PM +0530, Siddhesh Poyarekar wrote: > Ping! > > On Fri, Apr 11, 2014 at 03:11:12PM +0530, Siddhesh Poyarekar wrote: > > This fell off my ping radar, so... > > > > Ping! > > > > On Mon, Mar 24, 2014 at 09:21:38PM +0530, Siddhesh Poyarekar wrote: > > > Ping! > > > > > > On Wed, Mar 19, 2014 at 12:37:55PM +0530, Siddhesh Poyarekar wrote: > > > > The test case had a stupid error that got caught when I tested on > > > > powerpc. Updated patch. > > > > > > > > The offset computation in write mode uses the fact that _IO_read_end > > > > is kept in sync with the external file offset. This however is not > > > > true when O_APPEND is in effect since switching to write mode ought to > > > > send the external file offset to the end of file without making the > > > > necessary adjustment to _IO_read_end. > > > > > > > > Hence in append mode, offset computation when writing should only > > > > consider the effect of unflushed writes, i.e. from _IO_write_base to > > > > _IO_write_ptr. > > > > > > > > Tested on x86_64 (and not x86_66 like I mentioned earlier ;)), i686, > > > > s390 and ppc. > > > > > > > > Siddhesh > > > > > > > > [BZ #16724] > > > > * libio/tst-ftell-append.c: New test case. > > > > * libio/Makefile (tests): Add test case. > > > > * libio/fileops.c (do_ftell): Don't trust _IO_read_end when in > > > > append mode. > > > > * libio/wfileops.c (do_ftell_wide): Likewise. > > > > > > > > --- > > > > libio/Makefile | 3 +- > > > > libio/fileops.c | 12 +++- > > > > libio/tst-ftell-append.c | 169 +++++++++++++++++++++++++++++++++++++++++++++++ > > > > libio/wfileops.c | 13 +++- > > > > 4 files changed, 192 insertions(+), 5 deletions(-) > > > > create mode 100644 libio/tst-ftell-append.c > > > > > > > > diff --git a/libio/Makefile b/libio/Makefile > > > > index 69c25c0..4bedfad 100644 > > > > --- a/libio/Makefile > > > > +++ b/libio/Makefile > > > > @@ -60,7 +60,8 @@ 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-ftell-active-handler > > > > + tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \ > > > > + tst-ftell-append > > > > 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 cf68dbf..204cfea 100644 > > > > --- a/libio/fileops.c > > > > +++ b/libio/fileops.c > > > > @@ -91,7 +91,9 @@ extern struct __gconv_trans_data __libio_translit attribute_hidden; > > > > > > > > The position in the buffer that corresponds to the position > > > > in external file system is normally _IO_read_end, except in putback > > > > - mode, when it is _IO_save_end. > > > > + mode, when it is _IO_save_end and also when the file is in append mode, > > > > + since switching from read to write mode automatically sends the position in > > > > + the external file system to the end of file. > > > > If the field _fb._offset is >= 0, it gives the offset in > > > > the file as a whole corresponding to eGptr(). (?) > > > > > > > > @@ -966,6 +968,14 @@ do_ftell (_IO_FILE *fp) > > > > /* Adjust for unflushed data. */ > > > > if (!was_writing) > > > > offset -= fp->_IO_read_end - fp->_IO_read_ptr; > > > > + /* We don't trust _IO_read_end to represent the current file offset when > > > > + writing in append mode because the value would have to be shifted to > > > > + the end of the file during a flush. Use the write base instead, along > > > > + with the new offset we got above when we did a seek to the end of the > > > > + file. */ > > > > + else if (append_mode) > > > > + offset += fp->_IO_write_ptr - fp->_IO_write_base; > > > > + /* For all other modes, _IO_read_end represents the file offset. */ > > > > else > > > > offset += fp->_IO_write_ptr - fp->_IO_read_end; > > > > } > > > > diff --git a/libio/tst-ftell-append.c b/libio/tst-ftell-append.c > > > > new file mode 100644 > > > > index 0000000..604dc03 > > > > --- /dev/null > > > > +++ b/libio/tst-ftell-append.c > > > > @@ -0,0 +1,169 @@ > > > > +/* Verify that ftell returns the correct value after a read and a write on a > > > > + file opened in a+ mode. > > > > + 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 <locale.h> > > > > +#include <wchar.h> > > > > + > > > > +/* data points to either char_data or wide_data, depending on whether we're > > > > + testing regular file mode or wide mode respectively. Similarly, > > > > + fputs_func points to either fputs or fputws. data_len keeps track of the > > > > + length of the current data and file_len maintains the current file > > > > + length. */ > > > > +#define BUF_LEN 4 > > > > +static void *buf; > > > > +static char char_buf[BUF_LEN]; > > > > +static wchar_t wide_buf[BUF_LEN]; > > > > +static const void *data; > > > > +static const char *char_data = "abcdefghijklmnopqrstuvwxyz"; > > > > +static const wchar_t *wide_data = L"abcdefghijklmnopqrstuvwxyz"; > > > > +static size_t data_len; > > > > +static size_t file_len; > > > > + > > > > +typedef int (*fputs_func_t) (const void *data, FILE *fp); > > > > +fputs_func_t fputs_func; > > > > + > > > > +typedef void *(*fgets_func_t) (void *s, int size, FILE *stream); > > > > +fgets_func_t fgets_func; > > > > + > > > > +static int do_test (void); > > > > + > > > > +#define TEST_FUNCTION do_test () > > > > +#include "../test-skeleton.c" > > > > + > > > > +static FILE * > > > > +init_file (const char *filename) > > > > +{ > > > > + FILE *fp = fopen (filename, "w"); > > > > + if (fp == NULL) > > > > + { > > > > + printf ("fopen: %m\n"); > > > > + return NULL; > > > > + } > > > > + > > > > + int written = fputs_func (data, fp); > > > > + > > > > + if (written == EOF) > > > > + { > > > > + printf ("fputs failed to write data\n"); > > > > + fclose (fp); > > > > + return NULL; > > > > + } > > > > + > > > > + file_len = data_len; > > > > + > > > > + fclose (fp); > > > > + > > > > + fp = fopen (filename, "a+"); > > > > + if (fp == NULL) > > > > + { > > > > + printf ("fopen(a+): %m\n"); > > > > + return NULL; > > > > + } > > > > + > > > > + return fp; > > > > +} > > > > + > > > > +static int > > > > +do_one_test (const char *filename) > > > > +{ > > > > + FILE *fp = init_file (filename); > > > > + > > > > + if (fp == NULL) > > > > + return 1; > > > > + > > > > + void *ret = fgets_func (buf, BUF_LEN, fp); > > > > + > > > > + if (ret == NULL) > > > > + { > > > > + printf ("read failed: %m\n"); > > > > + fclose (fp); > > > > + return 1; > > > > + } > > > > + > > > > + int written = fputs_func (data, fp); > > > > + > > > > + if (written == EOF) > > > > + { > > > > + printf ("fputs failed to write data\n"); > > > > + fclose (fp); > > > > + return 1; > > > > + } > > > > + > > > > + file_len += data_len; > > > > + > > > > + long off = ftell (fp); > > > > + > > > > + if (off != file_len) > > > > + { > > > > + printf ("Incorrect offset %ld, expected %zu\n", off, file_len); > > > > + fclose (fp); > > > > + return 1; > > > > + } > > > > + else > > > > + printf ("Correct offset %ld after write.\n", off); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +/* Run the tests for regular files and wide mode files. */ > > > > +static int > > > > +do_test (void) > > > > +{ > > > > + int ret = 0; > > > > + char *filename; > > > > + int fd = create_temp_file ("tst-ftell-append-tmp.", &filename); > > > > + > > > > + if (fd == -1) > > > > + { > > > > + printf ("create_temp_file: %m\n"); > > > > + return 1; > > > > + } > > > > + > > > > + close (fd); > > > > + > > > > + /* Tests for regular files. */ > > > > + puts ("Regular mode:"); > > > > + fputs_func = (fputs_func_t) fputs; > > > > + fgets_func = (fgets_func_t) fgets; > > > > + data = char_data; > > > > + buf = char_buf; > > > > + data_len = strlen (char_data); > > > > + ret |= do_one_test (filename); > > > > + > > > > + /* 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; > > > > + fgets_func = (fgets_func_t) fgetws; > > > > + data = wide_data; > > > > + buf = wide_buf; > > > > + data_len = wcslen (wide_data); > > > > + ret |= do_one_test (filename); > > > > + > > > > + return ret; > > > > +} > > > > diff --git a/libio/wfileops.c b/libio/wfileops.c > > > > index 3199861..f123add 100644 > > > > --- a/libio/wfileops.c > > > > +++ b/libio/wfileops.c > > > > @@ -713,9 +713,16 @@ do_ftell_wide (_IO_FILE *fp) > > > > 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; > > > > + /* We don't trust _IO_read_end to represent the current file offset > > > > + when writing in append mode because the value would have to be > > > > + shifted to the end of the file during a flush. Use the write base > > > > + instead, along with the new offset we got above when we did a seek > > > > + to the end of the file. */ > > > > + if (append_mode) > > > > + offset += fp->_IO_write_ptr - fp->_IO_write_base; > > > > + /* For all other modes, _IO_read_end represents the file offset. */ > > > > + else > > > > + offset += fp->_IO_write_ptr - fp->_IO_read_end; > > > > } > > > > } > > > > > > > > -- > > > > 1.8.3.1 > > > > > > > > > > > > > > > >
Ping! On Tue, Apr 22, 2014 at 03:17:32PM +0530, Siddhesh Poyarekar wrote: > Ping! > > On Fri, Apr 18, 2014 at 01:02:23PM +0530, Siddhesh Poyarekar wrote: > > Ping! > > > > On Fri, Apr 11, 2014 at 03:11:12PM +0530, Siddhesh Poyarekar wrote: > > > This fell off my ping radar, so... > > > > > > Ping! > > > > > > On Mon, Mar 24, 2014 at 09:21:38PM +0530, Siddhesh Poyarekar wrote: > > > > Ping! > > > > > > > > On Wed, Mar 19, 2014 at 12:37:55PM +0530, Siddhesh Poyarekar wrote: > > > > > The test case had a stupid error that got caught when I tested on > > > > > powerpc. Updated patch. > > > > > > > > > > The offset computation in write mode uses the fact that _IO_read_end > > > > > is kept in sync with the external file offset. This however is not > > > > > true when O_APPEND is in effect since switching to write mode ought to > > > > > send the external file offset to the end of file without making the > > > > > necessary adjustment to _IO_read_end. > > > > > > > > > > Hence in append mode, offset computation when writing should only > > > > > consider the effect of unflushed writes, i.e. from _IO_write_base to > > > > > _IO_write_ptr. > > > > > > > > > > Tested on x86_64 (and not x86_66 like I mentioned earlier ;)), i686, > > > > > s390 and ppc. > > > > > > > > > > Siddhesh > > > > > > > > > > [BZ #16724] > > > > > * libio/tst-ftell-append.c: New test case. > > > > > * libio/Makefile (tests): Add test case. > > > > > * libio/fileops.c (do_ftell): Don't trust _IO_read_end when in > > > > > append mode. > > > > > * libio/wfileops.c (do_ftell_wide): Likewise. > > > > > > > > > > --- > > > > > libio/Makefile | 3 +- > > > > > libio/fileops.c | 12 +++- > > > > > libio/tst-ftell-append.c | 169 +++++++++++++++++++++++++++++++++++++++++++++++ > > > > > libio/wfileops.c | 13 +++- > > > > > 4 files changed, 192 insertions(+), 5 deletions(-) > > > > > create mode 100644 libio/tst-ftell-append.c > > > > > > > > > > diff --git a/libio/Makefile b/libio/Makefile > > > > > index 69c25c0..4bedfad 100644 > > > > > --- a/libio/Makefile > > > > > +++ b/libio/Makefile > > > > > @@ -60,7 +60,8 @@ 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-ftell-active-handler > > > > > + tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \ > > > > > + tst-ftell-append > > > > > 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 cf68dbf..204cfea 100644 > > > > > --- a/libio/fileops.c > > > > > +++ b/libio/fileops.c > > > > > @@ -91,7 +91,9 @@ extern struct __gconv_trans_data __libio_translit attribute_hidden; > > > > > > > > > > The position in the buffer that corresponds to the position > > > > > in external file system is normally _IO_read_end, except in putback > > > > > - mode, when it is _IO_save_end. > > > > > + mode, when it is _IO_save_end and also when the file is in append mode, > > > > > + since switching from read to write mode automatically sends the position in > > > > > + the external file system to the end of file. > > > > > If the field _fb._offset is >= 0, it gives the offset in > > > > > the file as a whole corresponding to eGptr(). (?) > > > > > > > > > > @@ -966,6 +968,14 @@ do_ftell (_IO_FILE *fp) > > > > > /* Adjust for unflushed data. */ > > > > > if (!was_writing) > > > > > offset -= fp->_IO_read_end - fp->_IO_read_ptr; > > > > > + /* We don't trust _IO_read_end to represent the current file offset when > > > > > + writing in append mode because the value would have to be shifted to > > > > > + the end of the file during a flush. Use the write base instead, along > > > > > + with the new offset we got above when we did a seek to the end of the > > > > > + file. */ > > > > > + else if (append_mode) > > > > > + offset += fp->_IO_write_ptr - fp->_IO_write_base; > > > > > + /* For all other modes, _IO_read_end represents the file offset. */ > > > > > else > > > > > offset += fp->_IO_write_ptr - fp->_IO_read_end; > > > > > } > > > > > diff --git a/libio/tst-ftell-append.c b/libio/tst-ftell-append.c > > > > > new file mode 100644 > > > > > index 0000000..604dc03 > > > > > --- /dev/null > > > > > +++ b/libio/tst-ftell-append.c > > > > > @@ -0,0 +1,169 @@ > > > > > +/* Verify that ftell returns the correct value after a read and a write on a > > > > > + file opened in a+ mode. > > > > > + 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 <locale.h> > > > > > +#include <wchar.h> > > > > > + > > > > > +/* data points to either char_data or wide_data, depending on whether we're > > > > > + testing regular file mode or wide mode respectively. Similarly, > > > > > + fputs_func points to either fputs or fputws. data_len keeps track of the > > > > > + length of the current data and file_len maintains the current file > > > > > + length. */ > > > > > +#define BUF_LEN 4 > > > > > +static void *buf; > > > > > +static char char_buf[BUF_LEN]; > > > > > +static wchar_t wide_buf[BUF_LEN]; > > > > > +static const void *data; > > > > > +static const char *char_data = "abcdefghijklmnopqrstuvwxyz"; > > > > > +static const wchar_t *wide_data = L"abcdefghijklmnopqrstuvwxyz"; > > > > > +static size_t data_len; > > > > > +static size_t file_len; > > > > > + > > > > > +typedef int (*fputs_func_t) (const void *data, FILE *fp); > > > > > +fputs_func_t fputs_func; > > > > > + > > > > > +typedef void *(*fgets_func_t) (void *s, int size, FILE *stream); > > > > > +fgets_func_t fgets_func; > > > > > + > > > > > +static int do_test (void); > > > > > + > > > > > +#define TEST_FUNCTION do_test () > > > > > +#include "../test-skeleton.c" > > > > > + > > > > > +static FILE * > > > > > +init_file (const char *filename) > > > > > +{ > > > > > + FILE *fp = fopen (filename, "w"); > > > > > + if (fp == NULL) > > > > > + { > > > > > + printf ("fopen: %m\n"); > > > > > + return NULL; > > > > > + } > > > > > + > > > > > + int written = fputs_func (data, fp); > > > > > + > > > > > + if (written == EOF) > > > > > + { > > > > > + printf ("fputs failed to write data\n"); > > > > > + fclose (fp); > > > > > + return NULL; > > > > > + } > > > > > + > > > > > + file_len = data_len; > > > > > + > > > > > + fclose (fp); > > > > > + > > > > > + fp = fopen (filename, "a+"); > > > > > + if (fp == NULL) > > > > > + { > > > > > + printf ("fopen(a+): %m\n"); > > > > > + return NULL; > > > > > + } > > > > > + > > > > > + return fp; > > > > > +} > > > > > + > > > > > +static int > > > > > +do_one_test (const char *filename) > > > > > +{ > > > > > + FILE *fp = init_file (filename); > > > > > + > > > > > + if (fp == NULL) > > > > > + return 1; > > > > > + > > > > > + void *ret = fgets_func (buf, BUF_LEN, fp); > > > > > + > > > > > + if (ret == NULL) > > > > > + { > > > > > + printf ("read failed: %m\n"); > > > > > + fclose (fp); > > > > > + return 1; > > > > > + } > > > > > + > > > > > + int written = fputs_func (data, fp); > > > > > + > > > > > + if (written == EOF) > > > > > + { > > > > > + printf ("fputs failed to write data\n"); > > > > > + fclose (fp); > > > > > + return 1; > > > > > + } > > > > > + > > > > > + file_len += data_len; > > > > > + > > > > > + long off = ftell (fp); > > > > > + > > > > > + if (off != file_len) > > > > > + { > > > > > + printf ("Incorrect offset %ld, expected %zu\n", off, file_len); > > > > > + fclose (fp); > > > > > + return 1; > > > > > + } > > > > > + else > > > > > + printf ("Correct offset %ld after write.\n", off); > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > +/* Run the tests for regular files and wide mode files. */ > > > > > +static int > > > > > +do_test (void) > > > > > +{ > > > > > + int ret = 0; > > > > > + char *filename; > > > > > + int fd = create_temp_file ("tst-ftell-append-tmp.", &filename); > > > > > + > > > > > + if (fd == -1) > > > > > + { > > > > > + printf ("create_temp_file: %m\n"); > > > > > + return 1; > > > > > + } > > > > > + > > > > > + close (fd); > > > > > + > > > > > + /* Tests for regular files. */ > > > > > + puts ("Regular mode:"); > > > > > + fputs_func = (fputs_func_t) fputs; > > > > > + fgets_func = (fgets_func_t) fgets; > > > > > + data = char_data; > > > > > + buf = char_buf; > > > > > + data_len = strlen (char_data); > > > > > + ret |= do_one_test (filename); > > > > > + > > > > > + /* 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; > > > > > + fgets_func = (fgets_func_t) fgetws; > > > > > + data = wide_data; > > > > > + buf = wide_buf; > > > > > + data_len = wcslen (wide_data); > > > > > + ret |= do_one_test (filename); > > > > > + > > > > > + return ret; > > > > > +} > > > > > diff --git a/libio/wfileops.c b/libio/wfileops.c > > > > > index 3199861..f123add 100644 > > > > > --- a/libio/wfileops.c > > > > > +++ b/libio/wfileops.c > > > > > @@ -713,9 +713,16 @@ do_ftell_wide (_IO_FILE *fp) > > > > > 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; > > > > > + /* We don't trust _IO_read_end to represent the current file offset > > > > > + when writing in append mode because the value would have to be > > > > > + shifted to the end of the file during a flush. Use the write base > > > > > + instead, along with the new offset we got above when we did a seek > > > > > + to the end of the file. */ > > > > > + if (append_mode) > > > > > + offset += fp->_IO_write_ptr - fp->_IO_write_base; > > > > > + /* For all other modes, _IO_read_end represents the file offset. */ > > > > > + else > > > > > + offset += fp->_IO_write_ptr - fp->_IO_read_end; > > > > > } > > > > > } > > > > > > > > > > -- > > > > > 1.8.3.1 > > > > > > > > > > > > > > > > > > > > > > > > >
Ping! On Mon, Apr 28, 2014 at 08:20:01PM +0530, Siddhesh Poyarekar wrote: > Ping! > > On Tue, Apr 22, 2014 at 03:17:32PM +0530, Siddhesh Poyarekar wrote: > > Ping! > > > > On Fri, Apr 18, 2014 at 01:02:23PM +0530, Siddhesh Poyarekar wrote: > > > Ping! > > > > > > On Fri, Apr 11, 2014 at 03:11:12PM +0530, Siddhesh Poyarekar wrote: > > > > This fell off my ping radar, so... > > > > > > > > Ping! > > > > > > > > On Mon, Mar 24, 2014 at 09:21:38PM +0530, Siddhesh Poyarekar wrote: > > > > > Ping! > > > > > > > > > > On Wed, Mar 19, 2014 at 12:37:55PM +0530, Siddhesh Poyarekar wrote: > > > > > > The test case had a stupid error that got caught when I tested on > > > > > > powerpc. Updated patch. > > > > > > > > > > > > The offset computation in write mode uses the fact that _IO_read_end > > > > > > is kept in sync with the external file offset. This however is not > > > > > > true when O_APPEND is in effect since switching to write mode ought to > > > > > > send the external file offset to the end of file without making the > > > > > > necessary adjustment to _IO_read_end. > > > > > > > > > > > > Hence in append mode, offset computation when writing should only > > > > > > consider the effect of unflushed writes, i.e. from _IO_write_base to > > > > > > _IO_write_ptr. > > > > > > > > > > > > Tested on x86_64 (and not x86_66 like I mentioned earlier ;)), i686, > > > > > > s390 and ppc. > > > > > > > > > > > > Siddhesh > > > > > > > > > > > > [BZ #16724] > > > > > > * libio/tst-ftell-append.c: New test case. > > > > > > * libio/Makefile (tests): Add test case. > > > > > > * libio/fileops.c (do_ftell): Don't trust _IO_read_end when in > > > > > > append mode. > > > > > > * libio/wfileops.c (do_ftell_wide): Likewise. > > > > > > > > > > > > --- > > > > > > libio/Makefile | 3 +- > > > > > > libio/fileops.c | 12 +++- > > > > > > libio/tst-ftell-append.c | 169 +++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > libio/wfileops.c | 13 +++- > > > > > > 4 files changed, 192 insertions(+), 5 deletions(-) > > > > > > create mode 100644 libio/tst-ftell-append.c > > > > > > > > > > > > diff --git a/libio/Makefile b/libio/Makefile > > > > > > index 69c25c0..4bedfad 100644 > > > > > > --- a/libio/Makefile > > > > > > +++ b/libio/Makefile > > > > > > @@ -60,7 +60,8 @@ 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-ftell-active-handler > > > > > > + tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \ > > > > > > + tst-ftell-append > > > > > > 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 cf68dbf..204cfea 100644 > > > > > > --- a/libio/fileops.c > > > > > > +++ b/libio/fileops.c > > > > > > @@ -91,7 +91,9 @@ extern struct __gconv_trans_data __libio_translit attribute_hidden; > > > > > > > > > > > > The position in the buffer that corresponds to the position > > > > > > in external file system is normally _IO_read_end, except in putback > > > > > > - mode, when it is _IO_save_end. > > > > > > + mode, when it is _IO_save_end and also when the file is in append mode, > > > > > > + since switching from read to write mode automatically sends the position in > > > > > > + the external file system to the end of file. > > > > > > If the field _fb._offset is >= 0, it gives the offset in > > > > > > the file as a whole corresponding to eGptr(). (?) > > > > > > > > > > > > @@ -966,6 +968,14 @@ do_ftell (_IO_FILE *fp) > > > > > > /* Adjust for unflushed data. */ > > > > > > if (!was_writing) > > > > > > offset -= fp->_IO_read_end - fp->_IO_read_ptr; > > > > > > + /* We don't trust _IO_read_end to represent the current file offset when > > > > > > + writing in append mode because the value would have to be shifted to > > > > > > + the end of the file during a flush. Use the write base instead, along > > > > > > + with the new offset we got above when we did a seek to the end of the > > > > > > + file. */ > > > > > > + else if (append_mode) > > > > > > + offset += fp->_IO_write_ptr - fp->_IO_write_base; > > > > > > + /* For all other modes, _IO_read_end represents the file offset. */ > > > > > > else > > > > > > offset += fp->_IO_write_ptr - fp->_IO_read_end; > > > > > > } > > > > > > diff --git a/libio/tst-ftell-append.c b/libio/tst-ftell-append.c > > > > > > new file mode 100644 > > > > > > index 0000000..604dc03 > > > > > > --- /dev/null > > > > > > +++ b/libio/tst-ftell-append.c > > > > > > @@ -0,0 +1,169 @@ > > > > > > +/* Verify that ftell returns the correct value after a read and a write on a > > > > > > + file opened in a+ mode. > > > > > > + 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 <locale.h> > > > > > > +#include <wchar.h> > > > > > > + > > > > > > +/* data points to either char_data or wide_data, depending on whether we're > > > > > > + testing regular file mode or wide mode respectively. Similarly, > > > > > > + fputs_func points to either fputs or fputws. data_len keeps track of the > > > > > > + length of the current data and file_len maintains the current file > > > > > > + length. */ > > > > > > +#define BUF_LEN 4 > > > > > > +static void *buf; > > > > > > +static char char_buf[BUF_LEN]; > > > > > > +static wchar_t wide_buf[BUF_LEN]; > > > > > > +static const void *data; > > > > > > +static const char *char_data = "abcdefghijklmnopqrstuvwxyz"; > > > > > > +static const wchar_t *wide_data = L"abcdefghijklmnopqrstuvwxyz"; > > > > > > +static size_t data_len; > > > > > > +static size_t file_len; > > > > > > + > > > > > > +typedef int (*fputs_func_t) (const void *data, FILE *fp); > > > > > > +fputs_func_t fputs_func; > > > > > > + > > > > > > +typedef void *(*fgets_func_t) (void *s, int size, FILE *stream); > > > > > > +fgets_func_t fgets_func; > > > > > > + > > > > > > +static int do_test (void); > > > > > > + > > > > > > +#define TEST_FUNCTION do_test () > > > > > > +#include "../test-skeleton.c" > > > > > > + > > > > > > +static FILE * > > > > > > +init_file (const char *filename) > > > > > > +{ > > > > > > + FILE *fp = fopen (filename, "w"); > > > > > > + if (fp == NULL) > > > > > > + { > > > > > > + printf ("fopen: %m\n"); > > > > > > + return NULL; > > > > > > + } > > > > > > + > > > > > > + int written = fputs_func (data, fp); > > > > > > + > > > > > > + if (written == EOF) > > > > > > + { > > > > > > + printf ("fputs failed to write data\n"); > > > > > > + fclose (fp); > > > > > > + return NULL; > > > > > > + } > > > > > > + > > > > > > + file_len = data_len; > > > > > > + > > > > > > + fclose (fp); > > > > > > + > > > > > > + fp = fopen (filename, "a+"); > > > > > > + if (fp == NULL) > > > > > > + { > > > > > > + printf ("fopen(a+): %m\n"); > > > > > > + return NULL; > > > > > > + } > > > > > > + > > > > > > + return fp; > > > > > > +} > > > > > > + > > > > > > +static int > > > > > > +do_one_test (const char *filename) > > > > > > +{ > > > > > > + FILE *fp = init_file (filename); > > > > > > + > > > > > > + if (fp == NULL) > > > > > > + return 1; > > > > > > + > > > > > > + void *ret = fgets_func (buf, BUF_LEN, fp); > > > > > > + > > > > > > + if (ret == NULL) > > > > > > + { > > > > > > + printf ("read failed: %m\n"); > > > > > > + fclose (fp); > > > > > > + return 1; > > > > > > + } > > > > > > + > > > > > > + int written = fputs_func (data, fp); > > > > > > + > > > > > > + if (written == EOF) > > > > > > + { > > > > > > + printf ("fputs failed to write data\n"); > > > > > > + fclose (fp); > > > > > > + return 1; > > > > > > + } > > > > > > + > > > > > > + file_len += data_len; > > > > > > + > > > > > > + long off = ftell (fp); > > > > > > + > > > > > > + if (off != file_len) > > > > > > + { > > > > > > + printf ("Incorrect offset %ld, expected %zu\n", off, file_len); > > > > > > + fclose (fp); > > > > > > + return 1; > > > > > > + } > > > > > > + else > > > > > > + printf ("Correct offset %ld after write.\n", off); > > > > > > + > > > > > > + return 0; > > > > > > +} > > > > > > + > > > > > > +/* Run the tests for regular files and wide mode files. */ > > > > > > +static int > > > > > > +do_test (void) > > > > > > +{ > > > > > > + int ret = 0; > > > > > > + char *filename; > > > > > > + int fd = create_temp_file ("tst-ftell-append-tmp.", &filename); > > > > > > + > > > > > > + if (fd == -1) > > > > > > + { > > > > > > + printf ("create_temp_file: %m\n"); > > > > > > + return 1; > > > > > > + } > > > > > > + > > > > > > + close (fd); > > > > > > + > > > > > > + /* Tests for regular files. */ > > > > > > + puts ("Regular mode:"); > > > > > > + fputs_func = (fputs_func_t) fputs; > > > > > > + fgets_func = (fgets_func_t) fgets; > > > > > > + data = char_data; > > > > > > + buf = char_buf; > > > > > > + data_len = strlen (char_data); > > > > > > + ret |= do_one_test (filename); > > > > > > + > > > > > > + /* 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; > > > > > > + fgets_func = (fgets_func_t) fgetws; > > > > > > + data = wide_data; > > > > > > + buf = wide_buf; > > > > > > + data_len = wcslen (wide_data); > > > > > > + ret |= do_one_test (filename); > > > > > > + > > > > > > + return ret; > > > > > > +} > > > > > > diff --git a/libio/wfileops.c b/libio/wfileops.c > > > > > > index 3199861..f123add 100644 > > > > > > --- a/libio/wfileops.c > > > > > > +++ b/libio/wfileops.c > > > > > > @@ -713,9 +713,16 @@ do_ftell_wide (_IO_FILE *fp) > > > > > > 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; > > > > > > + /* We don't trust _IO_read_end to represent the current file offset > > > > > > + when writing in append mode because the value would have to be > > > > > > + shifted to the end of the file during a flush. Use the write base > > > > > > + instead, along with the new offset we got above when we did a seek > > > > > > + to the end of the file. */ > > > > > > + if (append_mode) > > > > > > + offset += fp->_IO_write_ptr - fp->_IO_write_base; > > > > > > + /* For all other modes, _IO_read_end represents the file offset. */ > > > > > > + else > > > > > > + offset += fp->_IO_write_ptr - fp->_IO_read_end; > > > > > > } > > > > > > } > > > > > > > > > > > > -- > > > > > > 1.8.3.1 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
Ping! On Thu, May 15, 2014 at 02:09:27PM +0530, Siddhesh Poyarekar wrote: > Ping! > > On Mon, Apr 28, 2014 at 08:20:01PM +0530, Siddhesh Poyarekar wrote: > > Ping! > > > > On Tue, Apr 22, 2014 at 03:17:32PM +0530, Siddhesh Poyarekar wrote: > > > Ping! > > > > > > On Fri, Apr 18, 2014 at 01:02:23PM +0530, Siddhesh Poyarekar wrote: > > > > Ping! > > > > > > > > On Fri, Apr 11, 2014 at 03:11:12PM +0530, Siddhesh Poyarekar wrote: > > > > > This fell off my ping radar, so... > > > > > > > > > > Ping! > > > > > > > > > > On Mon, Mar 24, 2014 at 09:21:38PM +0530, Siddhesh Poyarekar wrote: > > > > > > Ping! > > > > > > > > > > > > On Wed, Mar 19, 2014 at 12:37:55PM +0530, Siddhesh Poyarekar wrote: > > > > > > > The test case had a stupid error that got caught when I tested on > > > > > > > powerpc. Updated patch. > > > > > > > > > > > > > > The offset computation in write mode uses the fact that _IO_read_end > > > > > > > is kept in sync with the external file offset. This however is not > > > > > > > true when O_APPEND is in effect since switching to write mode ought to > > > > > > > send the external file offset to the end of file without making the > > > > > > > necessary adjustment to _IO_read_end. > > > > > > > > > > > > > > Hence in append mode, offset computation when writing should only > > > > > > > consider the effect of unflushed writes, i.e. from _IO_write_base to > > > > > > > _IO_write_ptr. > > > > > > > > > > > > > > Tested on x86_64 (and not x86_66 like I mentioned earlier ;)), i686, > > > > > > > s390 and ppc. > > > > > > > > > > > > > > Siddhesh > > > > > > > > > > > > > > [BZ #16724] > > > > > > > * libio/tst-ftell-append.c: New test case. > > > > > > > * libio/Makefile (tests): Add test case. > > > > > > > * libio/fileops.c (do_ftell): Don't trust _IO_read_end when in > > > > > > > append mode. > > > > > > > * libio/wfileops.c (do_ftell_wide): Likewise. > > > > > > > > > > > > > > --- > > > > > > > libio/Makefile | 3 +- > > > > > > > libio/fileops.c | 12 +++- > > > > > > > libio/tst-ftell-append.c | 169 +++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > > libio/wfileops.c | 13 +++- > > > > > > > 4 files changed, 192 insertions(+), 5 deletions(-) > > > > > > > create mode 100644 libio/tst-ftell-append.c > > > > > > > > > > > > > > diff --git a/libio/Makefile b/libio/Makefile > > > > > > > index 69c25c0..4bedfad 100644 > > > > > > > --- a/libio/Makefile > > > > > > > +++ b/libio/Makefile > > > > > > > @@ -60,7 +60,8 @@ 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-ftell-active-handler > > > > > > > + tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \ > > > > > > > + tst-ftell-append > > > > > > > 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 cf68dbf..204cfea 100644 > > > > > > > --- a/libio/fileops.c > > > > > > > +++ b/libio/fileops.c > > > > > > > @@ -91,7 +91,9 @@ extern struct __gconv_trans_data __libio_translit attribute_hidden; > > > > > > > > > > > > > > The position in the buffer that corresponds to the position > > > > > > > in external file system is normally _IO_read_end, except in putback > > > > > > > - mode, when it is _IO_save_end. > > > > > > > + mode, when it is _IO_save_end and also when the file is in append mode, > > > > > > > + since switching from read to write mode automatically sends the position in > > > > > > > + the external file system to the end of file. > > > > > > > If the field _fb._offset is >= 0, it gives the offset in > > > > > > > the file as a whole corresponding to eGptr(). (?) > > > > > > > > > > > > > > @@ -966,6 +968,14 @@ do_ftell (_IO_FILE *fp) > > > > > > > /* Adjust for unflushed data. */ > > > > > > > if (!was_writing) > > > > > > > offset -= fp->_IO_read_end - fp->_IO_read_ptr; > > > > > > > + /* We don't trust _IO_read_end to represent the current file offset when > > > > > > > + writing in append mode because the value would have to be shifted to > > > > > > > + the end of the file during a flush. Use the write base instead, along > > > > > > > + with the new offset we got above when we did a seek to the end of the > > > > > > > + file. */ > > > > > > > + else if (append_mode) > > > > > > > + offset += fp->_IO_write_ptr - fp->_IO_write_base; > > > > > > > + /* For all other modes, _IO_read_end represents the file offset. */ > > > > > > > else > > > > > > > offset += fp->_IO_write_ptr - fp->_IO_read_end; > > > > > > > } > > > > > > > diff --git a/libio/tst-ftell-append.c b/libio/tst-ftell-append.c > > > > > > > new file mode 100644 > > > > > > > index 0000000..604dc03 > > > > > > > --- /dev/null > > > > > > > +++ b/libio/tst-ftell-append.c > > > > > > > @@ -0,0 +1,169 @@ > > > > > > > +/* Verify that ftell returns the correct value after a read and a write on a > > > > > > > + file opened in a+ mode. > > > > > > > + 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 <locale.h> > > > > > > > +#include <wchar.h> > > > > > > > + > > > > > > > +/* data points to either char_data or wide_data, depending on whether we're > > > > > > > + testing regular file mode or wide mode respectively. Similarly, > > > > > > > + fputs_func points to either fputs or fputws. data_len keeps track of the > > > > > > > + length of the current data and file_len maintains the current file > > > > > > > + length. */ > > > > > > > +#define BUF_LEN 4 > > > > > > > +static void *buf; > > > > > > > +static char char_buf[BUF_LEN]; > > > > > > > +static wchar_t wide_buf[BUF_LEN]; > > > > > > > +static const void *data; > > > > > > > +static const char *char_data = "abcdefghijklmnopqrstuvwxyz"; > > > > > > > +static const wchar_t *wide_data = L"abcdefghijklmnopqrstuvwxyz"; > > > > > > > +static size_t data_len; > > > > > > > +static size_t file_len; > > > > > > > + > > > > > > > +typedef int (*fputs_func_t) (const void *data, FILE *fp); > > > > > > > +fputs_func_t fputs_func; > > > > > > > + > > > > > > > +typedef void *(*fgets_func_t) (void *s, int size, FILE *stream); > > > > > > > +fgets_func_t fgets_func; > > > > > > > + > > > > > > > +static int do_test (void); > > > > > > > + > > > > > > > +#define TEST_FUNCTION do_test () > > > > > > > +#include "../test-skeleton.c" > > > > > > > + > > > > > > > +static FILE * > > > > > > > +init_file (const char *filename) > > > > > > > +{ > > > > > > > + FILE *fp = fopen (filename, "w"); > > > > > > > + if (fp == NULL) > > > > > > > + { > > > > > > > + printf ("fopen: %m\n"); > > > > > > > + return NULL; > > > > > > > + } > > > > > > > + > > > > > > > + int written = fputs_func (data, fp); > > > > > > > + > > > > > > > + if (written == EOF) > > > > > > > + { > > > > > > > + printf ("fputs failed to write data\n"); > > > > > > > + fclose (fp); > > > > > > > + return NULL; > > > > > > > + } > > > > > > > + > > > > > > > + file_len = data_len; > > > > > > > + > > > > > > > + fclose (fp); > > > > > > > + > > > > > > > + fp = fopen (filename, "a+"); > > > > > > > + if (fp == NULL) > > > > > > > + { > > > > > > > + printf ("fopen(a+): %m\n"); > > > > > > > + return NULL; > > > > > > > + } > > > > > > > + > > > > > > > + return fp; > > > > > > > +} > > > > > > > + > > > > > > > +static int > > > > > > > +do_one_test (const char *filename) > > > > > > > +{ > > > > > > > + FILE *fp = init_file (filename); > > > > > > > + > > > > > > > + if (fp == NULL) > > > > > > > + return 1; > > > > > > > + > > > > > > > + void *ret = fgets_func (buf, BUF_LEN, fp); > > > > > > > + > > > > > > > + if (ret == NULL) > > > > > > > + { > > > > > > > + printf ("read failed: %m\n"); > > > > > > > + fclose (fp); > > > > > > > + return 1; > > > > > > > + } > > > > > > > + > > > > > > > + int written = fputs_func (data, fp); > > > > > > > + > > > > > > > + if (written == EOF) > > > > > > > + { > > > > > > > + printf ("fputs failed to write data\n"); > > > > > > > + fclose (fp); > > > > > > > + return 1; > > > > > > > + } > > > > > > > + > > > > > > > + file_len += data_len; > > > > > > > + > > > > > > > + long off = ftell (fp); > > > > > > > + > > > > > > > + if (off != file_len) > > > > > > > + { > > > > > > > + printf ("Incorrect offset %ld, expected %zu\n", off, file_len); > > > > > > > + fclose (fp); > > > > > > > + return 1; > > > > > > > + } > > > > > > > + else > > > > > > > + printf ("Correct offset %ld after write.\n", off); > > > > > > > + > > > > > > > + return 0; > > > > > > > +} > > > > > > > + > > > > > > > +/* Run the tests for regular files and wide mode files. */ > > > > > > > +static int > > > > > > > +do_test (void) > > > > > > > +{ > > > > > > > + int ret = 0; > > > > > > > + char *filename; > > > > > > > + int fd = create_temp_file ("tst-ftell-append-tmp.", &filename); > > > > > > > + > > > > > > > + if (fd == -1) > > > > > > > + { > > > > > > > + printf ("create_temp_file: %m\n"); > > > > > > > + return 1; > > > > > > > + } > > > > > > > + > > > > > > > + close (fd); > > > > > > > + > > > > > > > + /* Tests for regular files. */ > > > > > > > + puts ("Regular mode:"); > > > > > > > + fputs_func = (fputs_func_t) fputs; > > > > > > > + fgets_func = (fgets_func_t) fgets; > > > > > > > + data = char_data; > > > > > > > + buf = char_buf; > > > > > > > + data_len = strlen (char_data); > > > > > > > + ret |= do_one_test (filename); > > > > > > > + > > > > > > > + /* 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; > > > > > > > + fgets_func = (fgets_func_t) fgetws; > > > > > > > + data = wide_data; > > > > > > > + buf = wide_buf; > > > > > > > + data_len = wcslen (wide_data); > > > > > > > + ret |= do_one_test (filename); > > > > > > > + > > > > > > > + return ret; > > > > > > > +} > > > > > > > diff --git a/libio/wfileops.c b/libio/wfileops.c > > > > > > > index 3199861..f123add 100644 > > > > > > > --- a/libio/wfileops.c > > > > > > > +++ b/libio/wfileops.c > > > > > > > @@ -713,9 +713,16 @@ do_ftell_wide (_IO_FILE *fp) > > > > > > > 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; > > > > > > > + /* We don't trust _IO_read_end to represent the current file offset > > > > > > > + when writing in append mode because the value would have to be > > > > > > > + shifted to the end of the file during a flush. Use the write base > > > > > > > + instead, along with the new offset we got above when we did a seek > > > > > > > + to the end of the file. */ > > > > > > > + if (append_mode) > > > > > > > + offset += fp->_IO_write_ptr - fp->_IO_write_base; > > > > > > > + /* For all other modes, _IO_read_end represents the file offset. */ > > > > > > > + else > > > > > > > + offset += fp->_IO_write_ptr - fp->_IO_read_end; > > > > > > > } > > > > > > > } > > > > > > > > > > > > > > -- > > > > > > > 1.8.3.1 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
Ping! On Thu, May 22, 2014 at 11:16:34AM +0530, Siddhesh Poyarekar wrote: > Ping! > > On Thu, May 15, 2014 at 02:09:27PM +0530, Siddhesh Poyarekar wrote: > > Ping! > > > > On Mon, Apr 28, 2014 at 08:20:01PM +0530, Siddhesh Poyarekar wrote: > > > Ping! > > > > > > On Tue, Apr 22, 2014 at 03:17:32PM +0530, Siddhesh Poyarekar wrote: > > > > Ping! > > > > > > > > On Fri, Apr 18, 2014 at 01:02:23PM +0530, Siddhesh Poyarekar wrote: > > > > > Ping! > > > > > > > > > > On Fri, Apr 11, 2014 at 03:11:12PM +0530, Siddhesh Poyarekar wrote: > > > > > > This fell off my ping radar, so... > > > > > > > > > > > > Ping! > > > > > > > > > > > > On Mon, Mar 24, 2014 at 09:21:38PM +0530, Siddhesh Poyarekar wrote: > > > > > > > Ping! > > > > > > > > > > > > > > On Wed, Mar 19, 2014 at 12:37:55PM +0530, Siddhesh Poyarekar wrote: > > > > > > > > The test case had a stupid error that got caught when I tested on > > > > > > > > powerpc. Updated patch. > > > > > > > > > > > > > > > > The offset computation in write mode uses the fact that _IO_read_end > > > > > > > > is kept in sync with the external file offset. This however is not > > > > > > > > true when O_APPEND is in effect since switching to write mode ought to > > > > > > > > send the external file offset to the end of file without making the > > > > > > > > necessary adjustment to _IO_read_end. > > > > > > > > > > > > > > > > Hence in append mode, offset computation when writing should only > > > > > > > > consider the effect of unflushed writes, i.e. from _IO_write_base to > > > > > > > > _IO_write_ptr. > > > > > > > > > > > > > > > > Tested on x86_64 (and not x86_66 like I mentioned earlier ;)), i686, > > > > > > > > s390 and ppc. > > > > > > > > > > > > > > > > Siddhesh > > > > > > > > > > > > > > > > [BZ #16724] > > > > > > > > * libio/tst-ftell-append.c: New test case. > > > > > > > > * libio/Makefile (tests): Add test case. > > > > > > > > * libio/fileops.c (do_ftell): Don't trust _IO_read_end when in > > > > > > > > append mode. > > > > > > > > * libio/wfileops.c (do_ftell_wide): Likewise. > > > > > > > > > > > > > > > > --- > > > > > > > > libio/Makefile | 3 +- > > > > > > > > libio/fileops.c | 12 +++- > > > > > > > > libio/tst-ftell-append.c | 169 +++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > > > libio/wfileops.c | 13 +++- > > > > > > > > 4 files changed, 192 insertions(+), 5 deletions(-) > > > > > > > > create mode 100644 libio/tst-ftell-append.c > > > > > > > > > > > > > > > > diff --git a/libio/Makefile b/libio/Makefile > > > > > > > > index 69c25c0..4bedfad 100644 > > > > > > > > --- a/libio/Makefile > > > > > > > > +++ b/libio/Makefile > > > > > > > > @@ -60,7 +60,8 @@ 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-ftell-active-handler > > > > > > > > + tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \ > > > > > > > > + tst-ftell-append > > > > > > > > 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 cf68dbf..204cfea 100644 > > > > > > > > --- a/libio/fileops.c > > > > > > > > +++ b/libio/fileops.c > > > > > > > > @@ -91,7 +91,9 @@ extern struct __gconv_trans_data __libio_translit attribute_hidden; > > > > > > > > > > > > > > > > The position in the buffer that corresponds to the position > > > > > > > > in external file system is normally _IO_read_end, except in putback > > > > > > > > - mode, when it is _IO_save_end. > > > > > > > > + mode, when it is _IO_save_end and also when the file is in append mode, > > > > > > > > + since switching from read to write mode automatically sends the position in > > > > > > > > + the external file system to the end of file. > > > > > > > > If the field _fb._offset is >= 0, it gives the offset in > > > > > > > > the file as a whole corresponding to eGptr(). (?) > > > > > > > > > > > > > > > > @@ -966,6 +968,14 @@ do_ftell (_IO_FILE *fp) > > > > > > > > /* Adjust for unflushed data. */ > > > > > > > > if (!was_writing) > > > > > > > > offset -= fp->_IO_read_end - fp->_IO_read_ptr; > > > > > > > > + /* We don't trust _IO_read_end to represent the current file offset when > > > > > > > > + writing in append mode because the value would have to be shifted to > > > > > > > > + the end of the file during a flush. Use the write base instead, along > > > > > > > > + with the new offset we got above when we did a seek to the end of the > > > > > > > > + file. */ > > > > > > > > + else if (append_mode) > > > > > > > > + offset += fp->_IO_write_ptr - fp->_IO_write_base; > > > > > > > > + /* For all other modes, _IO_read_end represents the file offset. */ > > > > > > > > else > > > > > > > > offset += fp->_IO_write_ptr - fp->_IO_read_end; > > > > > > > > } > > > > > > > > diff --git a/libio/tst-ftell-append.c b/libio/tst-ftell-append.c > > > > > > > > new file mode 100644 > > > > > > > > index 0000000..604dc03 > > > > > > > > --- /dev/null > > > > > > > > +++ b/libio/tst-ftell-append.c > > > > > > > > @@ -0,0 +1,169 @@ > > > > > > > > +/* Verify that ftell returns the correct value after a read and a write on a > > > > > > > > + file opened in a+ mode. > > > > > > > > + 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 <locale.h> > > > > > > > > +#include <wchar.h> > > > > > > > > + > > > > > > > > +/* data points to either char_data or wide_data, depending on whether we're > > > > > > > > + testing regular file mode or wide mode respectively. Similarly, > > > > > > > > + fputs_func points to either fputs or fputws. data_len keeps track of the > > > > > > > > + length of the current data and file_len maintains the current file > > > > > > > > + length. */ > > > > > > > > +#define BUF_LEN 4 > > > > > > > > +static void *buf; > > > > > > > > +static char char_buf[BUF_LEN]; > > > > > > > > +static wchar_t wide_buf[BUF_LEN]; > > > > > > > > +static const void *data; > > > > > > > > +static const char *char_data = "abcdefghijklmnopqrstuvwxyz"; > > > > > > > > +static const wchar_t *wide_data = L"abcdefghijklmnopqrstuvwxyz"; > > > > > > > > +static size_t data_len; > > > > > > > > +static size_t file_len; > > > > > > > > + > > > > > > > > +typedef int (*fputs_func_t) (const void *data, FILE *fp); > > > > > > > > +fputs_func_t fputs_func; > > > > > > > > + > > > > > > > > +typedef void *(*fgets_func_t) (void *s, int size, FILE *stream); > > > > > > > > +fgets_func_t fgets_func; > > > > > > > > + > > > > > > > > +static int do_test (void); > > > > > > > > + > > > > > > > > +#define TEST_FUNCTION do_test () > > > > > > > > +#include "../test-skeleton.c" > > > > > > > > + > > > > > > > > +static FILE * > > > > > > > > +init_file (const char *filename) > > > > > > > > +{ > > > > > > > > + FILE *fp = fopen (filename, "w"); > > > > > > > > + if (fp == NULL) > > > > > > > > + { > > > > > > > > + printf ("fopen: %m\n"); > > > > > > > > + return NULL; > > > > > > > > + } > > > > > > > > + > > > > > > > > + int written = fputs_func (data, fp); > > > > > > > > + > > > > > > > > + if (written == EOF) > > > > > > > > + { > > > > > > > > + printf ("fputs failed to write data\n"); > > > > > > > > + fclose (fp); > > > > > > > > + return NULL; > > > > > > > > + } > > > > > > > > + > > > > > > > > + file_len = data_len; > > > > > > > > + > > > > > > > > + fclose (fp); > > > > > > > > + > > > > > > > > + fp = fopen (filename, "a+"); > > > > > > > > + if (fp == NULL) > > > > > > > > + { > > > > > > > > + printf ("fopen(a+): %m\n"); > > > > > > > > + return NULL; > > > > > > > > + } > > > > > > > > + > > > > > > > > + return fp; > > > > > > > > +} > > > > > > > > + > > > > > > > > +static int > > > > > > > > +do_one_test (const char *filename) > > > > > > > > +{ > > > > > > > > + FILE *fp = init_file (filename); > > > > > > > > + > > > > > > > > + if (fp == NULL) > > > > > > > > + return 1; > > > > > > > > + > > > > > > > > + void *ret = fgets_func (buf, BUF_LEN, fp); > > > > > > > > + > > > > > > > > + if (ret == NULL) > > > > > > > > + { > > > > > > > > + printf ("read failed: %m\n"); > > > > > > > > + fclose (fp); > > > > > > > > + return 1; > > > > > > > > + } > > > > > > > > + > > > > > > > > + int written = fputs_func (data, fp); > > > > > > > > + > > > > > > > > + if (written == EOF) > > > > > > > > + { > > > > > > > > + printf ("fputs failed to write data\n"); > > > > > > > > + fclose (fp); > > > > > > > > + return 1; > > > > > > > > + } > > > > > > > > + > > > > > > > > + file_len += data_len; > > > > > > > > + > > > > > > > > + long off = ftell (fp); > > > > > > > > + > > > > > > > > + if (off != file_len) > > > > > > > > + { > > > > > > > > + printf ("Incorrect offset %ld, expected %zu\n", off, file_len); > > > > > > > > + fclose (fp); > > > > > > > > + return 1; > > > > > > > > + } > > > > > > > > + else > > > > > > > > + printf ("Correct offset %ld after write.\n", off); > > > > > > > > + > > > > > > > > + return 0; > > > > > > > > +} > > > > > > > > + > > > > > > > > +/* Run the tests for regular files and wide mode files. */ > > > > > > > > +static int > > > > > > > > +do_test (void) > > > > > > > > +{ > > > > > > > > + int ret = 0; > > > > > > > > + char *filename; > > > > > > > > + int fd = create_temp_file ("tst-ftell-append-tmp.", &filename); > > > > > > > > + > > > > > > > > + if (fd == -1) > > > > > > > > + { > > > > > > > > + printf ("create_temp_file: %m\n"); > > > > > > > > + return 1; > > > > > > > > + } > > > > > > > > + > > > > > > > > + close (fd); > > > > > > > > + > > > > > > > > + /* Tests for regular files. */ > > > > > > > > + puts ("Regular mode:"); > > > > > > > > + fputs_func = (fputs_func_t) fputs; > > > > > > > > + fgets_func = (fgets_func_t) fgets; > > > > > > > > + data = char_data; > > > > > > > > + buf = char_buf; > > > > > > > > + data_len = strlen (char_data); > > > > > > > > + ret |= do_one_test (filename); > > > > > > > > + > > > > > > > > + /* 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; > > > > > > > > + fgets_func = (fgets_func_t) fgetws; > > > > > > > > + data = wide_data; > > > > > > > > + buf = wide_buf; > > > > > > > > + data_len = wcslen (wide_data); > > > > > > > > + ret |= do_one_test (filename); > > > > > > > > + > > > > > > > > + return ret; > > > > > > > > +} > > > > > > > > diff --git a/libio/wfileops.c b/libio/wfileops.c > > > > > > > > index 3199861..f123add 100644 > > > > > > > > --- a/libio/wfileops.c > > > > > > > > +++ b/libio/wfileops.c > > > > > > > > @@ -713,9 +713,16 @@ do_ftell_wide (_IO_FILE *fp) > > > > > > > > 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; > > > > > > > > + /* We don't trust _IO_read_end to represent the current file offset > > > > > > > > + when writing in append mode because the value would have to be > > > > > > > > + shifted to the end of the file during a flush. Use the write base > > > > > > > > + instead, along with the new offset we got above when we did a seek > > > > > > > > + to the end of the file. */ > > > > > > > > + if (append_mode) > > > > > > > > + offset += fp->_IO_write_ptr - fp->_IO_write_base; > > > > > > > > + /* For all other modes, _IO_read_end represents the file offset. */ > > > > > > > > + else > > > > > > > > + offset += fp->_IO_write_ptr - fp->_IO_read_end; > > > > > > > > } > > > > > > > > } > > > > > > > > > > > > > > > > -- > > > > > > > > 1.8.3.1 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
Hi Siddhesh, The patch look good, I would also add a point to https://sourceware.org/glibc/wiki/File%20offsets%20in%20a%20stdio%20stream%20and%20ftell in commit message. On 26-05-2014 03:08, Siddhesh Poyarekar wrote: > Ping! > > On Thu, May 22, 2014 at 11:16:34AM +0530, Siddhesh Poyarekar wrote: >> Ping! >> >> On Thu, May 15, 2014 at 02:09:27PM +0530, Siddhesh Poyarekar wrote: >>> Ping! >>> >>> On Mon, Apr 28, 2014 at 08:20:01PM +0530, Siddhesh Poyarekar wrote: >>>> Ping! >>>> >>>> On Tue, Apr 22, 2014 at 03:17:32PM +0530, Siddhesh Poyarekar wrote: >>>>> Ping! >>>>> >>>>> On Fri, Apr 18, 2014 at 01:02:23PM +0530, Siddhesh Poyarekar wrote: >>>>>> Ping! >>>>>> >>>>>> On Fri, Apr 11, 2014 at 03:11:12PM +0530, Siddhesh Poyarekar wrote: >>>>>>> This fell off my ping radar, so... >>>>>>> >>>>>>> Ping! >>>>>>> >>>>>>> On Mon, Mar 24, 2014 at 09:21:38PM +0530, Siddhesh Poyarekar wrote: >>>>>>>> Ping! >>>>>>>> >>>>>>>> On Wed, Mar 19, 2014 at 12:37:55PM +0530, Siddhesh Poyarekar wrote: >>>>>>>>> The test case had a stupid error that got caught when I tested on >>>>>>>>> powerpc. Updated patch. >>>>>>>>> >>>>>>>>> The offset computation in write mode uses the fact that _IO_read_end >>>>>>>>> is kept in sync with the external file offset. This however is not >>>>>>>>> true when O_APPEND is in effect since switching to write mode ought to >>>>>>>>> send the external file offset to the end of file without making the >>>>>>>>> necessary adjustment to _IO_read_end. >>>>>>>>> >>>>>>>>> Hence in append mode, offset computation when writing should only >>>>>>>>> consider the effect of unflushed writes, i.e. from _IO_write_base to >>>>>>>>> _IO_write_ptr. >>>>>>>>> >>>>>>>>> Tested on x86_64 (and not x86_66 like I mentioned earlier ;)), i686, >>>>>>>>> s390 and ppc. >>>>>>>>> >>>>>>>>> Siddhesh >>>>>>>>> >>>>>>>>> [BZ #16724] >>>>>>>>> * libio/tst-ftell-append.c: New test case. >>>>>>>>> * libio/Makefile (tests): Add test case. >>>>>>>>> * libio/fileops.c (do_ftell): Don't trust _IO_read_end when in >>>>>>>>> append mode. >>>>>>>>> * libio/wfileops.c (do_ftell_wide): Likewise. >>>>>>>>> >>>>>>>>> --- >>>>>>>>> libio/Makefile | 3 +- >>>>>>>>> libio/fileops.c | 12 +++- >>>>>>>>> libio/tst-ftell-append.c | 169 +++++++++++++++++++++++++++++++++++++++++++++++ >>>>>>>>> libio/wfileops.c | 13 +++- >>>>>>>>> 4 files changed, 192 insertions(+), 5 deletions(-) >>>>>>>>> create mode 100644 libio/tst-ftell-append.c >>>>>>>>> >>>>>>>>> diff --git a/libio/Makefile b/libio/Makefile >>>>>>>>> index 69c25c0..4bedfad 100644 >>>>>>>>> --- a/libio/Makefile >>>>>>>>> +++ b/libio/Makefile >>>>>>>>> @@ -60,7 +60,8 @@ 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-ftell-active-handler >>>>>>>>> + tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \ >>>>>>>>> + tst-ftell-append >>>>>>>>> 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 cf68dbf..204cfea 100644 >>>>>>>>> --- a/libio/fileops.c >>>>>>>>> +++ b/libio/fileops.c >>>>>>>>> @@ -91,7 +91,9 @@ extern struct __gconv_trans_data __libio_translit attribute_hidden; >>>>>>>>> >>>>>>>>> The position in the buffer that corresponds to the position >>>>>>>>> in external file system is normally _IO_read_end, except in putback >>>>>>>>> - mode, when it is _IO_save_end. >>>>>>>>> + mode, when it is _IO_save_end and also when the file is in append mode, >>>>>>>>> + since switching from read to write mode automatically sends the position in >>>>>>>>> + the external file system to the end of file. >>>>>>>>> If the field _fb._offset is >= 0, it gives the offset in >>>>>>>>> the file as a whole corresponding to eGptr(). (?) >>>>>>>>> >>>>>>>>> @@ -966,6 +968,14 @@ do_ftell (_IO_FILE *fp) >>>>>>>>> /* Adjust for unflushed data. */ >>>>>>>>> if (!was_writing) >>>>>>>>> offset -= fp->_IO_read_end - fp->_IO_read_ptr; >>>>>>>>> + /* We don't trust _IO_read_end to represent the current file offset when >>>>>>>>> + writing in append mode because the value would have to be shifted to >>>>>>>>> + the end of the file during a flush. Use the write base instead, along >>>>>>>>> + with the new offset we got above when we did a seek to the end of the >>>>>>>>> + file. */ >>>>>>>>> + else if (append_mode) >>>>>>>>> + offset += fp->_IO_write_ptr - fp->_IO_write_base; >>>>>>>>> + /* For all other modes, _IO_read_end represents the file offset. */ >>>>>>>>> else >>>>>>>>> offset += fp->_IO_write_ptr - fp->_IO_read_end; >>>>>>>>> } >>>>>>>>> diff --git a/libio/tst-ftell-append.c b/libio/tst-ftell-append.c >>>>>>>>> new file mode 100644 >>>>>>>>> index 0000000..604dc03 >>>>>>>>> --- /dev/null >>>>>>>>> +++ b/libio/tst-ftell-append.c >>>>>>>>> @@ -0,0 +1,169 @@ >>>>>>>>> +/* Verify that ftell returns the correct value after a read and a write on a >>>>>>>>> + file opened in a+ mode. >>>>>>>>> + 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 <locale.h> >>>>>>>>> +#include <wchar.h> >>>>>>>>> + >>>>>>>>> +/* data points to either char_data or wide_data, depending on whether we're >>>>>>>>> + testing regular file mode or wide mode respectively. Similarly, >>>>>>>>> + fputs_func points to either fputs or fputws. data_len keeps track of the >>>>>>>>> + length of the current data and file_len maintains the current file >>>>>>>>> + length. */ >>>>>>>>> +#define BUF_LEN 4 >>>>>>>>> +static void *buf; >>>>>>>>> +static char char_buf[BUF_LEN]; >>>>>>>>> +static wchar_t wide_buf[BUF_LEN]; >>>>>>>>> +static const void *data; >>>>>>>>> +static const char *char_data = "abcdefghijklmnopqrstuvwxyz"; >>>>>>>>> +static const wchar_t *wide_data = L"abcdefghijklmnopqrstuvwxyz"; >>>>>>>>> +static size_t data_len; >>>>>>>>> +static size_t file_len; >>>>>>>>> + >>>>>>>>> +typedef int (*fputs_func_t) (const void *data, FILE *fp); >>>>>>>>> +fputs_func_t fputs_func; >>>>>>>>> + >>>>>>>>> +typedef void *(*fgets_func_t) (void *s, int size, FILE *stream); >>>>>>>>> +fgets_func_t fgets_func; >>>>>>>>> + >>>>>>>>> +static int do_test (void); >>>>>>>>> + >>>>>>>>> +#define TEST_FUNCTION do_test () >>>>>>>>> +#include "../test-skeleton.c" >>>>>>>>> + >>>>>>>>> +static FILE * >>>>>>>>> +init_file (const char *filename) >>>>>>>>> +{ >>>>>>>>> + FILE *fp = fopen (filename, "w"); >>>>>>>>> + if (fp == NULL) >>>>>>>>> + { >>>>>>>>> + printf ("fopen: %m\n"); >>>>>>>>> + return NULL; >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> + int written = fputs_func (data, fp); >>>>>>>>> + >>>>>>>>> + if (written == EOF) >>>>>>>>> + { >>>>>>>>> + printf ("fputs failed to write data\n"); >>>>>>>>> + fclose (fp); >>>>>>>>> + return NULL; >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> + file_len = data_len; >>>>>>>>> + >>>>>>>>> + fclose (fp); >>>>>>>>> + >>>>>>>>> + fp = fopen (filename, "a+"); >>>>>>>>> + if (fp == NULL) >>>>>>>>> + { >>>>>>>>> + printf ("fopen(a+): %m\n"); >>>>>>>>> + return NULL; >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> + return fp; >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> +static int >>>>>>>>> +do_one_test (const char *filename) >>>>>>>>> +{ >>>>>>>>> + FILE *fp = init_file (filename); >>>>>>>>> + >>>>>>>>> + if (fp == NULL) >>>>>>>>> + return 1; >>>>>>>>> + >>>>>>>>> + void *ret = fgets_func (buf, BUF_LEN, fp); >>>>>>>>> + >>>>>>>>> + if (ret == NULL) >>>>>>>>> + { >>>>>>>>> + printf ("read failed: %m\n"); >>>>>>>>> + fclose (fp); >>>>>>>>> + return 1; >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> + int written = fputs_func (data, fp); >>>>>>>>> + >>>>>>>>> + if (written == EOF) >>>>>>>>> + { >>>>>>>>> + printf ("fputs failed to write data\n"); >>>>>>>>> + fclose (fp); >>>>>>>>> + return 1; >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> + file_len += data_len; >>>>>>>>> + >>>>>>>>> + long off = ftell (fp); >>>>>>>>> + >>>>>>>>> + if (off != file_len) >>>>>>>>> + { >>>>>>>>> + printf ("Incorrect offset %ld, expected %zu\n", off, file_len); >>>>>>>>> + fclose (fp); >>>>>>>>> + return 1; >>>>>>>>> + } >>>>>>>>> + else >>>>>>>>> + printf ("Correct offset %ld after write.\n", off); >>>>>>>>> + >>>>>>>>> + return 0; >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> +/* Run the tests for regular files and wide mode files. */ >>>>>>>>> +static int >>>>>>>>> +do_test (void) >>>>>>>>> +{ >>>>>>>>> + int ret = 0; >>>>>>>>> + char *filename; >>>>>>>>> + int fd = create_temp_file ("tst-ftell-append-tmp.", &filename); >>>>>>>>> + >>>>>>>>> + if (fd == -1) >>>>>>>>> + { >>>>>>>>> + printf ("create_temp_file: %m\n"); >>>>>>>>> + return 1; >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> + close (fd); >>>>>>>>> + >>>>>>>>> + /* Tests for regular files. */ >>>>>>>>> + puts ("Regular mode:"); >>>>>>>>> + fputs_func = (fputs_func_t) fputs; >>>>>>>>> + fgets_func = (fgets_func_t) fgets; >>>>>>>>> + data = char_data; >>>>>>>>> + buf = char_buf; >>>>>>>>> + data_len = strlen (char_data); >>>>>>>>> + ret |= do_one_test (filename); >>>>>>>>> + >>>>>>>>> + /* 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; >>>>>>>>> + fgets_func = (fgets_func_t) fgetws; >>>>>>>>> + data = wide_data; >>>>>>>>> + buf = wide_buf; >>>>>>>>> + data_len = wcslen (wide_data); >>>>>>>>> + ret |= do_one_test (filename); >>>>>>>>> + >>>>>>>>> + return ret; >>>>>>>>> +} >>>>>>>>> diff --git a/libio/wfileops.c b/libio/wfileops.c >>>>>>>>> index 3199861..f123add 100644 >>>>>>>>> --- a/libio/wfileops.c >>>>>>>>> +++ b/libio/wfileops.c >>>>>>>>> @@ -713,9 +713,16 @@ do_ftell_wide (_IO_FILE *fp) >>>>>>>>> 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; >>>>>>>>> + /* We don't trust _IO_read_end to represent the current file offset >>>>>>>>> + when writing in append mode because the value would have to be >>>>>>>>> + shifted to the end of the file during a flush. Use the write base >>>>>>>>> + instead, along with the new offset we got above when we did a seek >>>>>>>>> + to the end of the file. */ >>>>>>>>> + if (append_mode) >>>>>>>>> + offset += fp->_IO_write_ptr - fp->_IO_write_base; >>>>>>>>> + /* For all other modes, _IO_read_end represents the file offset. */ >>>>>>>>> + else >>>>>>>>> + offset += fp->_IO_write_ptr - fp->_IO_read_end; >>>>>>>>> } >>>>>>>>> } >>>>>>>>> >>>>>>>>> -- >>>>>>>>> 1.8.3.1 >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
On Mon, May 26, 2014 at 05:57:13PM -0300, Adhemerval Zanella wrote: > Hi Siddhesh, > > The patch look good, I would also add a point to > https://sourceware.org/glibc/wiki/File%20offsets%20in%20a%20stdio%20stream%20and%20ftell > in commit message. Done, Thanks. Siddhesh
diff --git a/libio/Makefile b/libio/Makefile index 69c25c0..4bedfad 100644 --- a/libio/Makefile +++ b/libio/Makefile @@ -60,7 +60,8 @@ 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-ftell-active-handler + tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \ + tst-ftell-append 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 cf68dbf..204cfea 100644 --- a/libio/fileops.c +++ b/libio/fileops.c @@ -91,7 +91,9 @@ extern struct __gconv_trans_data __libio_translit attribute_hidden; The position in the buffer that corresponds to the position in external file system is normally _IO_read_end, except in putback - mode, when it is _IO_save_end. + mode, when it is _IO_save_end and also when the file is in append mode, + since switching from read to write mode automatically sends the position in + the external file system to the end of file. If the field _fb._offset is >= 0, it gives the offset in the file as a whole corresponding to eGptr(). (?) @@ -966,6 +968,14 @@ do_ftell (_IO_FILE *fp) /* Adjust for unflushed data. */ if (!was_writing) offset -= fp->_IO_read_end - fp->_IO_read_ptr; + /* We don't trust _IO_read_end to represent the current file offset when + writing in append mode because the value would have to be shifted to + the end of the file during a flush. Use the write base instead, along + with the new offset we got above when we did a seek to the end of the + file. */ + else if (append_mode) + offset += fp->_IO_write_ptr - fp->_IO_write_base; + /* For all other modes, _IO_read_end represents the file offset. */ else offset += fp->_IO_write_ptr - fp->_IO_read_end; } diff --git a/libio/tst-ftell-append.c b/libio/tst-ftell-append.c new file mode 100644 index 0000000..604dc03 --- /dev/null +++ b/libio/tst-ftell-append.c @@ -0,0 +1,169 @@ +/* Verify that ftell returns the correct value after a read and a write on a + file opened in a+ mode. + 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 <locale.h> +#include <wchar.h> + +/* data points to either char_data or wide_data, depending on whether we're + testing regular file mode or wide mode respectively. Similarly, + fputs_func points to either fputs or fputws. data_len keeps track of the + length of the current data and file_len maintains the current file + length. */ +#define BUF_LEN 4 +static void *buf; +static char char_buf[BUF_LEN]; +static wchar_t wide_buf[BUF_LEN]; +static const void *data; +static const char *char_data = "abcdefghijklmnopqrstuvwxyz"; +static const wchar_t *wide_data = L"abcdefghijklmnopqrstuvwxyz"; +static size_t data_len; +static size_t file_len; + +typedef int (*fputs_func_t) (const void *data, FILE *fp); +fputs_func_t fputs_func; + +typedef void *(*fgets_func_t) (void *s, int size, FILE *stream); +fgets_func_t fgets_func; + +static int do_test (void); + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" + +static FILE * +init_file (const char *filename) +{ + FILE *fp = fopen (filename, "w"); + if (fp == NULL) + { + printf ("fopen: %m\n"); + return NULL; + } + + int written = fputs_func (data, fp); + + if (written == EOF) + { + printf ("fputs failed to write data\n"); + fclose (fp); + return NULL; + } + + file_len = data_len; + + fclose (fp); + + fp = fopen (filename, "a+"); + if (fp == NULL) + { + printf ("fopen(a+): %m\n"); + return NULL; + } + + return fp; +} + +static int +do_one_test (const char *filename) +{ + FILE *fp = init_file (filename); + + if (fp == NULL) + return 1; + + void *ret = fgets_func (buf, BUF_LEN, fp); + + if (ret == NULL) + { + printf ("read failed: %m\n"); + fclose (fp); + return 1; + } + + int written = fputs_func (data, fp); + + if (written == EOF) + { + printf ("fputs failed to write data\n"); + fclose (fp); + return 1; + } + + file_len += data_len; + + long off = ftell (fp); + + if (off != file_len) + { + printf ("Incorrect offset %ld, expected %zu\n", off, file_len); + fclose (fp); + return 1; + } + else + printf ("Correct offset %ld after write.\n", off); + + return 0; +} + +/* Run the tests for regular files and wide mode files. */ +static int +do_test (void) +{ + int ret = 0; + char *filename; + int fd = create_temp_file ("tst-ftell-append-tmp.", &filename); + + if (fd == -1) + { + printf ("create_temp_file: %m\n"); + return 1; + } + + close (fd); + + /* Tests for regular files. */ + puts ("Regular mode:"); + fputs_func = (fputs_func_t) fputs; + fgets_func = (fgets_func_t) fgets; + data = char_data; + buf = char_buf; + data_len = strlen (char_data); + ret |= do_one_test (filename); + + /* 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; + fgets_func = (fgets_func_t) fgetws; + data = wide_data; + buf = wide_buf; + data_len = wcslen (wide_data); + ret |= do_one_test (filename); + + return ret; +} diff --git a/libio/wfileops.c b/libio/wfileops.c index 3199861..f123add 100644 --- a/libio/wfileops.c +++ b/libio/wfileops.c @@ -713,9 +713,16 @@ do_ftell_wide (_IO_FILE *fp) 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; + /* We don't trust _IO_read_end to represent the current file offset + when writing in append mode because the value would have to be + shifted to the end of the file during a flush. Use the write base + instead, along with the new offset we got above when we did a seek + to the end of the file. */ + if (append_mode) + offset += fp->_IO_write_ptr - fp->_IO_write_base; + /* For all other modes, _IO_read_end represents the file offset. */ + else + offset += fp->_IO_write_ptr - fp->_IO_read_end; } }