diff mbox

VDR timer + mplayer = VDR crash. Please fix!

Message ID f4aver$gb4$1@video.local.muempf.de
State New
Headers

Commit Message

Stefan Huelswitt June 8, 2007, 7:11 a.m. UTC 
  On 07 Jun 2007 Anssi Hannula <anssi.hannula@gmail.com> wrote:

> Attached is a patch for mplayer plugin which closes file descriptors 
> (except 0, 1, 2) when forking mplayer script.

Thanks.
I took over that with slight changes. The final patch is attached.
Could I please have some feedback if it's working as good as
yours?

Regards.

Comments

Anssi Hannula June 8, 2007, 3:45 p.m. UTC | #1 
Stefan Huelswitt wrote:
> On 07 Jun 2007 Anssi Hannula <anssi.hannula@gmail.com> wrote:
> 
>> Attached is a patch for mplayer plugin which closes file descriptors 
>> (except 0, 1, 2) when forking mplayer script.
> 
> Thanks.
> I took over that with slight changes. The final patch is attached.
> Could I please have some feedback if it's working as good as
> yours?

Testing confirms it is closing the file descriptors as well, yes :)
VDRU VDRU June 8, 2007, 3:58 p.m. UTC | #2 
On 6/8/07, Stefan Huelswitt <s.huelswitt@gmx.de> wrote:
> > Attached is a patch for mplayer plugin which closes file descriptors
> > (except 0, 1, 2) when forking mplayer script.
>
> Thanks.
> I took over that with slight changes. The final patch is attached.
> Could I please have some feedback if it's working as good as
> yours?

I have tested your patch severl times in a row just now and while VDR
did not crash once, mplayer playback was aborted about 20% of the time
when the timer was triggered.  This doesn't (or hasn't so far) happen
with Anssi's patch however.  What are the differences between when/how
he closes the file descriptors and when/how you are?

Many thanks Stefan & Anssi!
Stefan Huelswitt June 8, 2007, 4:07 p.m. UTC | #3 
On 08 Jun 2007 "VDR User" <user.vdr@gmail.com> wrote:

> I have tested your patch severl times in a row just now and while VDR
> did not crash once, mplayer playback was aborted about 20% of the time
> when the timer was triggered.  This doesn't (or hasn't so far) happen
> with Anssi's patch however.  What are the differences between when/how
> he closes the file descriptors and when/how you are?

As far as I see there is no difference beside the different way
to get the max. number of filedescriptors, but this shouldn't
have any impact.

Regards.
VDRU VDRU June 8, 2007, 6:05 p.m. UTC | #4 
On 6/8/07, Stefan Huelswitt <s.huelswitt@gmx.de> wrote:
> As far as I see there is no difference beside the different way
> to get the max. number of filedescriptors, but this shouldn't
> have any impact.

The difference of how/when the fd's were being closed was the only
thing I can think of that may be different between your patch and
Anssi's.  No other changes were made so I'm really at a loss why your
version of the fix randomly aborts playback.  Can you think of any
further tests I can do?

Cheers
VDRU VDRU June 13, 2007, 11:38 p.m. UTC | #5 
Ok, after much testing Stefan's patch frequently aborts mplayer
playback when a timer is triggered while Anssi's doesn't.  I don't
know why this is if both patches are essentially doing the same thing
during the same time.  Something must be different to give different
results.

At any rate, please don't accept Stefan's method into the next version
of the plugin until this last problem has been resolved.

Many thanks for the work to fix this once and for all!
diff mbox

Patch

--- player-mplayer.c
+++ player-mplayer.c
@@ -383,17 +383,19 @@ 
     dsyslog("mplayer: mplayer child started (pid=%d)", getpid());
 
     if(MPlayerSetup.SlaveMode) {
-      close(inpipe[1]);
-      close(outpipe[0]);
       if(dup2(inpipe[0],STDIN_FILENO)<0 ||
          dup2(outpipe[1],STDOUT_FILENO)<0 ||
          dup2(outpipe[1],STDERR_FILENO)<0) {
         esyslog("ERROR: dup2() failed in MPlayer child: (%d) %s",errno,strerror(errno));
         exit(127);
         }
-      close(inpipe[0]);
-      close(outpipe[1]);
       }
+    else {
+      int nfd=open("/dev/null",O_RDONLY);
+      if(nfd<0 || dup2(nfd,STDIN_FILENO)<0)
+        esyslog("ERROR: redirect of STDIN failed in MPlayer child: (%d) %s",errno,strerror(errno));
+      }
+    for(int i=getdtablesize()-1; i>STDERR_FILENO; i--) close(i);
 
     char cmd[64+PATH_MAX*2], aid[20];
     char *fname=Quote(file->FullPath());