[Xcb] [PATCH libxcb 4/4] send_fds(): Handle too many outstanding requests

[Uli Schlachter]

Hi,

Am 16.05.2015 um 16:16 schrieb Ran Benita:
> On Thu, May 14, 2015 at 09:55:35AM +0200, Uli Schlachter wrote:
>> 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 FD 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.
> 
> I think I'm really misunderstanding here, so feel free to ignore.
> But fds are associated with a specific request (and are sent as
> ancillary data with the request itself). So sending a *dummy* request to
> clear up the fds buffer seems really strange - what about the original
> request they were intended for?

When xcb_send_fd() was originally added, I asked something similar. Back then I
was told that the X11 server buffers FDs and uses them for the next request that
"needs" them. So sending FDs before the corresponding request is safe (but not
the other way around).

> I don't really understand why the fds array of a request can't just be
> kept along with it in whatever queue xcb keeps, then there's no need for
> synchronization. With your new combined send_request_with_fds(), it seems
> possible to make it so that the request and the fds flow together to the
> eventual sendmsg().

We don't immediately send every single request, but collect them in c->out.queue
until the buffer becomes full, so fds would need to be buffered as well. This is
just what the code does. It's just that "bytes" and "file descriptors" are
different things and belong into different buffers. :-)

> I guess xcb_send_fd() still needs to use the shared fds buffer by design,
> but can't it just be removed? After all we don't support using
> libxcb-foo.so without the exact matching version of libxcb.so. Or is it
> considered public API?

I guess removing it could mean that steam might break, because it likes to mix
and match versions of library. I guess we cannot remove it for ABI reasons. I'm
not sure about API.

Cheers,
Uli

> Ran
> 
>>
>> Signed-off-by: Uli Schlachter <psychon at znc.in>
>> ---
>>  src/xcb_out.c | 16 +++++++++++++---
>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/xcb_out.c b/src/xcb_out.c
>> index 06fae4c..37fd16d 100644
>> --- a/src/xcb_out.c
>> +++ b/src/xcb_out.c
>> @@ -187,11 +187,17 @@ static void send_fds(xcb_connection_t *c, unsigned int num_fds, int *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) {
>>              _xcb_out_flush_to(c, c->out.request);
>>              if (c->has_error)
>>                  break;
>> +
>> +            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;
>> +            }
>>          }
>>          if (!c->has_error)
>>              c->out.out_fd.fd[c->out.out_fd.nfd++] = fds[0];
>> @@ -296,6 +302,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, num_fds, fds);
>> +
>>      prepare_socket_request(c);
>>  
>>      /* send GetInputFocus (sync_req) when 64k-2 requests have been sent without
>> @@ -304,7 +315,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)
>>      {
>> @@ -312,7 +323,6 @@ uint64_t xcb_send_request_with_fds64(xcb_connection_t *c, int flags, struct iove
>>          prepare_socket_request(c);
>>      }
>>  
>> -    send_fds(c, num_fds, fds);
>>      send_request(c, req->isvoid, workaround, flags, vector, veclen);
>>      request = c->has_error ? 0 : c->out.request;
>>      pthread_mutex_unlock(&c->iolock);
>> -- 
>> 2.1.4
>>
>> _______________________________________________
>> Xcb mailing list
>> Xcb at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/xcb
> _______________________________________________
> Xcb mailing list
> Xcb at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/xcb
> 


-- 
- He made himself, me nothing, you nothing out of the dust
- Er machte sich mir nichts, dir nichts aus dem Staub