[PATCH] libio: Start to return errors when flushing fwrite's buffer

[Tulio Magno Quites Machado Filho]

I failed to make it explicit in the commit message that this patch is
fixing bug 29459.

Florian Weimer <fweimer@redhat.com> writes:

> * Tulio Magno Quites Machado Filho:
>
>> diff --git a/libio/iofwrite.c b/libio/iofwrite.c
>> index af2e2070aff..8c0323947ae 100644
>> --- a/libio/iofwrite.c
>> +++ b/libio/iofwrite.c
>> @@ -38,12 +38,25 @@ _IO_fwrite (const void *buf, size_t size, size_t count, FILE *fp)
>>    if (_IO_vtable_offset (fp) != 0 || _IO_fwide (fp, -1) == -1)
>>      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)
>> +  if (written == request)
>> +    /* We have written all of the input successfully.  */
>>      return count;
>> +  else if (written == EOF)
>> +    {
>> +      /* sputn() has the same semantics as fputs(), returning EOF on error.
>> +	 It also means we did not manage to flush the buffer, but the data is
>> +	 in the buffer and therefore written, which is a conflicting
>> +	 scenario.
>> +
>> +	 Confirm that an irrecoverable error happened and return an error,
>> +	 i.e. return less than count.
>> +	 Otherwise, return success (aka. count) and let the caller try
>> +	 again, which is the behavior that fwrite had for years.  */
>> +      if ((fp->_flags & _IO_ERR_SEEN) && errno != 0 && errno != EAGAIN)
>> +	return 0;
>> +      else
>> +	return count;
>
> Is it really appropriate to handle EAGAIN differently here?  I don't see
> anything about that in POSIX.  In fact, some EAGAIN errors may not
> actually be recoverable as required by POSIX.

IMHO, this is the most important question that I'd like to answer with
this fix.
I'm happy to treat all cases where sputn() returns EOF as errors.
But I realized that we might be able to keep the old behavior
(i.e. returning success) if a recoverable error is found.

I do agree that treating EAGAIN as "recoverable error" is not optimal.

With that said, I return this question to you and the community:
should we treat recoverable errors differently?

> The idea is that it's possible for an application to clear the error
> indicator, try again, and preserve item boundaries (if the item size is
> not greater than the buffer size).  This is all a bit theoretical at
> this point because I don't think the way libio is layered, it can
> actually handle things in this way.  But we should be able to something
> more useful than just returning 0 here.

I don't think we can do this now, but I agree that we should be able to
do better.  While I do agree with some of your suggestions here, I
believe they're orthogonal to this patch, which is intended to fix bug
29459.  I'd appreciate if the fix could be merged without requiring to
rewrite libio.

> I think we should have a FUSE-based test, so that we can also check that
> the file-position indicator is updated as required by the specification.
> This requires a seekable file descriptor, which rules out using a pipe.

I didn't understand your suggestion. Are you suggesting to replace
tst-fwrite-bz29459.c with a FUSE-based one?
Keep in mind this is the original reproducer from bug 29459.

>> +  for (i = 1; i <= ITERATIONS; i++)
>> +    {
>> +      /* Keep writing to stdout.  Success means that fwrite () returns an
>> +	 error.  */
>
> “Success” is ambiguous here.  I think it's the test objective to trigger
> an fwrite failure?

I tried to clarify it with that source code comment.
Would it help if I replaced the current comment with the following?
"The test succeeds if fwrite () returns an error."

-- 
Tulio Magno