[Spice-devel] [PATCH] server/reds: allow call to reds_agent_remove even if it is gone

[Alon Levy]

On Sun, Mar 06, 2011 at 12:00:07PM +0200, Uri Lublin wrote:
> On 03/04/2011 01:52 PM, Alon Levy wrote:
> >The current assert(reds->agent_state.connected) tiggers if when
> >the agent disconnected there was still data waiting to be sent (for
> >instance if there is a bug in the client handling clipboard and it
> >is killed while a large clipboard transfer is in progress). So first
> >call to reds_agent_remove happens from spice_server_char_device_remove_interface,
> >and then it is called again (triggering the assert) from free_item_data
> >from read_from_vdi_port because of the channel destruction.
> >
> >Other option would be to not call it from one of the paths - but that
> >is suboptimal:
> >  * if there is no data in the pipe, the second call never happens.
> >  * the second call has to be there anyway, because it may fail during
> >   parsing data from the agent.
> >
> >This patch fixes a segfault on this assert when a client starts passing
> >from guest agent to client a large clipboard and dies in the middle. There
> >is still another assert happening occasionally at marshaller which I don't
> >have a fix for (but it doesn't seem to be related).
> >---
> >  server/reds.c |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> >
> >diff --git a/server/reds.c b/server/reds.c
> >index 750b785..c1873ef 100644
> >--- a/server/reds.c
> >+++ b/server/reds.c
> >@@ -699,7 +699,9 @@ static void reds_agent_remove()
> >      SpiceCharDeviceInstance *sin = vdagent;
> >      SpiceCharDeviceInterface *sif;
> >
> >-    ASSERT(reds->agent_state.connected)
> >+    if (!reds->agent_state.connected) {
> >+        return;
> >+    }
> >      reds->agent_state.connected = 0;
> >      vdagent = NULL;
> >      reds_update_mouse_mode();
> 
> 
> Maybe we should not allow to disconnect, until all packets are flushed.
> Or alternatively, we need to cleanup the queue (which with this
> patch stays "dirty").

The queue seems to be cleared at the end of reds_agent_remove:
reds_reset_vdp();

reds_reset_vdp frees all items in the write_queue.

The problem is that after this clear the guest sends some new data, which
fails to be sent, resulting in a call to reds_agent_remove, resulting in
the assert.

I don't think we want to disconnect the agent when the client disconnects,
right? we should perhaps send a message to the agent telling it the client
disconnected, but then it would race with the data the guest is still sending
(the remains of the clipboard in this case).

> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel