[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole
From: |
Anthony Liguori |
Subject: |
Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole |
Date: |
Tue, 03 Aug 2010 10:46:24 -0500 |
User-agent: |
Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.11) Gecko/20100713 Lightning/1.0b1 Thunderbird/3.0.6 |
On 08/03/2010 10:28 AM, Gerd Hoffmann wrote:
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.
Because it could be used for host event but let's ignore that for now.
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.
But is there really any reasonable way to define it? Wouldn't it be
better to just prevent the operations in the first place and basically
push back to the front-ends?
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
If we think this is useful, then we can find a way to make the command
line syntax work. If we don't think it's useful, then there's no point
in doing it.
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.
Can you explain what spice uses these events for?
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.
But I think we ultimately need to switch to having the front-ends having
a NULL check. Even beyond front-end initiated connect/disconnect,
front-end's need to learn to deal with back-end initiated
disconnect/connect.
If you look at something like the serial device's chardev usage, right
now, it writes to the stream regardless of whether the back-end is
connected and we have different semantics in each backend about what we
do. It would be far better to just expose the fact that the backend
isn't connected to the device such that it can either present that to
the guest or make it's own decision about what to do.
I think the model where we always write to a chardev is fundamentally
broken. Sending life cycle events over an always open stream is even
more broken and I think it's a good opportunity to introduce life cycle
awareness into the API (especially since it can be done as a pretty
small incremental change).
Regards,
Anthony Liguori
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
- [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole, Alon Levy, 2010/08/02
- Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole, Alon Levy, 2010/08/02
- Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole, Anthony Liguori, 2010/08/02
- Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole, Gerd Hoffmann, 2010/08/03
- Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole, Anthony Liguori, 2010/08/03
- Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole, Gerd Hoffmann, 2010/08/03
- Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole,
Anthony Liguori <=
- Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole, Gerd Hoffmann, 2010/08/03
- Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole, Anthony Liguori, 2010/08/03
- Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole, Gerd Hoffmann, 2010/08/03
- Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole, Anthony Liguori, 2010/08/03
- Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole, Gerd Hoffmann, 2010/08/03
- Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole, Anthony Liguori, 2010/08/03
Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole, Alon Levy, 2010/08/03