[MPlayer-dev-eng] [PATCH] Network synchronized playback using UDP

[Reimar Döffinger]

On Wed, Feb 24, 2010 at 07:40:55PM -0800, Jason Holt wrote:
> >
> > > -static void exit_sighandler(int x){
> > > +void exit_sighandler(int x){
> >
> > No way.
> >
> 
> No idea what you're objecting to.

Making it public and using it from anywhere else. I don't want a signal handler
called from normal code.

> > > +    long sock_flags;
> >
> > Never ever use the long type. Just don't.
> >
> >
> man fcntl says:
> 
>         int fcntl(int fd, int cmd, long arg);
> 
> But I assume you know what you're talking about and changed it to int.

Sorry, I somehow imagined it was the variable you used for the return
value.
In this case it seems like long unfortunately is the right type :-(

> > > +    if (n == -1 && errno == EINTR) {
> > > +        exit_sighandler(SIGINT); // actual signal number isn't available
> > > +    }
> >
> > Not sure, but I suspect you completely misunderstood what EINTR means.
> > In particular this would end up calling the signal handler a second time
> > for a single signal.
> >
> 
> I recall reading several long, conflicting threads about EINTR, like this
> one (with Linus, even):
> 
> http://lkml.indiana.edu/hypermail/linux/kernel/0507.3/0004.html

That is about SA_RESTART behaviour, what I can't figure out where you found
the idea of calling the signal handler manually.
The point of interrupting those functions is to be able to call the signal
handler after all.

> > > +        if (strcmp(mesg, "bye") == 0 || errno == EINTR) {
> > > +            mp_msg(MSGT_CPLAYER, MSGL_INFO, MSGTR_MasterQuit);
> >
> > At the very least the message is nonsense for EINTR.
> > In principle, not retrying will break SIGTRAP debugging helper code -
> > not that I consider it work much effort to avoid that.
> 
> errno == EINTR is unnecessary there due to while (-1 != recvfrom...)

The main point is that the code is definitely wrong because it prints a
message that is wrong for EINTR.
I'll come back to the loop another time, but mostly I think the small tricky
differences either need to go or the code be changed to be more self-explanatory
about them, a huge comment is not good enough IMO.