[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]

On Thu, Sep 26, 2013 at 4:55 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.

Sorry to ask, could you detail why "It is not safe to call
red_channel_client_disconnect from ChannelCbs"

I fail to understand directly why disconnect() would fix the crash in
#1004443, could you explain that too?

I understand that simply shutdown() might not be sufficient, since the
channel has still more a "connected" state, and will eventually call
disconnect() later, but we might have bad condition checks that lead
to the crash, so I can imagine why your solution would work, it's just
not obvious :)

thanks

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