----- "Anthony Liguori" <anth...@codemonkey.ws> 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
> >
