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

[Hans de Goede]

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


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.

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.