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

[Anthony Liguori <anthony@codemonkey.ws>]

On 08/02/2010 12:28 PM, Alon Levy wrote:
> ----- "Anthony Liguori"<anthony@codemonkey.ws>  wrote:
>
>    
>> On 08/02/2010 03:33 AM, Alon Levy wrote:
>>      
>>> Hi,
>>>
>>>    This patch adds three CHR_IOCTLs and uses them in virtserial
>>>        
>> devices, to be used
>>      
>>> by a chardev backend, such as a spice vm channel (spice is a vdi
>>>        
>> solution).
>>      
>>>    Basically virtio-serial provides three driver initiated events for
>>>        
>> guest open of
>>      
>>> a device, guest close, and guest ready (driver port init complete)
>>>        
>> that before this
>>      
>>> patch are not exposed to the chardev backend.
>>>
>>>    With the spicevmc backend this is used like this:
>>> qemu -chardev spicevmc,id=vdiport,name=vdiport -device
>>>        
>> virtserialport,chardev=vdiport,name=com.redhat.spice.0
>>      
>>>    I'd appreciate any feedback if this seems the right way to
>>>        
>> accomplish this, and
>>      
>>> for the numbers I grabbed.
>>>
>>>        
>> I really hate to add connection semantics via IOCTLs.  I would rather
>> we
>> add them as first class semantics to the char device layer.  This
>> would
>> allow us to use char devices for VNC, for instance.
>>
>>      
> Ok, that's actually what I wanted to do at first, how about:
>    

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;

Obviously, more thought is needed but I hope the point comes across.  We 
should be able to reflect the connect/disconnect semantics with an 
object that has a life cycle that matches the session instead of forcing 
each user to keep track of the session's life cycle.

Regards,

Anthony Liguori

