Re: [PATCH] Do not break buffers in fvwrite for unbuffered files

[Federico Terraneo <fede dot tft at hotmail dot it>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/31/2013 03:41 PM, Corinna Vinschen wrote:
> On Oct 31 12:55, Federico Terraneo wrote:
>> 
>> This can't happen, since there's an
>> 
>> else if (fp->_p > fp->_bf._base || len < fp->_bf._size)
>> 
>> that catches this case and fills the file buffer instead.
> 
> Oh, hmm.  I didn't notice the || in the if clause.  Doesn't that
> mean that the last else branch is never used since there's always a
> buffer at _bf._base?  The fact that we use a FILE buffer is now
> sufficient to use it and never to write directly, AFAICS.
> 

Not exactly. The

if (fp->_p > fp->_bf._base

isn't true if there's a buffer. It's true if there is at least a
character in the buffer, fp->_p is the 'put pointer'.

The rationale is this: when shall we pass through the buffer? When we
are asked to write less than fp->_bf._size (to write in large chunks and
optimize performance), or when there's at least one character in the
buffer (to preserve the order of written data we can't just forget about
eventual data lying in the buffer)

Consider the following case, assuming fp->_bf._size=1024:

FILE *f=...
char buf1[16];
char buf2[32768];
fwrite(buf1,1,sizeof(buf1),f);
fwrite(buf2,1,sizeof(buf2),f);

The first fwrite goes through the buffer beacuse of its small size (len
< fp->_bf._size is true). The buffer now holds 16 char.

The second fwrite does three iterations:
- - first, since fp->_p > fp->_bf._base is true the buffer is filled with
  the remaining 1024-16 char and flushed as the first 16 char have to
  be written first.
- - the size of the data to write is now 32768-(1024-16)=31760, but
  fp->_p > fp->_bf._base is now false due to the fflush. We now write
  31744 (31KBytes, 31760/1024*1024) directly without passing through
  the buffer.
- - lastly, we are left with another 16 char to write, the difference
  between 31760 and 31744, and those will be put in the buffer.

Attached is a patch with the requested changes in the previous mail.



-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJSc3bEAAoJECkLFtN5Xr9fATYH/33ljM/Cm4Hqj0ctXTXJiItQ
kcvsORwSft96hpD9cdiXMcw+mnw/3/MatyCvwwA9aeqUpxmxHJKyTCy70hRJyKcI
xPBl5aJeV0tm19CGjjIOJK2aOJnjcjLgPQVLd17tVXxcJd6aPj63mhvKXRmSvxOa
YY2T2x13fXjEmJA2ft8VcCbCfOUmbaTOohnifNLlydfFO+V3PJxEaRdtmYN9EfgY
OuMQc8G+09LhYVZV3kHyRAcBjFyFaW60NKKResjhiBJwycPS4iEOOuGVlRDwrwdi
Gx5n1Ci5NZfsbsy2fg06WHkIcpdozPEE22h+1iCnj6RH/ZNr/TpLsJoZ5SeYgqs=
=TP8b
-----END PGP SIGNATURE-----

diff -ruN a/newlib/libc/stdio/fvwrite.c b/newlib/libc/stdio/fvwrite.c
--- a/newlib/libc/stdio/fvwrite.c	2013-10-31 01:10:06.110679241 +0100
+++ b/newlib/libc/stdio/fvwrite.c	2013-10-31 19:58:15.828200705 +0100
@@ -21,6 +21,7 @@
 #include <string.h>
 #include <stdlib.h>
 #include <errno.h>
+#include <limits.h>
 #include "local.h"
 #include "fvwrite.h"
 
@@ -89,12 +90,14 @@
   if (fp->_flags & __SNBF)
     {
       /*
-       * Unbuffered: write up to BUFSIZ bytes at a time.
+       * Unbuffered: split the buffer in the larger chunk that
+       * is less than INT_MAX, as some legacy code may expect
+       * int instead of size_t
        */
       do
 	{
 	  GETIOV (;);
-	  w = fp->_write (ptr, fp->_cookie, p, MIN (len, BUFSIZ));
+	  w = fp->_write (ptr, fp->_cookie, p, MIN (len, INT_MAX));
 	  if (w <= 0)
 	    goto err;
 	  p += w;
@@ -177,30 +180,24 @@
 	      fp->_p += w;
 	      w = len;		/* but pretend copied all */
 	    }
-	  else if (fp->_p > fp->_bf._base && len > w)
+	  else if (fp->_p > fp->_bf._base || len < fp->_bf._size)
 	    {
-	      /* fill and flush */
+	      /* pass through the buffer */
+	      w = MIN (len, w);
 	      COPY (w);
-	      /* fp->_w -= w; *//* unneeded */
+	      fp->_w -= w;
 	      fp->_p += w;
-	      if (_fflush_r (ptr, fp))
+	      if (fp->_w == 0 && _fflush_r (ptr, fp))
 		goto err;
 	    }
-	  else if (len >= (w = fp->_bf._size))
+	  else
 	    {
 	      /* write directly */
+	      w = ((int)MIN (len, INT_MAX)) / fp->_bf._size * fp->_bf._size;
 	      w = fp->_write (ptr, fp->_cookie, p, w);
 	      if (w <= 0)
 		goto err;
 	    }
-	  else
-	    {
-	      /* fill and done */
-	      w = len;
-	      COPY (w);
-	      fp->_w -= w;
-	      fp->_p += w;
-	    }
 	  p += w;
 	  len -= w;
 	}

Attachment: fvwrite.patch.sig
Description: Binary data