[Xcb] [PATCH libxcb v2 1/4] xcb_send_fd(): Always close fds
Uli Schlachter
psychon at znc.in
Mon May 18 12:40:14 PDT 2015
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.
More information about the Xcb
mailing list