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@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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel