Re: Ping[3]: [PATCH][BZ #11741] printf should return negative valueon I/O error

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

On Thu, Nov 15, 2012 at 11:27 PM, Siddhesh Poyarekar
<siddhesh@redhat.com> wrote:
> On Thu, 15 Nov 2012 23:00:48 -0500, Carlos wrote:
>> Your original patch still suffers the same problem, you make
>> a change to libio/iowpadn.c that isn't needed?
>>
>> diff --git a/libio/iowpadn.c b/libio/iowpadn.c
>> index 05632d5..334e01a 100644
>> --- a/libio/iowpadn.c
>> +++ b/libio/iowpadn.c
>> @@ -65,7 +65,7 @@ _IO_wpadn (fp, pad, count)
>>        w = _IO_sputn (fp, (char *) padptr, PADSIZE);
>>        written += w;
>>        if (w != PADSIZE)
>> -     return written;
>> +     return w <= 0 ? w : written;
>>      }
>>
>>    if (i > 0)
>> ---
>>
>> Can you confirm you need this change?
>>
>
> Ah, sorry, I have finally paged in the context of this patch.  The
> changes are needed.
>
> _IO_file_xsputn can return EOF if overflow handling fails, i.e.
> flushing to the file fails:

So the patch is needed to propagate the EOF?

Isn't that a violation of what xsputn should do?

I assume we're following ISO C++'s definition of xsputn, in which
case it should return the number of bytes written.

> @@ -1330,9 +1331,10 @@ _IO_new_file_xsputn (f, data, n)
>        _IO_size_t block_size, do_write;
>        /* Next flush the (full) buffer. */
>        if (_IO_OVERFLOW (f, EOF) == EOF)
> -       /* If nothing else has to be written we must not signal the
> -          caller that everything has been written.  */
> -       return to_do == 0 ? EOF : n - to_do;
> +       /* If nothing else has to be written or nothing has been written, we
> +          must not signal the caller that the call was even partially
> +          successful.  */
> +       return (to_do == 0 || to_do == n) ? EOF : n - to_do;

This code, *and* the original code seems wrong, we shouldn't return EOF,
only the number of bytes written e.g. n - to_do.

Does the caller really need to know about the EOF?

>
>        /* Try to maintain alignment: write a whole number of blocks.
>          dont_write is what gets left over. */
>
> Of course, it doesn't currently since _IO_new_file_write (which is
> called in the overflow handling) fails to propagate the error, which
> this patch now fixes. In fact, I wasn't sure if xsputn returning 0 was
> expected since I was expecting it to return either EOF (0 or error) or
> the number of bytes read.  Andreas' response reinforced that and hence
> the modified patch.

I expect xsputn to return the number of bytes written, including 0,
and never EOF, since that is what ISO C++ says.

If we aren't going to follow exactly what xsputn does in C++ then we
should add a very large warning about this.

Cheers,
Carlos.