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

[Alon Levy]

----- "Anthony Liguori" <address@hidden> wrote:

> 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.
> 
Ok, I don't see how it's that totally simple right now, but I'm going to
send a patch along those lines. Basically just adding a qemu_chr_server_write
to be used instead of the qemu_chr_write.

I think it would actually be simpler to add connect and disconnect to 
ChrDriverState
and have a guest_connect host_connected booleans (basically same as 
virtio-serial).
For the normal case (all existing backends) a connected bool would be 
initialized to 1,
and write would have an if (!connected) return, and connect/disconnect would be 
empty.
For spicevmc backend I would implement those as I would have the ioctl's / 
events.

> Regards,
> 
> Anthony Liguori
> 
> 
> >
> > cheers,
> >   Gerd
> >