don't flag extra reply in xcb_take_socket

Submitted by Erik Kurzinger on Aug. 20, 2018, 7:06 p.m.

Details

Message ID ab9adc9d901a47a380eabb3a6e97a6cd@HQMAIL108.nvidia.com
State New
Series "don't flag extra reply in xcb_take_socket"
Headers show

Commit Message

Erik Kurzinger Aug. 20, 2018, 7:06 p.m.
Hi Uli,

Thanks for taking a look! I tried modifying the 'else' case in _xcb_in_replies_done
as you suggested, I agree that it's a bit cleaner than what I had. 

It still appears to fix the hang in the example program as well as the KWin crash
I had mentioned so everything looks good. I can't think of a better name than 
'prev_next' either - it'll have to do :)

Cheers,
Erik

---
 src/xcb_in.c  | 16 ++++++++++++++--
 src/xcb_out.c | 10 ++++++++--
 2 files changed, 22 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/xcb_in.c b/src/xcb_in.c
index 73209e0..58fe896 100644
--- a/src/xcb_in.c
+++ b/src/xcb_in.c
@@ -958,8 +958,20 @@  void _xcb_in_replies_done(xcb_connection_t *c)
         pend = container_of(c->in.pending_replies_tail, struct pending_reply, next);
         if(pend->workaround == WORKAROUND_EXTERNAL_SOCKET_OWNER)
         {
-            pend->last_request = c->out.request;
-            pend->workaround = WORKAROUND_NONE;
+            if (XCB_SEQUENCE_COMPARE(pend->first_request, <=, c->out.request)) {
+                pend->last_request = c->out.request;
+                pend->workaround = WORKAROUND_NONE;
+            } else {
+                /* The socket was taken, but no requests were actually sent
+                 * so just discard the pending_reply that was created.
+                 */
+                struct pending_reply **prev_next = &c->in.pending_replies;
+                while (*prev_next != pend)
+                    prev_next = &(*prev_next)->next;
+                *prev_next = NULL;
+                c->in.pending_replies_tail = prev_next;
+                free(pend);
+            }
         }
     }
 }
