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

[Yonit Halperin]

Thanks for the ack, but I'll try to convince you anyway :)

The crash details are:

kvm:14005): SpiceWorker-Warning **: 
red_worker.c:11009:red_wait_outgoing_item: timeout
09/03 15:22:26 INFO |   aexpect:0816| [qemu output] 
(/usr/libexec/qemu-kvm:14005): SpiceWorker-ERROR **: 
red_worker.c:11473:dev_destroy_primary_surface: assertion 
`!worker->surfaces[surface_id].context.canvas' failed

- red_wait_outgoing_item was triggered from destroy_surface_wait.
- When destroy_surface_wait timeout-ed, we called 
red_channel_client_shutdown.
- The outgoing pipe item was still alive.
- This pipe-item held a reference to the surface, and the
   surface wasn't destroyed (i.e., 
worker->surfaces[surface_id].context.canvas != NULL).

If we s/red_channel_client_shutdown/red_channel_client_disconnect, this
wouldn't have happened, because red_channel_client_disconnect would 
release the outgoing pipe item.

Hope this makes it clearer
Yonit.
On 09/26/2013 02:10 PM, Marc-André Lureau wrote:
> 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
>
>
>