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

[Reimar Döffinger]

On 6 jan 2011, at 19:30, Tobias Diedrich <ranma at tdiedrich.de> wrote:
> Reimar Döffinger wrote:
>> On Thu, Jan 06, 2011 at 05:06:33PM +0100, Tobias Diedrich wrote:
>>> 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.
> 
> Well, given that I was adding retry code for write I thought I
> should look at the read codepath too.
> Posix spec says that read() too may be interrupted by signals and
> then return less than requested (probably won't happen on Linux since
> reads should be blocking)

All signals are fatal for MPlayer, however there's a risk your patch makes it more likely to get stuck in a loop so it can't shutdown cleanly.

> and of course it can happen on pipes/fifos.

Why? For writes there are good reasons, but I see no reason why reads from pipes or fifos should ever return only partial data (that kind of info seems rather hard to find though).