[Xcb] [PATCH] _xcb_conn_wait(): Do better at detecting closed sockets

[Adam Jackson]

On Thu, 2016-10-13 at 20:20 +0200, Uli Schlachter wrote:
> On 13.10.2016 17:02, Adam Jackson wrote:
> [...]
> > @@ -484,6 +485,19 @@ int _xcb_conn_wait(xcb_connection_t *c,
> > pthread_cond_t *cond, struct iovec **vec
> >              ret = -1;
> >              break;
> >          }
> > +
> > +	/* hangup with no data left to read is an error */
> 
> Hangup with data left to read is not an error? Why?

Surely from a correctness standpoint every reply or event that we've
already received should be processed. Consider a pair of applications
using ClientMessage events for IPC. If one side does:

    XSendEvent();
    XSync();
    XKillClient();

then it would be entirely reasonable to expect that the event has
actually been sent to the other end before it was disconnected. If we
treated POLLHUP-with-data-remaining as an error, we'd likely throw away
an event already enqueued.

It doesn't end up mattering on Linux, because POLLHUP isn't raised
until there's no data left in the read queue. But I'm told there are
other OSes in the world.

> > +	if (fd.revents & POLLHUP)
> > +	{
> > +	    int unread = -1;
> > +	    ioctl(c->fd, FIONREAD, &unread);
> > +	    if (unread <= 0)
> > +	    {
> > +		/* coerce errno to not be EAGAIN */
> > +		errno = EPIPE;
> > +		ret = -1;
> > +	    }
> > +	}
> >  #else
> >          ret = select(c->fd + 1, &rfds, &wfds, 0, 0);
> >  #endif
> > 
> 
> The above might work in 80% of cases, but won't work if some data is
> left to read.

No. If there's data left to read, the ioctl will return a non-negative
integer in 'unread'. Only if there is nothing left to read will we
tranform this into an error condition.

Although, I am missing a break in the inner if block.  Tsk tsk.

> How about fixing this properly in Xlib instead? If it wants to check
> if the error was due to "the other end closed the connection", it can
> do something like this:

Fair enough. Do note that the existing code already does consider this
case to be an error, due to the stanza above the bit I added:

        ret = poll(&fd, 1, -1);
        /* If poll() returns an event we didn't expect, such as POLLNVAL, treat
         * it as if it failed. */
        if(ret >= 0 && (fd.revents & ~fd.events))
        {
            ret = -1;
            break;
        }

Since we're not presently adding POLLHUP to fd.events...

- ajax