This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: Ping: [PATCH] grep testsuite: test-fwrite fails with git glibc
On Tue, Nov 27, 2012 at 2:22 AM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote:
> Pinging this patch sooner than usual since I believe Dave would like
> to freeze the tree on 28th, which is tomorrow (which comes in a bit
> earlier where I live ;)).
>
> Siddhesh
>
>
> On Sun, Nov 25, 2012 at 04:31:20PM +0530, Siddhesh Poyarekar wrote:
>> On Sun, Nov 25, 2012 at 10:20:20AM +0100, Andreas Jaeger wrote:
>> > So, this patch fixes the grep testsuite? Great! Could you add a
>> > testcase for glibc so that we don't regress again?
>> >
>>
>> Here's an updated patch with a test case adapted from the grep
>> testsuite. I did not run the grep testsuite directly, but I compiled
>> and executed all programs in gnulib-tests to ensure that there isn't
>> another failure.
>>
>> Siddhesh
>>
>> ChangeLog:
>>
>> * libio/Makefile (tests): Add test case tst-fwrite-error.
>> * libio/iofwrite.c (_IO_fwrite): Return 0 on EOF.
>> * libio/iofwrite_u.c (fwrite_unlocked): Likewise.
>> * libio/tst-fwrite-error.c: New test case.
>
>> diff --git a/libio/Makefile b/libio/Makefile
>> index 9ccd6a0..83d90d0 100644
>> --- a/libio/Makefile
>> +++ b/libio/Makefile
>> @@ -59,7 +59,8 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc \
>> tst-memstream1 tst-memstream2 \
>> tst-wmemstream1 tst-wmemstream2 \
>> bug-memstream1 bug-wmemstream1 \
>> - tst-setvbuf1 tst-popen1 tst-fgetwc bug-wsetpos bug-fclose1 tst-fseek
>> + tst-setvbuf1 tst-popen1 tst-fgetwc bug-wsetpos bug-fclose1 tst-fseek \
>> + tst-fwrite-error
>> ifeq (yes,$(build-shared))
>> # Add test-fopenloc only if shared library is enabled since it depends on
>> # shared localedata objects.
>> diff --git a/libio/iofwrite.c b/libio/iofwrite.c
>> index d4610f7..e93a656 100644
>> --- a/libio/iofwrite.c
>> +++ b/libio/iofwrite.c
>> @@ -43,11 +43,19 @@ _IO_fwrite (buf, size, count, fp)
>> written = _IO_sputn (fp, (const char *) buf, request);
>> _IO_release_lock (fp);
>> /* We have written all of the input in case the return value indicates
>> - this or EOF is returned. The latter is a special case where we
>> - simply did not manage to flush the buffer. But the data is in the
>> - buffer and therefore written as far as fwrite is concerned. */
>> - if (written == request || written == EOF)
>> + this. */
>> + if (written == request)
>> return count;
>> + /* It is possible that the data was written out into buffer and we just
>> + failed to flush it out. However, this is not necessarily always the
>> + case and we cannot really differentiate this with a case when a flush
>> + failed and all of the data was not in the buffer. Hence, just return 0
>> + (the flush failure should already have set the errno) and let the user
>> + decide what to do. A future enhancement could be to find out how much
>> + data is in the buffer and return that as a short write instead of just
>> + 0. */
Returning 0 here violates POSIX which says "shall return the number of
elements successfully written, which may be less than nitems if a
write error is encountered."
What's your estimate on how hard is it to actually fix this correctly
and return the number of bytes written?
I agree that previously the code was really wrong.
Cheers,
Carlos.