[Spice-devel] agent channel behavior in case of unexpected client / agent close

[Alon Levy]

On Thu, Jul 21, 2011 at 02:16:06PM +0200, Hans de Goede wrote:
> Hi All,
> 
> I would like to add support for the state callback of SpiceCharDeviceInterface
> to spice-qemu-char.c, because I need it for ubsredir support.
> 
> There already is code in spice-server which will call this if defined, and
> this code lives in reds.c and has to do with the agent channel.
> 
> This has let me to analyze what exactly happens if the guest agent, or
> the client unexpectedly (iow in the middle of a read / write) exit. Yes
> I've written a similar mail before, but with the agent msg filter things
> have changed, and some corner-cases which were not handled before are
> handled now.
> 
> Before starting my analysis first a short overview of the agent channel,
> since the agent channel is a somewhat complicated beast. We basically
> have 2 separate connections with the server in the middle:
> 
> agent <-> server <-> client
> 
> And the server does more then just dumb forwarding (which is my plan
> for usbredir), it actually parses the messages being send, and adds /
> removes an additional header on the server agent link:
> 
> agent <------------> server <-------------------> client
>             |                         |
>        vdichunk hdr +            vdagent hdr +
>        vdagent hdr +             payload
>        payload
> 
> The reason for this is that the server also generates its own
> vdagent messages, for mouse events and multiplexes these into
> the stream from the client, so logically the view is:
> 
>       | <-- server messages <--|
> agent-|                        |-server <---> client
>       | <-- client messages -->|
> 
> 

Nice ascii art.

> So now lets analyze the 4 possible corner cases:
> 
> 1) The agent disconnects halfway through sending a message:
> The server gets notified of this, and resets its own parsers
> (both at the vdichunk and vdagent proto level) so that they
> are in a clean state if the agent re-connects. The server also
> sends a message to the client, which in turn should reset
> its vdagent parser state (I've not checked it actually does
> this) -> All good.
> 
> 2) The agents connects halfway through reading a message:
> The server will discard any queued writes, and in case the
> client was halfway through sending a message, it will discard
> any messages for the agent from the client too -> All good.
> 
> 3) The client disconnects halfway through sending a message.
> The agent will see an incomplete message, and if another client
> connects the first data it will send will get seen as the rest
> of the previous message -> Not good!
> 
> 4) The client disconnects halfway through receiving a message.
> In this case the server will keep consuming the rest of the
> message from the agent and simply discard it -> All good.
> 
> 
> So only scenario 3 is a problem, if we add support for the
> state callback of SpiceCharDeviceInterface and make that
> generate qemu chardev open/close events, things change though:
> 
> 1 and 2 are unchanged.
> 
> 3) Is unchanged, but at least the Linux agent can be modified
> to receive a notification of the close event, and then reset
> its parser fixing the potential issues caused by this scenario.
> 
> 4) If we send a close event to the chardev, then the virtio
> serial "stack" will discard any pending and future write, causing
> the parser in the server to get out of sync with the agent -> not
> good (TM)
> 
> Also if we send a close event from qemu-char-dev to the virtio port,
> then any reads (or select for read) done by the agent will no longer
> block, but instead return immediately with a count of 0, causing
> the agent to go into a 100% cpu usage loop.
> 
> So all in all enabling open / close event for the agent channel
> is a BAD (tm) idea. However as stated in the start for usbredir
> actually getting client connect / disconnect events is desirable,
> so that usbredir can reset its parser to start fresh with the
> new client. And so that ic can "unplug" the redirected usbdevice
> from the vm when the client disconnects, since that is what
> essentially happens then.
> 
> Thus I'm going to send a patch soon, which adds a
> state callback to the SpiceCharDeviceInterface of spice-qemu-char,
> but turns this into a no-op for anything but usbredir. Alternatively
> we could make it a no-op only for vdagent type spicevmc chardevs,
> but that requires an analysis like the one above to be done
> for the smartcard stuff.

Since I'm not going to do that check, I think your suggestion sounds
fine. I think the vdagent code could use some cleanup too, but that's
a different subject (I'm looking at agent_state.connected).

> 
> Regards,
> 
> Hans
> 
> 
> p.s.
> 
> One idea to fix scenario 3 is to add a new vdagent message
> type which tells the agent the client has disconnected, which
> gets send by the server, and thus through the "server" channel
> of the muxed client/server connection between the server and
> client, the agent can then reset the client vdagent message
> parsing part. To ensure compatibility with older agents an
> capability should be added for this and the server should only
> send this message to agents claiming this capability. I will
> happily review patches for this (hint hint). If no one else
> gets around to this I'll put it on my to do list.

.. hope you throttle your to do list.

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