[Xcb] [PATCH libxcb v2 1/4] xcb_send_fd(): Always close fds

[Ran Benita]

Hi Uli,

I really don't know anything about xcb locking etc. but I'll try anyway :)

On Thu, May 14, 2015 at 09:52:36AM +0200, Uli Schlachter wrote:
> The API docs for xcb_send_fd() says "After this function returns, the file
> descriptor given is owned by xcb and will be closed eventually".

> Let the implementation live up to its documentation.

Can you add that this is already done on success, just not on the error
paths?

I also checked
http://cgit.freedesktop.org/mesa/mesa/tree/src/glx/dri3_glx.c?id=35c28103b02598bb5f7b4888384b02d31ee371b5#n939
which doesn't close the fd itself after calling xcb. So this change
seems good here.

> 
> (This also does sneak in some preparatory functions for follow-up commits and
> thus does things in a more complicated way than really necessary.)
> 
> Signed-off-by: Uli Schlachter <psychon at znc.in>
> ---
>  src/xcb_out.c | 43 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 33 insertions(+), 10 deletions(-)
> 
> diff --git a/src/xcb_out.c b/src/xcb_out.c
> index 8cc5be8..51d911f 100644
> --- a/src/xcb_out.c
> +++ b/src/xcb_out.c
> @@ -177,6 +177,33 @@ uint32_t xcb_get_maximum_request_length(xcb_connection_t *c)
>      return c->out.maximum_request_length.value;
>  }
>  
> +static void close_fds(unsigned int num_fds, int *fds)
> +{
> +    for (unsigned int index = 0; index < num_fds; index++)
> +        close(fds[index]);
> +}
> +
> +static void send_fds(xcb_connection_t *c, unsigned int num_fds, int *fds)

You don't change this but I wonder anyway:
If sendfd() is not available, xcb_send_fd() (now send_fds()) is a nop,
which is strange. In that case it's expected that the server delivers an
error? (Not that anyone would try to use DRI3 on windows..).

Also I usually prefer the size to come after the array (unless there's
precedence).

> +{
> +#if HAVE_SENDMSG
> +    while (num_fds > 0) {

Why not use a for-loop i < num_fds ?

> +        /* 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);

_xcb_out_flush_to() potentially releases iolock (in a condwait). I
haven't checked, but just to make sure, is there something that prevents
another thread from entering here and interleaving its own fds?

> +            if (c->has_error)
> +                break;
> +        }
> +        if (!c->has_error)

Can has_error be cleared between iterations? If not, it seems better to
return early. If yes, then does that mean some, but not all, fds can be
skipped? That feels like it can cause problems, especially if the
request expects a specific fd in a specific position.

Ran

> +            c->out.out_fd.fd[c->out.out_fd.nfd++] = fds[0];
> +
> +        fds++;
> +        num_fds--;
> +    }
> +#else
> +    close_fds(num_fds, fds);
> +#endif
> +}
> +
>  uint64_t xcb_send_request64(xcb_connection_t *c, int flags, struct iovec *vector, const xcb_protocol_request_t *req)
>  {
>      uint64_t request;
> @@ -295,19 +322,15 @@ unsigned int xcb_send_request(xcb_connection_t *c, int flags, struct iovec *vect
>  void
>  xcb_send_fd(xcb_connection_t *c, int fd)
>  {
> -#if HAVE_SENDMSG
> -    if (c->has_error)
> +    int fds[1] = { fd };
> +
> +    if (c->has_error) {
> +        close(fd);
>          return;
> -    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_lock(&c->iolock);
> +    send_fds(c, 1, &fds[0]);
>      pthread_mutex_unlock(&c->iolock);
> -#endif
>  }
>  
>  int xcb_take_socket(xcb_connection_t *c, void (*return_socket)(void *closure), void *closure, int flags, uint64_t *sent)
> -- 
> 2.1.4
> 
> _______________________________________________
> Xcb mailing list
> Xcb at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/xcb