Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole

[Anthony Liguori <anthony@codemonkey.ws>]

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
>