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

[Tobias Diedrich]

Reimar Döffinger wrote:
> 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).

Maybe a bit of a contrieved example, but:

(echo foo; sleep 1; echo bar) > test.fifo
If I start a read() on the fifo before executing this, the first read
returns "foo" and I have to call read a second time to get the
second half.  So if we want stream_file to work reliably on
pipes/fifos this would be needed I think.

Using the following test program I get:
|ranma at navi:~$ ./fifotest
|nr=3
|nr=3


#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <errno.h>
#include <sys/fcntl.h>
#include <sys/types.h>
#include <sys/stat.h>

int main(int argc, char **argv)
{
	int pid;
	int pfd[2];
	int nr;
	char buf[4096];

	if (pipe(pfd) == -1) {
		perror("pipe");
		return EXIT_FAILURE;
	}

	pid = fork();
	if (pid == -1) {
		perror("fork");
		return EXIT_FAILURE;
	}

	if (pid == 0) {
		close(pfd[0]);
		write(pfd[1], "foo", 3);
		sleep(1);
		write(pfd[1], "bar", 3);
		close(pfd[1]);
	} else {
		close(pfd[1]);
		nr = read(pfd[0], buf, sizeof(buf));
		printf("nr=%d\n", nr);
		nr = read(pfd[0], buf, sizeof(buf));
		printf("nr=%d\n", nr);
		close(pfd[0]);
	}

	return EXIT_SUCCESS;
}

-- 
Tobias						PGP: http://8ef7ddba.uguu.de