Ok, I still don't understand clearly how this avoids the crash in rhbz#1004443
But I am ready to trust you on this one... So if nobody else shimes
here, consider this as ack :)
On Thu, Sep 26, 2013 at 7:59 PM, Yonit Halperin <yhalperi@xxxxxxxxxx> wrote:
> rhbz#1004443
>
> The methods that trigger waitings on the client pipe require that
> the waiting will succeed in order to continue, or otherwise, that
> all the living pipe items will be released (e.g., when
> we must destroy a surface, we need that all its related pipe items will
> be released). Shutdown of the socket will eventually trigger
> red_channel_client_disconnect (*), which will empty the pipe. However,
> if the blocking method failed, we need to empty the pipe synchronously.
> It is not safe(**) to call red_channel_client_disconnect from ChannelCbs
> , but all the blocking calls in red_worker are done from callbacks that
> are triggered from the device.
> To summarize, calling red_channel_client_disconnect instead of calling
> red_channel_client_shutdown will immediately release all the pipe items that are
> held by the channel client (by calling red_channel_client_pipe_clear).
> If red_clear_surface_drawables_from_pipe timeouts,
> red_channel_client_disconnect will make sure that the surface we wish to
> release is not referenced by any pipe-item.
>
> (*) After a shutdown of a socket, we expect that later, when
> red_peer_handle_incoming is called, it will encounter a socket
> error and will call the channel's on_error callback which calls
> red_channel_client_disconnect.
>
> (**) I believe it was not safe before commit 2d2121a17038bc0 (before adding ref
> count to ChannelClient). However, I think it might still be unsafe, because
> red_channel_client_disconnect sets rcc->stream to NULL, and rcc->stream
> may be referred later inside a red_channel_client method unsafely. So instead
> of checking if (stream != NULL) after calling callbacks, we try to avoid
> calling red_channel_client_disconnect from callbacks.
> ---
> server/red_worker.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/server/red_worker.c b/server/red_worker.c
> index dea7325..1f67212 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -10959,10 +10959,10 @@ void handle_dev_destroy_surface_wait(void *opaque, void *payload)
> dev_destroy_surface_wait(worker, msg->surface_id);
> }
>
> -static void rcc_shutdown_if_pending_send(RedChannelClient *rcc)
> +static void rcc_disconnect_if_pending_send(RedChannelClient *rcc)
> {
> if (red_channel_client_blocked(rcc) || rcc->pipe_size > 0) {
> - red_channel_client_shutdown(rcc);
> + red_channel_client_disconnect(rcc);
> } else {
> spice_assert(red_channel_client_no_item_being_sent(rcc));
> }
> @@ -10988,7 +10988,7 @@ static inline void red_cursor_reset(RedWorker *worker)
> if (!red_channel_wait_all_sent(&worker->cursor_channel->common.base,
> DISPLAY_CLIENT_TIMEOUT)) {
> red_channel_apply_clients(&worker->cursor_channel->common.base,
> - rcc_shutdown_if_pending_send);
> + rcc_disconnect_if_pending_send);
> }
> }
> }
> @@ -11275,12 +11275,12 @@ void handle_dev_stop(void *opaque, void *payload)
> if (!red_channel_wait_all_sent(&worker->display_channel->common.base,
> DISPLAY_CLIENT_TIMEOUT)) {
> red_channel_apply_clients(&worker->display_channel->common.base,
> - rcc_shutdown_if_pending_send);
> + rcc_disconnect_if_pending_send);
> }
> if (!red_channel_wait_all_sent(&worker->cursor_channel->common.base,
> DISPLAY_CLIENT_TIMEOUT)) {
> red_channel_apply_clients(&worker->cursor_channel->common.base,
> - rcc_shutdown_if_pending_send);
> + rcc_disconnect_if_pending_send);
> }
> }
>
> --
> 1.8.1.4
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
--
Marc-André Lureau
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel