[Xcb] [PATCH libxcb 4/4] send_fds(): Handle too many outstanding requests

[Ran Benita]

On Thu, May 14, 2015 at 09:55:35AM +0200, Uli Schlachter wrote:
> Before this patch, the following code caused an endless loop in send_fds(),
> because the queue of FDs to send was eventually full, but _xcb_out_flush_to()
> didn't make any progress, since there was no FD to send:
> 
>    while (1) { xcb_send_fd(conn, dup(some_fd)); }
> 
> Fix this by sending a sync when flushing didn't make any progress. That way we
> actually have something to send and can attach the pending FDs.
> 
> Because send_fds() can now send requests, the code in
> xcb_send_request_with_fds64() has to be changed. It has to call send_fds()
> before it establishes a good sequence number for the request it wants to send.

I think I'm really misunderstanding here, so feel free to ignore.
But fds are associated with a specific request (and are sent as
ancillary data with the request itself). So sending a *dummy* request to
clear up the fds buffer seems really strange - what about the original
request they were intended for?

I don't really understand why the fds array of a request can't just be
kept along with it in whatever queue xcb keeps, then there's no need for
synchronization. With your new combined send_request_with_fds(), it seems
possible to make it so that the request and the fds flow together to the
eventual sendmsg().

I guess xcb_send_fd() still needs to use the shared fds buffer by design,
but can't it just be removed? After all we don't support using
libxcb-foo.so without the exact matching version of libxcb.so. Or is it
considered public API?

Ran

> 
> Signed-off-by: Uli Schlachter <psychon at znc.in>
> ---
>  src/xcb_out.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/src/xcb_out.c b/src/xcb_out.c
> index 06fae4c..37fd16d 100644
> --- a/src/xcb_out.c
> +++ b/src/xcb_out.c
> @@ -187,11 +187,17 @@ static void send_fds(xcb_connection_t *c, unsigned int num_fds, int *fds)
>  {
>  #if HAVE_SENDMSG
>      while (num_fds > 0) {
> -        /* FIXME: This will busy-loop when XCB_MAX_PASS_FD fds are sent at once */
>          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->out.out_fd.nfd == XCB_MAX_PASS_FD) {
> +                /* We need some request to send FDs with */
> +                _xcb_out_send_sync(c);
> +                if (c->has_error)
> +                    break;
> +            }
>          }
>          if (!c->has_error)
>              c->out.out_fd.fd[c->out.out_fd.nfd++] = fds[0];
> @@ -296,6 +302,11 @@ uint64_t xcb_send_request_with_fds64(xcb_connection_t *c, int flags, struct iove
>      /* get a sequence number and arrange for delivery. */
>      pthread_mutex_lock(&c->iolock);
>  
> +    /* send FDs before establishing a good request number, because this might
> +     * call send_sync(), too
> +     */
> +    send_fds(c, num_fds, fds);
> +
>      prepare_socket_request(c);
>  
>      /* send GetInputFocus (sync_req) when 64k-2 requests have been sent without
> @@ -304,7 +315,7 @@ uint64_t xcb_send_request_with_fds64(xcb_connection_t *c, int flags, struct iove
>       * applications see sequence 0 as that is used to indicate
>       * an error in sending the request
>       */
> -     
> +
>      while ((req->isvoid && c->out.request == c->in.request_expected + (1 << 16) - 2) ||
>             (unsigned int) (c->out.request + 1) == 0)
>      {
> @@ -312,7 +323,6 @@ uint64_t xcb_send_request_with_fds64(xcb_connection_t *c, int flags, struct iove
>          prepare_socket_request(c);
>      }
>  
> -    send_fds(c, num_fds, fds);
>      send_request(c, req->isvoid, workaround, flags, vector, veclen);
>      request = c->has_error ? 0 : c->out.request;
>      pthread_mutex_unlock(&c->iolock);
> -- 
> 2.1.4
> 
> _______________________________________________
> Xcb mailing list
> Xcb at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/xcb