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

[Corinna Vinschen <vinschen at redhat dot com>]

On Oct 31 12:55, Federico Terraneo wrote:
> >> @@ -89,12 +90,16 @@ if (fp->_flags & __SNBF) { /* -       *
> >> Unbuffered: write up to BUFSIZ bytes at a time. +       *
> >> Unbuffered: split the buffer in the larger chunk that +       *
> >> is both less than INT_MAX, as some legacy code may +       *
> >> expect int instead of size_t, and is a multiple of +       *
> >> fp->_bf._size. */ do { GETIOV (;); -	  w = fp->_write (ptr,
> >> fp->_cookie, p, MIN (len, BUFSIZ)); +	  w = fp->_write (ptr,
> >> fp->_cookie, p, +	    ((int)MIN(len, INT_MAX)) / fp->_bf._size *
> >> fp->_bf._size);
> > 
> > What if len is < fp->_bf._size?  AFAICS, this would result in
> > writing 0 bytes.
> 
> You're right. Thanks for noticing it.
> 
> One way to fix it is:
> 
>   if (len < fp->_bf._size)
>     w = len;
>   else
>     w = ((int)MIN (len, INT_MAX)) / fp->_bf._size * fp->_bf._size;
>   w = fp->_write (ptr, fp->_cookie, p, w);
> 
> but maybe we can just do a
> 
>   w = fp->_write (ptr, fp->_cookie, p, MIN (len, INT_MAX));
> 
> since with unbuffered files there's no real way to avoid writing in
> sizes not multiple of fp->_bf._size anyway.

Yes, on second thought that should be sufficient.  Worse, we don't even
know if fp->_bf._size isn't == 0 in the unbuffered case.

> >> if (w <= 0) goto err; p += w; @@ -177,30 +182,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(len > w)
> >> if(_fflush_r (ptr, fp))
> > 
> > This looks weird.  Shouldn't that be
> > 
> > if (len > w && _fflush_r (ptr, fp))
> > 
> > or, for more clearness
> > 
> > if (fp->_w == 0 && _fflush_r (ptr, fp))
> > 
> > ?   Here's why:
> > 
> > The original test was performed before changing the buffer.  By
> > testing len > w, it made sure to fill the buffer.  Now the test is
> > performed after changing the buffer, so it can precisely test if
> > the buffer is full.  In other words, check for fp->_w == 0.
> > Testing len > w is kind of circuitous at this point, isn't it?
> 
> I did think about doing a fp->_w == 0, but I noticed that the previous
> code when the buffer was empty a write of size fp->_bf._size would
> bypass the buffer and write directly. On the other hand, if the buffer
> is gradually filled, say the buffer is 1024 bytes and you do two
> writes of 512, the buffer is not flushed on the second call, and the
> flush is delayed till another write finds the buffer entirely full.
> 
> Since I didn't know if this behaviour was desired or not, I've
> replicated it with the len > w check. However, for me either of the
> two option is good, so let me know which one you prefer and I'll send
> you another patch.

Never mind.

> >> /* write directly */ +	      w = (int)MIN(len, INT_MAX) /
> >> fp->_bf._size * fp->_bf._size;
> > 
> > Again, what if len < fp->_bf._size?
> 
> 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.

> >> 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; }
> > 
> > Other than that, your formatting is missing a couple of spaces.
> > Make that "if (...)" rather than "if(...)" or "MIN (...)" rather
> > than "MIN(...)".
> 
> Will be fixed in the next patch.

Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

Attachment: pgpKlKRIFdS_P.pgp
Description: PGP signature