diff --git a/src/xcb_out.c b/src/xcb_out.c
index 3601a5f..c9593e5 100644
--- a/src/xcb_out.c
+++ b/src/xcb_out.c
@@ -387,8 +387,14 @@  int xcb_take_socket(xcb_connection_t *c, void (*return_socket)(void *closure), v
     {
         c->out.return_socket = return_socket;
         c->out.socket_closure = closure;
-        if(flags)
-            _xcb_in_expect_reply(c, c->out.request, WORKAROUND_EXTERNAL_SOCKET_OWNER, flags);
+        if(flags) {
+            /* c->out.request + 1 will be the first request sent by the external
+             * socket owner. If the socket is returned before this request is sent
+             * it will be detected in _xcb_in_replies_done and this pending_reply
+             * will be discarded.
+             */
+            _xcb_in_expect_reply(c, c->out.request + 1, WORKAROUND_EXTERNAL_SOCKET_OWNER, flags);
+        }
         assert(c->out.request == c->out.request_written);
         *sent = c->out.request;
     }

Comments

Uli Schlachter Aug. 21, 2018, 5:02 p.m.
Thanks. I took the commit message from your original mail, added it to
this patch and pushed the result.

Cheers,
Uli

P.S.: And now I'll wait for someone to ask for an 1.13.1 release. Plus
someone who volunteers to do that release.

On 20.08.2018 21:06, Erik Kurzinger wrote:
> Hi Uli,
> 
> Thanks for taking a look! I tried modifying the 'else' case in _xcb_in_replies_done
> as you suggested, I agree that it's a bit cleaner than what I had. 
> 
> It still appears to fix the hang in the example program as well as the KWin crash
> I had mentioned so everything looks good. I can't think of a better name than 
> 'prev_next' either - it'll have to do :)
> 
> Cheers,
> Erik
> 
> ---
>  src/xcb_in.c  | 16 ++++++++++++++--
>  src/xcb_out.c | 10 ++++++++--
>  2 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/src/xcb_in.c b/src/xcb_in.c
> index 73209e0..58fe896 100644
> --- a/src/xcb_in.c
> +++ b/src/xcb_in.c
> @@ -958,8 +958,20 @@ void _xcb_in_replies_done(xcb_connection_t *c)
>          pend = container_of(c->in.pending_replies_tail, struct pending_reply, next);
>          if(pend->workaround == WORKAROUND_EXTERNAL_SOCKET_OWNER)
>          {
> -            pend->last_request = c->out.request;
> -            pend->workaround = WORKAROUND_NONE;
> +            if (XCB_SEQUENCE_COMPARE(pend->first_request, <=, c->out.request)) {
> +                pend->last_request = c->out.request;
> +                pend->workaround = WORKAROUND_NONE;
> +            } else {
> +                /* The socket was taken, but no requests were actually sent
> +                 * so just discard the pending_reply that was created.
> +                 */
> +                struct pending_reply **prev_next = &c->in.pending_replies;
> +                while (*prev_next != pend)
> +                    prev_next = &(*prev_next)->next;
> +                *prev_next = NULL;
> +                c->in.pending_replies_tail = prev_next;
> +                free(pend);
> +            }
>          }
>      }
>  }
> diff --git a/src/xcb_out.c b/src/xcb_out.c
> index 3601a5f..c9593e5 100644
> --- a/src/xcb_out.c
> +++ b/src/xcb_out.c
> @@ -387,8 +387,14 @@ int xcb_take_socket(xcb_connection_t *c, void (*return_socket)(void *closure), v
>      {
>          c->out.return_socket = return_socket;
>          c->out.socket_closure = closure;
> -        if(flags)
> -            _xcb_in_expect_reply(c, c->out.request, WORKAROUND_EXTERNAL_SOCKET_OWNER, flags);
> +        if(flags) {
> +            /* c->out.request + 1 will be the first request sent by the external
> +             * socket owner. If the socket is returned before this request is sent
> +             * it will be detected in _xcb_in_replies_done and this pending_reply
> +             * will be discarded.
> +             */
> +            _xcb_in_expect_reply(c, c->out.request + 1, WORKAROUND_EXTERNAL_SOCKET_OWNER, flags);
> +        }
>          assert(c->out.request == c->out.request_written);
>          *sent = c->out.request;
>      }
>
Uli Schlachter Sept. 16, 2018, 4:39 p.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 21.08.2018 19:02, Uli Schlachter wrote:
> P.S.: And now I'll wait for someone to ask for an 1.13.1 release.
> Plus someone who volunteers to do that release.

Sigh. No one took my bait.

Okay. I do not know when I will have the time to this, but would
anyone mind if I do an 1.13.1 release based on current master of
libxcb? That would just be 1.13 plus this one commit.

If no one speaks up, I will just do the release when I find the time
for it.

Cheers,
Uli
- -- 
99 little bugs in the code
99 little bugs in the code
Take one down, patch it around
117 little bugs in the code
  -- @irqed
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCgAdFiEEK7MviP89HnbmgjA/IuQo68uPywYFAluehz8ACgkQIuQo68uP
ywYyxggAsAXGKudVvCOpX318VmazqEdIc0/yXHxyPo/S2wrZGqjGdGXnVPTPvYC5
PdP8LHg0GWFIbqHNR0NrKlPY4C++ST3+L2I/93gdVPKLJakWgE3uqLcJkRDlJZBh
uobjK6ZXmTg+x9mAuxL3zRZDVeh92YeJ7fn8mGZnlvhvV8nM3oBVYFGhqhNn5Z8E
WibFnFpNxoAIqO19kzEv8ZQf+aP1QWTAZCC8chMm4zb0FIuw/Fyd47zKloWjF4dI
M8W2IjL6vEHazd5vXd2cpp4dkqw6h3V8eItGayAlwQDAQEtFIZ1S1ojmwYMQghQq
Y2drAxfaaaPqybD432yZ6XoR8p0GIw==
=H1d4
-----END PGP SIGNATURE-----