[Spice-devel] [PATCH 2/3] server: really wait for a surface to be destroyed, when calling destroy_surface_wait

[Yonit Halperin]

On 08/26/2010 05:25 PM, Alexander Larsson wrote:
> On Thu, 2010-08-26 at 12:41 +0300, Yonit Halperin wrote:
>> Waiting till all the pipe items that are dependent on the surface will be sent.
>> This was probably the cause for freedesktop bug #29750.
>> ---
>>   server/red_worker.c |   84 +++++++++++++++++++++++++++++++++++++++++++++-----
>>   1 files changed, 75 insertions(+), 9 deletions(-)
>>
>> diff --git a/server/red_worker.c b/server/red_worker.c
>> index 1a3f755..e688971 100644
>> --- a/server/red_worker.c
>> +++ b/server/red_worker.c
>
>> +    red_printf("");
>
> Leftover?
>
>> +    red_ref_channel(channel);
>> +    channel->hold_item(item);
>> +
>> +    end_time = red_now() + CHANNEL_PUSH_TIMEOUT;
>> +
>> +    if (channel->send_data.blocked) {
>> +        red_receive(channel);
>> +        red_send_data(channel, NULL);
>> +    }
>> +    // todo: different push for each channel
>> +    red_push(channel->worker);
>
> Why do this outside the loop, and why the check on send_data.blocked. Is
> it not better to start with red_push (which will do nothing if
> send_data.blocked), then receive+send, always in the loop. Then you can
> something loop like: push, receive, send, if blocked and item_in_pipe
> usleep.
>
>> +
>> +    while((item_in_pipe = ring_item_is_linked(&item->link))&&  (red_now()<  end_time)) {
>> +        usleep(CHANNEL_PUSH_SLEEP_DURATION);
>
> Why are you sleeping between each item sent? We should only need to
> sleep when channel->send_data.blocked is true.
>
red_push doesn't send one item at a time, but rather pushes till it 
blocks (due to socket block or missing acks). So if the item is still in 
the pipe, it means that channel->send_data.blocked
>> +        red_receive(channel);
>> +        red_send_data(channel, NULL);
>> +        red_push(channel->worker);
>> +    }
>
> Also, I'm generally worried a bit about the red_receive calls. While
> waiting for sending the stuff in the pipe, handling the
> destroy_surface_wait we may handle an incoming message from the client
> which sort of reenters the existing call stack. Isn't there a risk that
> such reentrancy causes problems? like deadlocks or weird crashes?
>
>
First, we must call red_receive, since if we are missing acks, we won't 
get out of the blocking mode. Second, I don't think that any message 
from the client can cause re-entering to this call stack.