Re: [PATCH xcb] don't flag extra reply in xcb_take_socket

On 18.08.2018 09:47, Uli Schlachter wrote:
[...]
> Thanks for figuring this out. Still, I'm slightly confused about this. I
> added another GetInputFocus request after the xcb_take_socket(). If I
> get the reply for this second request first, everything works fine. If I
> get the reply for this second request second, getting the first reply
> still hangs. (See attached file)

I figured this one out: There is no hang, because when XCB gets a reply
with sequence number 2, it knows for sure there will be no reply with
sequence number 1 and returns NULL. Since the test case did not actually
print anything about the reply, this was hidden.

> (Also, I patched my local xcb with just your change to
> xcb_take_socket(), expecting this to cause the first request after
> taking the socket to be discarded, but this did not happen either)

The above creates a pending reply with first_request = 2 and
last_request = 1. This last_request = 1 causes the pending reply to be
removed in read_packet() because it is considered "too old".

Also, I thought a bit about this and think that your patch is the best
possible approach, so:

Reviewed-By: Uli Schlachter <[email protected]>

However, I need a mail with a non-forwarded version of this patch:

> $ LC_ALL=C git am -s /tmp/mail
> Patch is empty.
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
Thus, I feel like I can also propose a tiny improvement to the patch:

Can you remove the code in the new "else"-branch of
_xcb_in_replies_done() with the following (and check if it works)?

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

Your version of this code keeps a pointer to the previous pending_reply
and has a special case when the to-be-removed pending_reply is the first
entry of the list.

The code above instead only tracks the address of the ->next field of
the previous pending_reply, thus allowing to get rid of the special case
with removing the first entry of the list.

[Feel free to rename prev_next to something good, this is the best that
I came up with]

Cheers,
Uli
-- 
Bruce Schneier can read and understand Perl programs.
_______________________________________________
Xcb mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/xcb