Re: [Qemu-devel] [PATCH 3/4] virtio-console: Add support for multiple ports for generic guest-host communication

[Gerd Hoffmann]

>   CharDriverState *qdev_init_chardev(DeviceState *dev)
>   {
>       static int next_serial;
> -    static int next_virtconsole;
> -    /* FIXME: This is a nasty hack that needs to go away.  */

Please don't drop this comment.  The whole function is a nasty hack ;)

> +VirtConBus *virtcon_bus_new(DeviceState *dev)
> +{
> +    if (virtcon_bus) {
> +        fprintf(stderr, "Can't create a second virtio-console bus\n");
> +        return NULL;
> +    }
> +    if (!dev) {
> +        dev = qdev_create(NULL, "virtio-console-sysbus");
> +        qdev_init(dev);
> +    }

Can this actually happen?  I think you'll allways have a virtio-console 
device as parent, right?

Looks like cut+pasted from isa-bus.c.  The ISA bus needs this for 
PCI-less machines where no PCI-ISA bridge is present and thus we need a 
isabus-sysbus bridge to hook up the ISA bus into the device tree.  Try 
'info qtree' on -M pc and -M isapc to see what I mean ;)

> +VirtConDevice *virtcon_create(const char *name)
> +VirtConDevice *virtcon_create_simple(const char *name)

These functions should get a VirtConBus passed in as first argument.

Looks like cut+paste from ISA bus again.  The ISA bus is special here as 
you never ever can have two ISA busses in a single machine.  That isn't 
true in general though.

Might also be you don't need these functions at all.  They usually used 
in case device creation is hard-coded somewhere (like standard isa 
devices present in every pc) or to keep old-style init functions working 
in the new qdev world (isa_ne2000_init for example).  Devices which are 
only ever created via -device don't take this code path ...

> +static void virtcon_bus_dev_print(Monitor *mon, DeviceState *dev, int indent)
> +{
> +    VirtConDevice *d = DO_UPCAST(VirtConDevice, qdev, dev);
> +    if (&d->qdev) {
> +      ;
> +    }
> +}

print callback isn't mandatory.  If you don't want to print anything 
just drop it.

> +static int virtcon_bus_init(SysBusDevice *dev)
> +{
> +    return 0;
> +}
> +
> +static SysBusDeviceInfo virtcon_sysbus_info = {
> +    .init = virtcon_bus_init,
> +    .qdev.name  = "virtio-console-sysbus",
> +    .qdev.size  = sizeof(SysBusDevice),
> +    .qdev.no_user = 1,
> +};
> +
> +static void virtcon_sysbus_register_devices(void)
> +{
> +    sysbus_register_withprop(&virtcon_sysbus_info);
> +}

See above.  I'm sure you don't need that.

> +struct VirtIOConsolePort {
> +    DeviceState dev;
> +
> +    VirtIOConsole *vcon;
> +    CharDriverState *hd;

This looks wrong.

> +    char *name;
> +
> +    QTAILQ_HEAD(, VirtIOConsolePortBuffer) unflushed_buffer_head;
> +
> +    bool guest_connected;
> +    bool host_connected;
> +};

Sticking a pointer to VirtConPortDeviceInfo here is probably handy.
More consistent naming please.

> +static int get_id_from_port(VirtIOConsolePort *port)
> +{
> +    uint32_t i;
> +
> +    for (i = 0; i<  MAX_VIRTIO_CONSOLE_PORTS; i++) {
> +        if (port == port->vcon->ports[i]) {
> +            return i;
> +        }
> +    }
> +    return VIRTIO_CONSOLE_BAD_ID;
> +}

Just sick a id element into VirtIOConsolePort?

> +static bool has_complete_data(VirtIOConsolePort *port)
> +{
> +    VirtIOConsolePortBuffer *buf;
> +    size_t len, size;
> +
> +    len = 0;
> +    size = 0;
> +    QTAILQ_FOREACH(buf,&port->unflushed_buffer_head, next) {
> +        if (!buf->size&&  buf == QTAILQ_FIRST(&port->unflushed_buffer_head)) {
> +            /* We have a buffer that's lost its way; just flush it */

Can this happen?  If not, assert() instead?

> +static size_t flush_buf(VirtIOConsolePort *port, const uint8_t *buf, size_t 
> len)
> +{
> +    if (!port->hd) {
> +        return 0;
> +    }
> +    return qemu_chr_write(port->hd, buf, len);

port->info->data_for_you(port, buf, len);

> +static void flush_queue(VirtIOConsolePort *port)
> +{
> +    VirtIOConsolePortBuffer *buf, *buf2;
> +    uint8_t *outbuf;
> +    size_t outlen, outsize;
> +
> +    while (!QTAILQ_EMPTY(&port->unflushed_buffer_head)) {
> +        if (!has_complete_data(port)) {
> +            break;
> +        }
> +
> +        buf = QTAILQ_FIRST(&port->unflushed_buffer_head);
> +        if (!buf->size) {
> +            /* This is a buf that didn't get consumed as part of a
> +             * previous data stream. Bad thing, shouldn't
> +             * happen. But let's handle it nonetheless
> +             */

If it shoudn't happen, then use assert().  If it triggers, find the bug.

> +/* Guest wants to notify us of some event */
> +static void handle_control_message(VirtIOConsolePort *port,
> +                                   struct virtio_console_control *cpkt)
> +{
> +    uint8_t *buffer;
> +    size_t buffer_len;
> +
> +    switch(cpkt->event) {
> +    case VIRTIO_CONSOLE_PORT_OPEN:
> +        port->guest_connected = cpkt->value;

port->info->guest_open() notify callback?

> +static int vcon_port_initfn(VirtConDevice *dev)
> +{
> +    VirtIOConsolePort *port;
> +
> +    port = DO_UPCAST(VirtIOConsolePort, dev,&dev->qdev);
> +
> +    QTAILQ_INIT(&port->unflushed_buffer_head);
> +
> +    port->vcon = virtio_console;
> +
> +    qemu_chr_add_handlers(port->hd, vcon_can_read, vcon_read, vcon_event, 
> port);

Why have a chardev here?

> +typedef struct VirtConPortDeviceInfo {
> +    DeviceInfo qdev;
> +    virtcon_port_qdev_initfn init;

Stick in more function pointers here.  guest_open(), data_for_you(), ...


Well.  The whole thing is still *way* to mixed up.  It should be cleanly 
separated.

You should be able to move the port driver(s) to a separate source file 
without much trouble.  Only the port driver should deal with a chardev. 
 The virtio-console core should not care at all how the data is piped 
to the (host side) users.  It just drives the ring, forwards events, 
accepts data for the guest (via helper function), passes on data from 
the guest (via callback in VirtConPortDeviceInfo).

cheers,
  Gerd