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

[Uli Schlachter]

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?

> +	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. How about the following idea which will work in 90% of cases?

--- xcb_in.c.orig	2016-10-13 20:07:52.464589712 +0200
+++ xcb_in.c	2016-10-13 20:13:31.177710447 +0200
@@ -400,7 +400,15 @@ static int read_block(const int fd, void
 #endif /* USE_POLL */
         }
         if(ret <= 0)
+        {
+            if (ret == 0)
+                /* Xlib assumes that errno is meaningful. That's not
going to
+                 * work (e.g. if the connection goes into an error
state at some
+                 * user code using xcb directly), but at least try.
+                 */
+                errno = EPIPE;
             return ret;
+        }
     }
     return len;
 }
@@ -988,6 +996,7 @@ int _xcb_in_read(xcb_connection_t *c)
      */
     if (msg.msg_flags & (MSG_TRUNC|MSG_CTRUNC)) {
         _xcb_conn_shutdown(c, XCB_CONN_CLOSED_FDPASSING_FAILED);
+        errno = ENO_IDEA_WHAT_TO_DO_HERE;
         return 0;
     }
 #else
@@ -1038,6 +1047,12 @@ int _xcb_in_read(xcb_connection_t *c)
     if((n > 0) || (n < 0 && WSAGetLastError() == WSAEWOULDBLOCK))
 #endif /* !_WIN32 */
         return 1;
+    if (n == 0)
+        /* Xlib assumes that errno is meaningful. That's not going to
+         * work (e.g. if the connection goes into an error state at some
+         * user code using xcb directly), but at least try.
+         */
+        errno = EPIPE;
     _xcb_conn_shutdown(c, XCB_CONN_ERROR);
     return 0;
 }

(Sorry for the line wrapping)
The comments tell you how much I like this idea.

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:

int fd = xcb_get_file_descriptor(conn);
if (fd != -1) {
  call select() or poll() or some magic ioctl to check for hung up
} else {
  /* Either old libxcb where the above returns -1 on error connection or
the connection always has been broken (should not happen) */
}

This patch should always work while errno-magic depends on the call
chain when the hang up happens.

What do you think?
Uli
-- 
"Do you know that books smell like nutmeg or some spice from a foreign
land?"
                                              -- Faber in Fahrenheit 451