diff mbox

[v1.1] Fix offset computation for append+ mode on switching from read (BZ #16724)

Message ID 20140319070755.GA25315@spoyarek.pnq.redhat.com
State Committed
Headers

Commit Message

Siddhesh Poyarekar March 19, 2014, 7:07 a.m. UTC 
  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

Comments

Siddhesh Poyarekar March 24, 2014, 3:51 p.m. UTC | #1 
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
>
Siddhesh Poyarekar April 11, 2014, 9:41 a.m. UTC | #2 
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
> > 
> 
>
Siddhesh Poyarekar April 18, 2014, 7:32 a.m. UTC | #3 
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
> > > 
> > 
> > 
> 
>
Siddhesh Poyarekar April 22, 2014, 9:47 a.m. UTC | #4 
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
> > > > 
> > > 
> > > 
> > 
> > 
> 
>
Siddhesh Poyarekar April 28, 2014, 2:50 p.m. UTC | #5 
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
> > > > > 
> > > > 
> > > > 
> > > 
> > > 
> > 
> > 
> 
>
Siddhesh Poyarekar May 15, 2014, 8:39 a.m. UTC | #6 
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
> > > > > > 
> > > > > 
> > > > > 
> > > > 
> > > > 
> > > 
> > > 
> > 
> > 
> 
>
Siddhesh Poyarekar May 22, 2014, 5:46 a.m. UTC | #7 
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
> > > > > > > 
> > > > > > 
> > > > > > 
> > > > > 
> > > > > 
> > > > 
> > > > 
> > > 
> > > 
> > 
> > 
> 
>
Siddhesh Poyarekar May 26, 2014, 6:08 a.m. UTC | #8 
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
> > > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > 
> > > > > > 
> > > > > 
> > > > > 
> > > > 
> > > > 
> > > 
> > > 
> > 
> > 
> 
>
Adhemerval Zanella Netto May 26, 2014, 8:57 p.m. UTC | #9 
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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
Siddhesh Poyarekar May 27, 2014, 8:26 a.m. UTC | #10 
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 mbox

Patch

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;
 	}
     }