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

[Gerd Hoffmann <kraxel@redhat.com>]

On 08/03/10 15:12, Anthony Liguori 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.

/me wonders what the point of the 'backlog' struct element is then.

That made me think this is intended for the host side as it would be 
useful there.  Monitor sockets could allow more than one connect then.

> 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.

That happens already.  Guest opens device, virtio-serial receives a 
control message and calls port->info->guest_open().  Likewise on close.

> The proposal is to implement this via events.

Basically forwarding the events virtio-serial provides to the linked 
chardev, yes.

> 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?

Most chardev backends don't care anyway.
In case they do it is up to them to define behavior when closed IMHO.

> 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.

Yes, we can do that.  I don't think it is useful.  Oh, and it also 
changes the command line interface.  Todays ...

    qemu -chardev soemthing,id=foo \
         -device virtserport,chardev=foo

... would turn into something like ...

   qemu -chardev something,if=foo \
        -charsrv simple,chardev=foo,id=bar \
        -device virtserport,charsrv=bar

> Spice would then implement it's own CharServerState and would use it to
> understand what state the session is in.

Spice would basically (ab-)use it as event delivery mechanism.

> 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.

Well.  I disagree.  Checking the state is needed nevertheless.  The 
places where virtio-serial checks port->state today it would have to 
check whenever port->chardev is non-NULL then.  The only difference is 
that failures to do so might become a bit more obvious as qemu will 
segfault due to the NULL pointer dereferences then.  I still think this 
isn't worth the effort though.

> 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.

Lets add assert()s (after 0.13-release) to catch those cases.

cheers,
   Gerd