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

[Alexander Larsson]

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_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?