[Spice-devel] [PATCH spice-server 3/3] red_worker: disconnect the channel instead of shutdown in case of a blocking method failure

[Marc-André Lureau]

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 at redhat.com> 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Marc-André Lureau