[Xcb] [PATCH 4/8] Add xcb_send_fd API

[Uli Schlachter]

On 07.11.2013 04:45, Keith Packard wrote:
> This uses sendmsg to transmit file descriptors from the application to
> the X server
> 
> Signed-off-by: Keith Packard <keithp at keithp.com>
[...]
> diff --git a/src/xcb_auth.c b/src/xcb_auth.c
> index a5b730c..2f7c93e 100644
> --- a/src/xcb_auth.c
> +++ b/src/xcb_auth.c
> @@ -34,6 +34,12 @@
>  #include <sys/param.h>
>  #include <unistd.h>
>  #include <stdlib.h>
> +#ifndef _WIN32
> +#include <arpa/inet.h>
> +#include <sys/socket.h>
> +#include <netinet/in.h>
> +#include <sys/un.h>
> +#endif
>  
>  #ifdef __INTERIX
>  /* _don't_ ask. interix has INADDR_LOOPBACK in here. */
> @@ -47,11 +53,6 @@
>  #include <X11/Xwindows.h>
>  #endif
>  #include "xcb_windefs.h"
> -#else
> -#include <arpa/inet.h>
> -#include <sys/socket.h>
> -#include <netinet/in.h>
> -#include <sys/un.h>
>  #endif /* _WIN32 */
>  
>  #include "xcb.h"

The above seems to be an unrelated change. If this is needed due to some
xcb-private header, then that header needs to be fixed. Otherwise this seems
unneeded (for this patch).

> diff --git a/src/xcb_conn.c b/src/xcb_conn.c
> index 7dd25d3..0237d16 100644
> --- a/src/xcb_conn.c
> +++ b/src/xcb_conn.c
> @@ -38,6 +38,11 @@
>  #include <errno.h>
>  #include <limits.h>
>  
> +#ifndef _WIN32
> +#include <sys/socket.h>
> +#include <netinet/in.h>
> +#endif /* !_WIN32 */
> +
>  #include "xcb.h"
>  #include "xcbint.h"
>  #if USE_POLL
> @@ -48,9 +53,6 @@
>  
>  #ifdef _WIN32
>  #include "xcb_windefs.h"
> -#else
> -#include <sys/socket.h>
> -#include <netinet/in.h>
>  #endif /* _WIN32 */

Same here.

>  /* SHUT_RDWR is fairly recent and is not available on all platforms */
> @@ -214,9 +216,34 @@ static int write_vec(xcb_connection_t *c, struct iovec **vector, int *count)
>      if (n > IOV_MAX)
>  	n = IOV_MAX;
>  
> -    n = writev(c->fd, *vector, n);
> -    if(n < 0 && errno == EAGAIN)
> -        return 1;
> +#if HAVE_SENDMSG
> +    if (c->out.out_fd.nfd) {
> +	struct msghdr msg = {
> +	    .msg_name = NULL,
> +	    .msg_namelen = 0,
> +	    .msg_iov = *vector,
> +	    .msg_iovlen = n,
> +	    .msg_control = &c->out.out_fd,
> +	    .msg_controllen = sizeof (struct cmsghdr) + c->out.out_fd.nfd * sizeof (int),
> +	};
> +        int i;
> +	c->out.out_fd.cmsghdr.cmsg_len = msg.msg_controllen;
> +	c->out.out_fd.cmsghdr.cmsg_level = SOL_SOCKET;
> +	c->out.out_fd.cmsghdr.cmsg_type = SCM_RIGHTS;
> +	n = sendmsg(c->fd, &msg, 0);
> +        if(n < 0 && errno == EAGAIN)
> +            return 1;
> +        for (i = 0; i < c->out.out_fd.nfd; i++)
> +            close(c->out.out_fd.fd[i]);
> +        c->out.out_fd.nfd = 0;
> +    } else
> +#endif
> +    {
> +	n = writev(c->fd, *vector, n);
> +        if(n < 0 && errno == EAGAIN)
> +            return 1;
> +    }
> +    
>  #endif /* _WIN32 */    
>  
>      if(n <= 0)

This indents with tabs. I know that there is no coding style documented
anywhere, but "most" of libxcb only uses spaces.

> diff --git a/src/xcb_ext.c b/src/xcb_ext.c
> index 831f283..1ff2b05 100644
> --- a/src/xcb_ext.c
> +++ b/src/xcb_ext.c
> @@ -31,6 +31,9 @@
>  
>  #include <stdlib.h>
>  #include <string.h>
> +#ifndef _WIN32
> +#include <sys/socket.h>
> +#endif

Same as above: Why?

> diff --git a/src/xcb_in.c b/src/xcb_in.c
> index 8a7af92..ac4d284 100644
> --- a/src/xcb_in.c
> +++ b/src/xcb_in.c
> @@ -36,16 +36,17 @@
>  #include <stdio.h>
>  #include <errno.h>
>  
> +#ifndef _WIN32
> +#include <sys/select.h>
> +#include <sys/socket.h>
> +#endif
> +
>  #include "xcb.h"
>  #include "xcbext.h"
>  #include "xcbint.h"
>  #if USE_POLL
>  #include <poll.h>
>  #endif
> -#ifndef _WIN32
> -#include <sys/select.h>
> -#include <sys/socket.h>
> -#endif

I think that I must be missing something here...

