[MPlayer-dev-eng] [PATCH] Use unrar for open vobsubs if available

[Reimar Doeffinger]

On Fri, Dec 07, 2007 at 11:11:45PM +0800, Ulion wrote:
> 2007/12/7, Reimar Doeffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de>:
> > On Fri, Dec 07, 2007 at 07:24:32PM +0800, Ulion wrote:
> > > > No, none of them need to be closed. All you need to do is replace stderr instead of
> > > > closing.
> > > > So something like
> > > > > fd = open("/dev/null", O_WRONLY);
> > > > > if (fd < 3) _exit(EXIT_FAILURE);
> > > > > dup2(fd, 2);
> > > > > close(fd);
> > > > should do it.
> > > > Note that this will cause MPlayer to fail if it was started without
> > > > stdin, stdout or stderr itself (though I don't think we really need
> > > > to care about that).
> > >
> > > dup2() will close target fd if it's openen. So current code looks ok for me:
> > >         /* suppress stderr messages */
> > >         close(2);
> > >         fd = open("/dev/null", O_WRONLY);
> > >         if(fd != 2) {
> > >             if (dup2(fd, 2))
> > >                 _exit(EXIT_FAILURE);
> > >             close(fd);
> > >         }
> > >
> > > Is it ok for you?
> >
> > No, not really. Your code is already like that more complex, but it also handles
> > the case that open fails only implicitly, which is more fragile, and with the
> > dup2 test removed it is just plain broken.
> 
> How about:
> 
>         fd = open("/dev/null", O_WRONLY);
>         if(fd == -1)
>             _exit(EXIT_FAILURE);
>         if (fd != 2) {
>             dup2(fd, 2);
>             close(fd);
>         }

What is the point of the != 2 check then? Either you assume that
stdin, stdout and stderr are correctly set at the beginning,
then fd _can_ not be 2 (because you did not close it before).
Or if you don't assume that and you should check for that case (which,
due to "open" being required to return the smallest non-open
descriptor, my < 3 test does, though I admit it is not ideal
in clearness).
Though in theory
> close(2);
> if (open("/dev/null", O_WRONLY) != 2)
>   _exit(EXIT_FAILURE);

should do the job as well, I usually just prefer being more
explicit with security-relevant stuff.

Greetings,
Reimar Döffinger