Re: fwrite.c bugfix

[Remco Treffkorn <remco@rvt.com>]

On Friday 11 July 2003 01:59, Ethan Benson wrote:
> On Thu, Jul 10, 2003 at 05:35:33PM -0700, Remco Treffkorn wrote:
> > Fwrite is supposed to return the number of items written.
>
> number of items, NOT number of characters:
>

I write: "Fwrite is supposed to return the number of items written", you write 
"number of items, NOT number of characters". You are re-stating what I said. 
Am I missing the point?

> from the man page:
...
I read the man page before I fixed fwrite().

>
> > The original version returned -1 on complete success.
>
> your right that return value appears broken, but your patch doesn't
> fix it properly, im tired but as i read the man page the only flaw is
> return count should be return bwrote.
>
> that variable isn't well named either, its not bytes wrote, its nmemb
> wrote.

It does not 'appear' to be broken, it is broken.
The return value should be the number of items successfully written. There is 
no anbiguity here. Please re-consider, the patch appears to fix it properly.

>
> > If I read the man page correctly, the following patch should fix it.
>
> the error returns were correct as i read the code and man page.
> please check your tests with glibc too so you can compare behavior.

I can not test with glibc, since I have no easy way to make it fail on the 
second item. I can only look at the code and the spec/man-page. The trivial 
case test agrees with me. The return valu for a single item successfully 
written is 1, not -1 as in your case.

Let's consider yesterdays version:
size_t fwrite(const void *buf, size_t size, size_t count, FILE *fp)
{
     ssize_t ret;
     size_t bwrote = 0;

     while (count--) {
          ret = write(fileno(fp), buf, size);
          if (ret == -1) {
               fp->error |= _STDIO_ERROR;
               return count-bwrote;
          } else if ((size_t)ret != size) {
               fp->error |= _STDIO_EOF;
               return count-bwrote;
          } else {
               bwrote++;
               buf += ret;
          }
     }
     return count;
}

In the trivial case 'count' is one. You post-decrement count, then enter the 
body and (let's assume) successfully write the item. You will then increment 
'bwrote' by one to one. You test and post-decrement count again. Because 
'count' was 0, you will not enter the body, but return 'count', which is now 
-1 because of the post-decrement.

In the trivial error case you will return 'count-bwrote' or 0-0, which happens 
to be correct for this case. In the non-trivial case, where count is > 1, you 
are in a world of pain.

Assume 'count=2'. The first item writes ok, you are trying to write the 
second, and it fails. 'count' is now 0 (twice evaluated with post-decrement) 
and bwrote is 1 (for one item written). You will return 'count-bwrote', which 
is -1 again, which is wrong again. Sure the variable name 'bwrote' is 
misleading, but I did not choose it.

Have a look at todays version :
size_t fwrite(const void *buf, size_t size, size_t count, FILE *fp)
{
     ssize_t ret;
     size_t nwrote = 0; /* item count, eg: number of size chunks */

     while (count--) {
          ret = write(fileno(fp), buf, size);
          if (ret == -1) {
               fp->error |= _STDIO_ERROR;
               return count-nwrote;
          } else if ((size_t)ret != size) {
               fp->error |= _STDIO_EOF;
               return count-nwrote;
          } else {
               nwrote++;
               buf += ret;
          }
     }
     return nwrote;
}

This fixes the trivial, non-error case. The name 'nwrote' better reflects 
purpose. The return value in the error case is still wrong. For the 'two item 
case failing on the second' case you still return -1 instead of 1. You need 
to return 'nwrote', not 'count-nwrote'.

Do you want me to re-submit a patch against todays version, or is the original 
from yesterday still usefull?
 
BTW: Mandatory overtime? So you have not progressed to the 40 percent paycut 
yet? You live a sheltered life :-)

-- 
Remco Treffkorn (RT445)
HAM DC2XT
remco@rvt.com   (831) 685-1201