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

[Uli Schlachter]

Am 16.05.2015 um 15:49 schrieb 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?

Done.

> 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.

xcb_send_fd() returns void, so there is nothing that callers could do about this
anyway.

>>
>> (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..).

I don't actually know, but I guess so. It would get a request that needs an FD
on a connection that can't pass FDs and doesn't provie a FD. So yeah, the server
should generate an error.

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

Done.

>> +{
>> +#if HAVE_SENDMSG
>> +    while (num_fds > 0) {
> 
> Why not use a for-loop i < num_fds ?

I changed this loop. If the connection enters an error state while send_fds() is
running, all the remaining fds will now be closed. Closing them is a lot easier
with this while-loop approach, although I agree that this isn't really readable.

>> +        /* 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?

....

Now this series has one more patch.

>> +            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.

c->has_error can only ever be set. It is never cleared.

I changed the code so that it closes all remaining FDs at the end now in case
only some of them were sent. Thanks for catching this.

Cheers,
Uli

> 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


-- 
"Because I'm in pain," he says. "That's the only way I get your attention."
He picks up the box. "Don't worry, Katniss. It'll pass."
He leaves before I can answer.