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 > [email protected] > http://lists.freedesktop.org/mailman/listinfo/spice-devel _______________________________________________ Spice-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/spice-devel
