On 08/03/2010 03:46 AM, Gerd Hoffmann wrote:
Hi,
My main objection to ioctls is that you change states based on event
delivery. This results in weird things like what happens when you do a
chr_write while not ready or not connected.
So what I'd rather see is a move to an API that was connection oriented.
For instance, we could treat CharDriverState as an established
connection. So something like:
typedef struct CharServerState
{
int backlog; /* max simultaneous connections; -1 for unlimited */
void (*connect)(CharServerState *s, CharDriverState *session);
void (*disconnect)(CharServerState *s, CharDriverState *session);
} CharDriverState;
Oh, that is a similar but unrelated issue.
We have open/close events on the *guest* side (i.e. some process
inside the guests opens/closes /dev/vmchannel/org.qemu.foo.42). This
is what Alon wants to propagate from the device backend to the chardev.
We also have open/close (or connect/disconnect) events on the *host*
side for the devices (or sockets) the chardevs are bound to. This is
what you are talking about.
No, I'm not. You have a front-end device that's connected to
virtio-serial. You're implementing the backend in spice. The
front-end needs to communicate to the backend events like connect,
ready, disconnect. I don't see what the difference between connect and
ready is so I'll ignore it for now.
The proposal is to implement this via events. My concern is that this
interface is brittle because it leaves a lot of behavior undefined.
There are three distinct states in the life cycle, DISCONNECTED,
CONNECTED_BUT_NOT_READY, and CONNECTED_AND_READY. The entire
CharDriverState interface is only useful in the CONNECTED_AND_READY
state so what's the behavior of every function in any of the other states?
My suggestion is to implement a simple CharServerState driver. This
interface is connection oriented. You can have a dummy CharServerState
that returns a single CharDriverState on connect() and does nothing on
disconnect(). That's how you bridge virtio-serial to what we have
today. But the idea is that virtio-serial no longer takes a
CharDriverState but a CharServerState.
Spice would then implement it's own CharServerState and would use it to
understand what state the session is in. It's a really simple interface
yet it makes the code much more robust because it eliminates the entire
class of errors associated with undefined behavior when state !=
CONNECTED_AND_READY.
The problem we've had with host side state is poorly defined semantics.
For instance, I still think we generate multiple OPENED events as
opposed to strictly generating CLOSED, followed by OPENED, followed by
CLOSED.
Note that we already have events (CHR_EVENT_OPENED,CLOSED) for the
host side. Adding events for the guest side open/close events makes
sense to me (and is certainly better than the ioctl patch).
We have the same problem with host side events today but it's even worse
because the semantics are very subtle. Ultimately we need something
like CharServerState and we could probably even use it but that's a
larger scope than just this patch.
The reason I think it's worth doing it this way is that I anticipate
future virtio-serial backends in QEMU. It's a very simple difference too.
Regards,
Anthony Liguori
cheers,
Gerd
