[PATCH 1/2] libio: Start to return errors when flushing fwrite's buffer [BZ #29459]

[Tulio Magno Quites Machado Filho]

DJ Delorie <dj@redhat.com> writes:

> Tulio Magno Quites Machado Filho <tuliom@ascii.art.br> writes:
>> +  __uint64_t _total_written;
>
> What initializes this?  Is it a per-fwrite count, or a per-stream count?

Oops. Fixed.

>> diff --git a/libio/iofwrite.c b/libio/iofwrite.c
>> index af2e2070aff..7aaa9e43c1f 100644
>> --- a/libio/iofwrite.c
>> +++ b/libio/iofwrite.c
>> @@ -36,13 +36,31 @@ _IO_fwrite (const void *buf, size_t size, size_t count, FILE *fp)
>>      return 0;
>>    _IO_acquire_lock (fp);
>>    if (_IO_vtable_offset (fp) != 0 || _IO_fwide (fp, -1) == -1)
>> -    written = _IO_sputn (fp, (const char *) buf, request);
>> +    {
>> +      /* Compute actually written bytes plus pending buffer
>> +         contents.  */
>> +      uint64_t original_total_written
>> +        = fp->_total_written + (fp->_IO_write_ptr - fp->_IO_write_base);
>> +      written = _IO_sputn (fp, (const char *) buf, request);
>> +      if (written == EOF)
>> +        {
>> +          if (fp->_total_written > original_total_written)
>> +            {
>> +              written = fp->_total_written - original_total_written;
>
> I don't understand why you're including the
> buffered-but-unwritten-to-disk count in the first case, but not in the
> second case.  Are you assuming the buffered data is written out in
> addition to the data passed?  If so, a comment to that effect might be
> useful.

No, we're actually trying to discover which kind of data got written,
e.g. if it was all data that had been buffered previously, which means
this fwrite wrote 0 bytes from this request.
I added a comment elaborating on this.

>> +   Copyright (C) 2024 Free Software Foundation, Inc.
>
> Needs 2025 now :-)

Fixed.

>> +# Copyright (C) 2024 Free Software Foundation, Inc.
>
> 2025 here too.

Fixed.

>> +   Copyright (C) 2024 Free Software Foundation, Inc.
>
> Here too.

Fixed.

>> +/* When the underlying write () fails with EPIPE, fwrite () is expected to
>> +   return an error by returning < size*nmemb and keeping errno=EPIPE.  */
>
> fwrite returns less than nmemb, not less than size*nmemb.

Fixed.

>> +      /* Read at least the first line from the pipe before closing it.
>> +	 This is important because it guarantees the file stream will have
>> +	 flag _IO_CURRENTLY_PUTTING set, which triggers important parts of
>> +	 the code that flushes lines from fwrite's internal buffer.  */
>> +      do {
>> +	bytes = read (fd[0], b, BUFFERSIZE);
>> +      } while(memrchr (b, '\n', bytes) == NULL);
>
> This needs a "bytes > 0 &&" in the while, in case of read error.

Done.

>> +      /* Ensure errno is not set before starting.  */
>> +      TEST_VERIFY_EXIT (errno == 0);
>
> Or you could have set errno to zero, which is what the standards
> typically suggest.

Changed.

I'm sending a newer version (v4) of this patch.

Thanks!

-- 
Tulio Magno