This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PING^8][PATCH v1.1] Fix offset computation for append+ mode on switching from read (BZ #16724)
- From: Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>
- To: libc-alpha at sourceware dot org
- Date: Mon, 26 May 2014 17:57:13 -0300
- Subject: Re: [PING^8][PATCH v1.1] Fix offset computation for append+ mode on switching from read (BZ #16724)
- Authentication-results: sourceware.org; auth=none
- References: <20140319070755 dot GA25315 at spoyarek dot pnq dot redhat dot com> <20140324155138 dot GY1850 at spoyarek dot pnq dot redhat dot com> <20140411094112 dot GD25518 at spoyarek dot pnq dot redhat dot com> <20140418073223 dot GU11072 at spoyarek dot pnq dot redhat dot com> <20140422094731 dot GF1868 at spoyarek dot pnq dot redhat dot com> <20140428145001 dot GO10922 at spoyarek dot pnq dot redhat dot com> <20140515083927 dot GE32387 at spoyarek dot pnq dot redhat dot com> <20140522054634 dot GS14500 at spoyarek dot pnq dot redhat dot com> <20140526060849 dot GN12497 at spoyarek dot pnq dot redhat dot com>
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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>