[Xcb-commit] libxcb: 6 commits - src
Uli Schlachter
psychon at kemper.freedesktop.org
Fri Jun 12 00:45:43 PDT 2015
src/c_client.py | 12 ++++++-
src/xcb_in.c | 1
src/xcb_out.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++---------
src/xcbext.h | 77 ++++++++++++++++++++++++++++++++++++++++++++++----
4 files changed, 153 insertions(+), 22 deletions(-)
New commits:
commit f85661c3bca97faa72431df92a3867be39a74e23
Author: Michel Dänzer <michel.daenzer at amd.com>
Date: Mon Jun 1 11:04:18 2015 +0900
Call _xcb_wake_up_next_reader from xcb_wait_for_special_event
All functions calling _xcb_conn_wait() must make sure that waiting
readers are woken up when we read a reply or event that they are waiting
for. xcb_wait_for_special_event() did not do so. This adds the missing
call to_xcb_in_wake_up_next_reader().
Fixes deadlock when waiting for a special event and concurrently
processing the display connection queue in another thread.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84252
Tested-by: Thomas Daede <bztdlinux at gmail.com>
Tested-by: Clément Guérin <geecko.dev at free.fr>
Reviewed-by: Uli Schlachter <psychon at znc.in>
Signed-off-by: Michel Dänzer <michel at daenzer.net>
Signed-off-by: Uli Schlachter <psychon at znc.in>
diff --git a/src/xcb_in.c b/src/xcb_in.c
index 623a0a8..322bed8 100644
--- a/src/xcb_in.c
+++ b/src/xcb_in.c
@@ -761,6 +761,7 @@ xcb_generic_event_t *xcb_wait_for_special_event(xcb_connection_t *c,
if(!_xcb_conn_wait(c, &se->special_event_cond, 0, 0))
break;
+ _xcb_in_wake_up_next_reader(c);
pthread_mutex_unlock(&c->iolock);
return event;
}
commit 8584c0e09573a29d8ba7050e3d5afd925b4d8d80
Author: Uli Schlachter <psychon at znc.in>
Date: Thu May 14 09:44:05 2015 +0200
send_fds(): Handle too many outstanding FDs to send
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 request 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.
Signed-off-by: Uli Schlachter <psychon at znc.in>
diff --git a/src/xcb_out.c b/src/xcb_out.c
index b5b6f34..3601a5f 100644
--- a/src/xcb_out.c
+++ b/src/xcb_out.c
@@ -196,12 +196,16 @@ static void send_fds(xcb_connection_t *c, int *fds, unsigned int num_fds)
prepare_socket_request(c);
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 && !c->has_error) {
/* XXX: if c->out.writing > 0, this releases the iolock and
* potentially allows other threads to interfere with their own fds.
*/
_xcb_out_flush_to(c, c->out.request);
+
+ 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;
@@ -306,6 +310,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, fds, num_fds);
+
prepare_socket_request(c);
/* send GetInputFocus (sync_req) when 64k-2 requests have been sent without
@@ -314,7 +323,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)
{
@@ -322,7 +331,6 @@ uint64_t xcb_send_request_with_fds64(xcb_connection_t *c, int flags, struct iove
prepare_socket_request(c);
}
- send_fds(c, fds, num_fds);
send_request(c, req->isvoid, workaround, flags, vector, veclen);
request = c->has_error ? 0 : c->out.request;
pthread_mutex_unlock(&c->iolock);
commit 658fb4a5f0050db68fdf092936afe596412ef5f7
Author: Uli Schlachter <psychon at znc.in>
Date: Wed Apr 22 09:26:05 2015 +0200
Code generator: Use xcb_send_request_with_fds()
Signed-off-by: Uli Schlachter <psychon at znc.in>
diff --git a/src/c_client.py b/src/c_client.py
index 9a7c67c..70f8429 100644
--- a/src/c_client.py
+++ b/src/c_client.py
@@ -2297,6 +2297,9 @@ def _c_request_helper(self, name, void, regular, aux=False, reply_fds=False):
_c(' unsigned int i;')
_c(' unsigned int xcb_tmp_len;')
_c(' char *xcb_tmp;')
+ num_fds = len([field for field in param_fields if field.isfd])
+ if num_fds > 0:
+ _c(' int fds[%d];' % (num_fds))
_c('')
# fixed size fields
@@ -2397,11 +2400,16 @@ def _c_request_helper(self, name, void, regular, aux=False, reply_fds=False):
# no padding necessary - _serialize() keeps track of padding automatically
_c('')
+ fd_index = 0
for field in param_fields:
if field.isfd:
- _c(' xcb_send_fd(c, %s);', field.c_field_name)
+ _c(' fds[%d] = %s;', fd_index, field.c_field_name)
+ fd_index = fd_index + 1
- _c(' xcb_ret.sequence = xcb_send_request(c, %s, xcb_parts + 2, &xcb_req);', func_flags)
+ if num_fds == 0:
+ _c(' xcb_ret.sequence = xcb_send_request(c, %s, xcb_parts + 2, &xcb_req);', func_flags)
+ else:
+ _c(' xcb_ret.sequence = xcb_send_request_with_fds(c, %s, xcb_parts + 2, &xcb_req, %d, fds);', func_flags, num_fds)
# free dyn. all. data, if any
for f in free_calls:
commit b15aa6bd4efde784e546d168bb23b8a8e816e85b
Author: Uli Schlachter <psychon at znc.in>
Date: Wed Apr 22 09:23:47 2015 +0200
Add xcb_send_request_with_fds() and *_with_fds64()
Doing xcb_send_fd(), xcb_send_request() is racy. If two threads do this at the
same time, they could mix up their file descriptors. This commit makes it
possibly to fix this race by providing a single function which does everything
that is needed.
Signed-off-by: Uli Schlachter <psychon at znc.in>
diff --git a/src/xcb_out.c b/src/xcb_out.c
index ec132fc..b5b6f34 100644
--- a/src/xcb_out.c
+++ b/src/xcb_out.c
@@ -214,15 +214,18 @@ static void send_fds(xcb_connection_t *c, int *fds, unsigned int num_fds)
close_fds(fds, num_fds);
}
-uint64_t xcb_send_request64(xcb_connection_t *c, int flags, struct iovec *vector, const xcb_protocol_request_t *req)
+uint64_t xcb_send_request_with_fds64(xcb_connection_t *c, int flags, struct iovec *vector,
+ const xcb_protocol_request_t *req, unsigned int num_fds, int *fds)
{
uint64_t request;
uint32_t prefix[2];
int veclen = req->count;
enum workarounds workaround = WORKAROUND_NONE;
- if(c->has_error)
+ if(c->has_error) {
+ close_fds(fds, num_fds);
return 0;
+ }
assert(c != 0);
assert(vector != 0);
@@ -241,6 +244,7 @@ uint64_t xcb_send_request64(xcb_connection_t *c, int flags, struct iovec *vector
const xcb_query_extension_reply_t *extension = xcb_get_extension_data(c, req->ext);
if(!(extension && extension->present))
{
+ close_fds(fds, num_fds);
_xcb_conn_shutdown(c, XCB_CONN_CLOSED_EXT_NOTSUPPORTED);
return 0;
}
@@ -271,6 +275,7 @@ uint64_t xcb_send_request64(xcb_connection_t *c, int flags, struct iovec *vector
}
else if(longlen > xcb_get_maximum_request_length(c))
{
+ close_fds(fds, num_fds);
_xcb_conn_shutdown(c, XCB_CONN_CLOSED_REQ_LEN_EXCEED);
return 0; /* server can't take this; maybe need BIGREQUESTS? */
}
@@ -317,6 +322,7 @@ uint64_t xcb_send_request64(xcb_connection_t *c, int flags, struct iovec *vector
prepare_socket_request(c);
}
+ send_fds(c, fds, num_fds);
send_request(c, req->isvoid, workaround, flags, vector, veclen);
request = c->has_error ? 0 : c->out.request;
pthread_mutex_unlock(&c->iolock);
@@ -324,6 +330,18 @@ uint64_t xcb_send_request64(xcb_connection_t *c, int flags, struct iovec *vector
}
/* request number are actually uint64_t internally but keep API compat with unsigned int */
+unsigned int xcb_send_request_with_fds(xcb_connection_t *c, int flags, struct iovec *vector,
+ const xcb_protocol_request_t *req, unsigned int num_fds, int *fds)
+{
+ return xcb_send_request_with_fds64(c, flags, vector, req, num_fds, fds);
+}
+
+uint64_t xcb_send_request64(xcb_connection_t *c, int flags, struct iovec *vector, const xcb_protocol_request_t *req)
+{
+ return xcb_send_request_with_fds64(c, flags, vector, req, 0, NULL);
+}
+
+/* request number are actually uint64_t internally but keep API compat with unsigned int */
unsigned int xcb_send_request(xcb_connection_t *c, int flags, struct iovec *vector, const xcb_protocol_request_t *req)
{
return xcb_send_request64(c, flags, vector, req);
diff --git a/src/xcbext.h b/src/xcbext.h
index b2575f7..44d789e 100644
--- a/src/xcbext.h
+++ b/src/xcbext.h
@@ -68,13 +68,13 @@ enum xcb_send_request_flags_t {
*
* This function sends a new request to the X server. The data of the request is
* given as an array of @c iovecs in the @p vector argument. The length of that
- * array and the neccessary management information are given in the @p request
+ * array and the necessary management information are given in the @p request
* argument.
*
* When this function returns, the request might or might not be sent already.
* Use xcb_flush() to make sure that it really was sent.
*
- * Please note that this function is not the prefered way for sending requests.
+ * Please note that this function is not the preferred way for sending requests.
* It's better to use the generated wrapper functions.
*
* Please note that xcb might use index -1 and -2 of the @p vector array internally,
@@ -83,6 +83,37 @@ 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);
/**
+ * @brief Send a request to the server.
+ * @param c: The connection to the X server.
+ * @param flags: A combination of flags from the xcb_send_request_flags_t enumeration.
+ * @param vector: Data to send; must have two iovecs before start for internal use.
+ * @param request: Information about the request to be sent.
+ * @param num_fds: Number of additional file descriptors to send to the server
+ * @param fds: Additional file descriptors that should be send to the server.
+ * @return The request's sequence number on success, 0 otherwise.
+ *
+ * This function sends a new request to the X server. The data of the request is
+ * given as an array of @c iovecs in the @p vector argument. The length of that
+ * array and the necessary management information are given in the @p request
+ * argument.
+ *
+ * If @p num_fds is non-zero, @p fds points to an array of file descriptors that
+ * will be sent to the X server along with this request. After this function
+ * returns, all file descriptors sent are owned by xcb and will be closed
+ * eventually.
+ *
+ * When this function returns, the request might or might not be sent already.
+ * Use xcb_flush() to make sure that it really was sent.
+ *
+ * Please note that this function is not the preferred way for sending requests.
+ *
+ * Please note that xcb might use index -1 and -2 of the @p vector array internally,
+ * so they must be valid!
+ */
+unsigned int xcb_send_request_with_fds(xcb_connection_t *c, int flags, struct iovec *vector,
+ const xcb_protocol_request_t *request, unsigned int num_fds, int *fds);
+
+/**
* @brief Send a request to the server, with 64-bit sequence number returned.
* @param c: The connection to the X server.
* @param flags: A combination of flags from the xcb_send_request_flags_t enumeration.
@@ -92,13 +123,13 @@ unsigned int xcb_send_request(xcb_connection_t *c, int flags, struct iovec *vect
*
* This function sends a new request to the X server. The data of the request is
* given as an array of @c iovecs in the @p vector argument. The length of that
- * array and the neccessary management information are given in the @p request
+ * array and the necessary management information are given in the @p request
* argument.
*
* When this function returns, the request might or might not be sent already.
* Use xcb_flush() to make sure that it really was sent.
*
- * Please note that this function is not the prefered way for sending requests.
+ * Please note that this function is not the preferred way for sending requests.
* It's better to use the generated wrapper functions.
*
* Please note that xcb might use index -1 and -2 of the @p vector array internally,
@@ -107,6 +138,38 @@ unsigned int xcb_send_request(xcb_connection_t *c, int flags, struct iovec *vect
uint64_t xcb_send_request64(xcb_connection_t *c, int flags, struct iovec *vector, const xcb_protocol_request_t *request);
/**
+ * @brief Send a request to the server, with 64-bit sequence number returned.
+ * @param c: The connection to the X server.
+ * @param flags: A combination of flags from the xcb_send_request_flags_t enumeration.
+ * @param vector: Data to send; must have two iovecs before start for internal use.
+ * @param request: Information about the request to be sent.
+ * @param num_fds: Number of additional file descriptors to send to the server
+ * @param fds: Additional file descriptors that should be send to the server.
+ * @return The request's sequence number on success, 0 otherwise.
+ *
+ * This function sends a new request to the X server. The data of the request is
+ * given as an array of @c iovecs in the @p vector argument. The length of that
+ * array and the necessary management information are given in the @p request
+ * argument.
+ *
+ * If @p num_fds is non-zero, @p fds points to an array of file descriptors that
+ * will be sent to the X server along with this request. After this function
+ * returns, all file descriptors sent are owned by xcb and will be closed
+ * eventually.
+ *
+ * When this function returns, the request might or might not be sent already.
+ * Use xcb_flush() to make sure that it really was sent.
+ *
+ * Please note that this function is not the preferred way for sending requests.
+ * It's better to use the generated wrapper functions.
+ *
+ * Please note that xcb might use index -1 and -2 of the @p vector array internally,
+ * so they must be valid!
+ */
+uint64_t xcb_send_request_with_fds64(xcb_connection_t *c, int flags, struct iovec *vector,
+ const xcb_protocol_request_t *request, unsigned int num_fds, int *fds);
+
+/**
* @brief Send a file descriptor to the server in the next call to xcb_send_request.
* @param c: The connection to the X server.
* @param fd: The file descriptor to send.
@@ -114,9 +177,9 @@ uint64_t xcb_send_request64(xcb_connection_t *c, int flags, struct iovec *vector
* After this function returns, the file descriptor given is owned by xcb and
* will be closed eventually.
*
- * FIXME: How the heck is this supposed to work in a thread-safe way? There is a
- * race between two threads doing xcb_send_fd(); xcb_send_request(); at the same
- * time.
+ * @deprecated This function cannot be used in a thread-safe way. Two threads
+ * that run xcb_send_fd(); xcb_send_request(); could mix up their file
+ * descriptors. Instead, xcb_send_request_with_fds() should be used.
*/
void xcb_send_fd(xcb_connection_t *c, int fd);
commit cc04cfb41bece6ec239f57d83822286b507f4482
Author: Uli Schlachter <psychon at znc.in>
Date: Mon May 18 21:40:34 2015 +0200
send_fds(): Make sure no other thread interrupts us
Two threads trying to send fds at the same time could interfere. To guarantee a
correct ordering, we have to use correct locking. The code in send_fds() missed
one case: If there was another thread already writing requests, we slept on the
"done with writing" condition variable (c->out.cond). This would allow other
threads to re-acquire the iolock before us and could cause fds to be sent out of
order.
To fix this, at the beginning of send_fds() we now make sure that no other
thread is already writing requests. This is what prepare_socket_request() does.
Additionally, it gets the socket back in case xcb_take_socket() was called,
which is a good thing, too, since fds are only sent with corresponding requests.
diff --git a/src/xcb_out.c b/src/xcb_out.c
index 722463e..ec132fc 100644
--- a/src/xcb_out.c
+++ b/src/xcb_out.c
@@ -186,6 +186,15 @@ static void close_fds(int *fds, unsigned int num_fds)
static void send_fds(xcb_connection_t *c, int *fds, unsigned int num_fds)
{
#if HAVE_SENDMSG
+ /* Calling _xcb_out_flush_to() can drop the iolock and wait on a condition
+ * variable if another thread is currently writing (c->out.writing > 0).
+ * This call waits for writers to be done and thus _xcb_out_flush_to() will
+ * do the work itself (in which case we are a writer and
+ * prepare_socket_request() will wait for us to be done if another threads
+ * tries to send fds, too). Thanks to this, we can atomically write out FDs.
+ */
+ prepare_socket_request(c);
+
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 && !c->has_error) {
commit 25f9e7e45a7652b35b71c7941beef774a39f0d86
Author: Uli Schlachter <psychon at znc.in>
Date: Wed Apr 22 09:20:38 2015 +0200
xcb_send_fd(): Always close fds
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. We now also close fds if fd
passing is unavailable (!HAVE_SENDMSG) and when the connection is in an error
state.
(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>
diff --git a/src/xcb_out.c b/src/xcb_out.c
index 8cc5be8..722463e 100644
--- a/src/xcb_out.c
+++ b/src/xcb_out.c
@@ -177,6 +177,34 @@ uint32_t xcb_get_maximum_request_length(xcb_connection_t *c)
return c->out.maximum_request_length.value;
}
+static void close_fds(int *fds, unsigned int num_fds)
+{
+ for (unsigned int index = 0; index < num_fds; index++)
+ close(fds[index]);
+}
+
+static void send_fds(xcb_connection_t *c, int *fds, unsigned int num_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 && !c->has_error) {
+ /* XXX: if c->out.writing > 0, this releases the iolock and
+ * potentially allows other threads to interfere with their own fds.
+ */
+ _xcb_out_flush_to(c, c->out.request);
+ }
+ if (c->has_error)
+ break;
+
+ c->out.out_fd.fd[c->out.out_fd.nfd++] = fds[0];
+ fds++;
+ num_fds--;
+ }
+#endif
+ close_fds(fds, num_fds);
+}
+
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 +323,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, &fds[0], 1);
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)
More information about the xcb-commit
mailing list