[MPlayer-dev-eng] [patch 3/5] Fix stream_write_buffer to make sure all requested bytes are written

[Reimar Döffinger]

On Thu, Jan 06, 2011 at 05:06:33PM +0100, Tobias Diedrich wrote:
> Reimar Döffinger wrote:
> > On Tue, Jan 04, 2011 at 09:35:04PM +0100, Tobias Diedrich wrote:
> > > Index: mplayer-patchset1/stream/stream.c
> > > ===================================================================
> > > --- mplayer-patchset1.orig/stream/stream.c	2010-12-21 20:45:31.157756000 +0100
> > > +++ mplayer-patchset1/stream/stream.c	2010-12-21 20:48:16.229732000 +0100
> > > @@ -324,13 +324,16 @@
> > >  }
> > >  
> > >  int stream_write_buffer(stream_t *s, unsigned char *buf, int len) {
> > > -  int rd;
> > > -  if(!s->write_buffer)
> > > +  int rd = 0;
> > > +  if(!s->write_buffer || len < 0)
> > >      return -1;
> > > -  rd = s->write_buffer(s, buf, len);
> > > -  if(rd < 0)
> > > -    return -1;
> > > -  s->pos += rd;
> > > +  while (rd < len) {
> > > +    int ret = s->write_buffer(s, buf+rd, len-rd);
> > > +    if (ret < 0)
> > > +      return -1;
> > > +    rd += ret;
> > > +    s->pos += ret;
> > > +  }
> > 
> > I think this is a risky place to do it, I'd be more in favour of only
> > adding an assert here and fix all write_buffer implementations to make
> > sure this issue never happens.
> > There is also the issue that if write_buffer ever ends up returning 0
> > this would become an endless loop.
> 
> Something more like this?
> 
> Index: mplayer-patchset1/stream/stream.c
> ===================================================================
> --- mplayer-patchset1.orig/stream/stream.c	2011-01-06 16:16:21.982600000 +0100
> +++ mplayer-patchset1/stream/stream.c	2011-01-06 16:47:28.561260000 +0100
> @@ -28,6 +28,7 @@
>  #endif
>  #include <fcntl.h>
>  #include <strings.h>
> +#include <assert.h>
>  
>  #include "config.h"
>  
> @@ -331,6 +332,7 @@
>    if(rd < 0)
>      return -1;
>    s->pos += rd;
> +  assert(rd == len && "stream_write_buffer(): unexpected short write");
>    return rd;
>  }
>  
> Index: mplayer-patchset1/stream/stream_file.c
> ===================================================================
> --- mplayer-patchset1.orig/stream/stream_file.c	2011-01-06 16:18:13.945756000 +0100
> +++ mplayer-patchset1/stream/stream_file.c	2011-01-06 16:45:35.911193000 +0100
> @@ -52,13 +52,32 @@
>  };
>  
>  static int fill_buffer(stream_t *s, char* buffer, int max_len){
> -  int r = read(s->fd,buffer,max_len);
> -  return (r <= 0) ? -1 : r;
> +  int nr = 0;
> +  int r;
> +  while (nr < max_len) {
> +    r = read(s->fd,buffer,max_len);
> +    if (r < 0)
> +      return -1;
> +    max_len -= r;
> +    buffer += r;
> +    nr += r;
> +    if (r == 0)
> +      break;
> +  }
> +  return nr;
>  }

Have you seen cases where this is necessary for read? The reason I
dislike it is because I think it might interfere with the code that
tries to do a clean exit on CTRL+C.
Also it looks to me like your code allows a 0 return value (which
previously was not possible), IIRC that can cause hangs.