> diff --git a/qemu-char.h b/qemu-char.h
> index 1df53ae..22973cd 100644
> --- a/qemu-char.h
> +++ b/qemu-char.h
> @@ -16,6 +16,8 @@
>   #define CHR_EVENT_MUX_OUT 4 /* mux-focus will move on */
>   #define CHR_EVENT_CLOSED  5 /* connection closed */
>
> +#define CHR_DEVICE_EVENT_OPENED 0
> +#define CHR_DEVICE_EVENT_CLOSED 1
>
>   #define CHR_IOCTL_SERIAL_SET_PARAMS   1
>   typedef struct {
> @@ -59,6 +61,7 @@ struct CharDriverState {
>       int (*chr_write)(struct CharDriverState *s, const uint8_t *buf, int len);
>       void (*chr_update_read_handler)(struct CharDriverState *s);
>       int (*chr_ioctl)(struct CharDriverState *s, int cmd, void *arg);
> +    int (*chr_device_event)(struct CharDriverState *s, int cmd, void *arg);
>       int (*get_msgfd)(struct CharDriverState *s);
>       IOEventHandler *chr_event;
>       IOCanReadHandler *chr_can_read;
> @@ -83,6 +86,7 @@ void qemu_chr_close(CharDriverState *chr);
>   void qemu_chr_printf(CharDriverState *s, const char *fmt, ...);
>   int qemu_chr_write(CharDriverState *s, const uint8_t *buf, int len);
>   void qemu_chr_send_event(CharDriverState *s, int event);
> +void qemu_chr_send_device_event(CharDriverState *s, int event, void *arg);
>   void qemu_chr_add_handlers(CharDriverState *s,
>                              IOCanReadHandler *fd_can_read,
>                              IOReadHandler *fd_read,
>
>
>    
>> Regards,
>>
>> Anthony Liguori
>>
>>      
>>> Alon
>>>
>>> -------------- commit message --------------------------------
>>>   From a90d4e26df727ed0d2b64b705e955f695289fa61 Mon Sep 17 00:00:00
>>>        
>> 2001
>>      
>>> From: Alon Levy<alevy@redhat.com>
>>> Date: Mon, 2 Aug 2010 11:22:58 +0300
>>> Subject: [PATCH] virtio-console: add IOCTL's for
>>>        
>> guest_{ready,open,close}
>>      
>>> Add three IOCTL corresponding to the three control events of:
>>>    guest_ready ->   CHR_IOCTL_VIRT_SERIAL_READY
>>>    guest_open  ->   CHR_IOCTL_VIRT_SERIAL_OPEN
>>>    guest_close ->   CHR_IOCTL_VIRT_SERIAL_CLOSE
>>>
>>> Can be used by a matching backend.
>>> ---
>>>    hw/virtio-console.c |   33 +++++++++++++++++++++++++++++++++
>>>    qemu-char.h         |    4 ++++
>>>    2 files changed, 37 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/virtio-console.c b/hw/virtio-console.c
>>> index caea11f..4c3686d 100644
>>> --- a/hw/virtio-console.c
>>> +++ b/hw/virtio-console.c
>>> @@ -58,6 +58,33 @@ static void chr_event(void *opaque, int event)
>>>        }
>>>    }
>>>
>>> +static void virtconsole_guest_open(VirtIOSerialPort *port)
>>> +{
>>> +    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
>>> +
>>> +    if (vcon->chr) {
>>> +        qemu_chr_ioctl(vcon->chr, CHR_IOCTL_VIRT_SERIAL_OPEN,
>>>        
>> NULL);
>>      
>>> +    }
>>> +}
>>> +
>>> +static void virtconsole_guest_close(VirtIOSerialPort *port)
>>> +{
>>> +    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
>>> +
>>> +    if (vcon->chr) {
>>> +        qemu_chr_ioctl(vcon->chr, CHR_IOCTL_VIRT_SERIAL_CLOSE,
>>>        
>> NULL);
>>      
>>> +    }
>>> +}
>>> +
>>> +static void virtconsole_guest_ready(VirtIOSerialPort *port)
>>> +{
>>> +    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
>>> +
>>> +    if (vcon->chr) {
>>> +        qemu_chr_ioctl(vcon->chr, CHR_IOCTL_VIRT_SERIAL_READY,
>>>        
>> NULL);
>>      
>>> +    }
>>> +}
>>> +
>>>    /* Virtio Console Ports */
>>>    static int virtconsole_initfn(VirtIOSerialDevice *dev)
>>>    {
>>> @@ -94,6 +121,9 @@ static VirtIOSerialPortInfo virtconsole_info = {
>>>        .qdev.size     = sizeof(VirtConsole),
>>>        .init          = virtconsole_initfn,
>>>        .exit          = virtconsole_exitfn,
>>> +    .guest_open    = virtconsole_guest_open,
>>> +    .guest_close   = virtconsole_guest_close,
>>> +    .guest_ready   = virtconsole_guest_ready,
>>>        .qdev.props = (Property[]) {
>>>            DEFINE_PROP_UINT8("is_console", VirtConsole,
>>>        
>> port.is_console, 1),
>>      
>>>            DEFINE_PROP_UINT32("nr", VirtConsole, port.id,
>>>        
>> VIRTIO_CONSOLE_BAD_ID),
>>      
>>> @@ -130,6 +160,9 @@ static VirtIOSerialPortInfo virtserialport_info
>>>        
>> = {
>>      
>>>        .qdev.size     = sizeof(VirtConsole),
>>>        .init          = virtserialport_initfn,
>>>        .exit          = virtconsole_exitfn,
>>> +    .guest_open    = virtconsole_guest_open,
>>> +    .guest_close   = virtconsole_guest_close,
>>> +    .guest_ready   = virtconsole_guest_ready,
>>>        .qdev.props = (Property[]) {
>>>            DEFINE_PROP_UINT32("nr", VirtConsole, port.id,
>>>        
>> VIRTIO_CONSOLE_BAD_ID),
>>      
>>>            DEFINE_PROP_CHR("chardev", VirtConsole, chr),
>>> diff --git a/qemu-char.h b/qemu-char.h
>>> index e3a0783..1df53ae 100644
>>> --- a/qemu-char.h
>>> +++ b/qemu-char.h
>>> @@ -41,6 +41,10 @@ typedef struct {
>>>    #define CHR_IOCTL_SERIAL_SET_TIOCM   13
>>>    #define CHR_IOCTL_SERIAL_GET_TIOCM   14
>>>
>>> +#define CHR_IOCTL_VIRT_SERIAL_OPEN   15
>>> +#define CHR_IOCTL_VIRT_SERIAL_CLOSE  16
>>> +#define CHR_IOCTL_VIRT_SERIAL_READY  17
>>> +
>>>    #define CHR_TIOCM_CTS	0x020
>>>    #define CHR_TIOCM_CAR	0x040
>>>    #define CHR_TIOCM_DSR	0x100
>>>
>>>