Re: Ping: [PATCH] grep testsuite: test-fwrite fails with git glibc

["Carlos O'Donell" <carlos at systemhalted dot org>]

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.