If there is a reason for this, I'd like to have it in the commit message. If
this is just supposed to be clean up, it should be split out into its own
commit. If this is just needed for xcbint.h, then xcbint.h should include those
headers.

> diff --git a/src/xcb_list.c b/src/xcb_list.c
> index 129540b..075c535 100644
> --- a/src/xcb_list.c
> +++ b/src/xcb_list.c
> @@ -30,6 +30,9 @@
>  #endif
>  
>  #include <stdlib.h>
> +#ifndef _WIN32
> +#include <sys/socket.h>
> +#endif
>  
>  #include "xcb.h"
>  #include "xcbint.h"
> diff --git a/src/xcb_out.c b/src/xcb_out.c
> index 429fa99..ed8458c 100644
> --- a/src/xcb_out.c
> +++ b/src/xcb_out.c
> @@ -33,6 +33,9 @@
>  #include <stdlib.h>
>  #include <unistd.h>
>  #include <string.h>
> +#ifndef _WIN32
> +#include <sys/socket.h>
> +#endif
>  
>  #include "xcb.h"
>  #include "xcbext.h"
> @@ -263,6 +266,22 @@ unsigned int xcb_send_request(xcb_connection_t *c, int flags, struct iovec *vect
>      return request;
>  }
>  
> +void
> +xcb_send_fd(xcb_connection_t *c, int fd)
> +{
> +#if HAVE_SENDMSG

  if (c->has_error)
    return;

This is needed before the pthread_mutex_lock(), because if
xcb_connect_to_display() fails, it returns a pointer to an integer. The
has_error 'field' is the only thing that might be accessed for such a connection
object.

> +    pthread_mutex_lock(&c->iolock);
> +    while (c->out.out_fd.nfd == XCB_MAX_PASS_FD) {
> +	_xcb_out_flush_to(c, c->out.request);
> +	if (c->has_error)
> +	    break;
> +    }
> +    if (!c->has_error)
> +	c->out.out_fd.fd[c->out.out_fd.nfd++] = fd;
> +    pthread_mutex_unlock(&c->iolock);
> +#endif
> +}

So, xcb_send_fd() hands FDs to libxcb which will all be included in the next
flush. The idea is that the user of the library calls (through the automatically
generated protocol functions) xcb_send_fd() and then xcb_send_request(). Correct?

However, what happens if the connection is flushed between the xcb_send_fd() and
the xcb_send_request() calls, because another thread "interferes"? This can also
happen with a single thread: xcb_send_request() could have to insert sync
requests (GetInputFocus) because of sequence number 0 or sequence number wraps.
This could cause the output buffer to go full before the "real" request is inserted.

I guess that the server expects the FDs to be sent with the request that they
are meant to (otherwise I could feed the server lots of FDs which it would have
to keep around forever). So this function doesn't really work, does it?

>  int xcb_take_socket(xcb_connection_t *c, void (*return_socket)(void *closure), void *closure, int flags, uint64_t *sent)
>  {
>      int ret;
> diff --git a/src/xcb_xid.c b/src/xcb_xid.c
> index 79a9a27..3c5682c 100644
> --- a/src/xcb_xid.c
> +++ b/src/xcb_xid.c
> @@ -31,6 +31,10 @@
>  
>  #include <assert.h>
>  #include <stdlib.h>
> +#ifndef _WIN32
> +#include <sys/socket.h>
> +#endif

...I must be missing something.

> diff --git a/src/xcbext.h b/src/xcbext.h
> index 4e1f2f7..44030c3 100644
> --- a/src/xcbext.h
> +++ b/src/xcbext.h
> @@ -59,6 +59,8 @@ enum xcb_send_request_flags_t {
>  
>  unsigned int xcb_send_request(xcb_connection_t *c, int flags, struct iovec *vector, const xcb_protocol_request_t *request);
>  
> +void xcb_send_fd(xcb_connection_t *c, int fd);
> +
>  /* xcb_take_socket allows external code to ask XCB for permission to
>   * take over the write side of the socket and send raw data with
>   * xcb_writev. xcb_take_socket provides the sequence number of the last
> diff --git a/src/xcbint.h b/src/xcbint.h
> index 7f9ab28..b2dfed3 100644
> --- a/src/xcbint.h
> +++ b/src/xcbint.h
> @@ -81,6 +81,16 @@ void *_xcb_map_remove(_xcb_map *q, unsigned int key);
>  
>  typedef void (*xcb_return_socket_func_t)(void *closure);
>  
> +#if HAVE_SENDMSG
> +#define XCB_MAX_PASS_FD	16
> +
> +typedef struct _xcb_fd {
> +    struct cmsghdr cmsghdr;
> +    int fd[XCB_MAX_PASS_FD];
> +    int nfd;
> +} _xcb_fd;
> +#endif
> +
>  typedef struct _xcb_out {
>      pthread_cond_t cond;
>      int writing;
> @@ -101,6 +111,9 @@ typedef struct _xcb_out {
>          xcb_big_requests_enable_cookie_t cookie;
>          uint32_t value;
>      } maximum_request_length;
> +#if HAVE_SENDMSG
> +    _xcb_fd out_fd;
> +#endif
>  } _xcb_out;
>  
>  int _xcb_out_init(_xcb_out *out);
> 


-- 
"In the beginning the Universe was created. This has made a lot of
 people very angry and has been widely regarded as a bad move."