Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports

[Gerd Hoffmann <kraxel@redhat.com>]

> +typedef struct VirtIOConsole
> +{

You are reusing this struct name a few times.
Better don't do that, it is confusing.

> +static VirtIOConsole *virtio_console;

Why do you need this?

> +static void flush_queue(VirtConPort *port)
> +{
> +    VirtConPortBuffer *buf, *buf2;
> +    uint8_t *outbuf;
> +    size_t outlen, outsize;
> +
> +    /*
> +     * If the app isn't interested in buffering packets till it's
> +     * opened, just drop the data guest sends us till a connection is
> +     * established.
> +     */
> +    if (!port->host_connected&&  !port->flush_buffers)
> +        return;

Hmm.  Who needs that buffering?  Only user seems to be the port driver 
(patch 4/6).  So move the buffering (and the host_connected state 
tracking) from the core to that driver?

> +/* Readiness of the guest to accept data on a port */
> +static int vcon_can_read(void *opaque)

int vcon_can_read(VirtConPort *port)

> +static void vcon_read(void *opaque, const uint8_t *buf, int size)
> +static void vcon_event(void *opaque, int event)

Likewise.

> +static VirtConBus *virtcon_bus;

Why do you need this?

> +static int virtcon_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
> +{

> +    if (port->chr) {
> +        qemu_chr_add_handlers(port->chr, vcon_can_read, vcon_read, vcon_event,
> +                              port);

Should be handled by the VirtConPort drivers.

> +    if (MAX_VIRTIO_CONSOLE_PORTS % 32) {
> +        /* We require MAX_VIRTIO_CONSOLE_PORTS be a multiple of 32:
> +         * We anyway use up that much space for the bitmap and it
> +         * simplifies some calculations
> +         */
> +        return NULL;
> +    }

Huh?  Runtime check for a compile-time constant?

> +    /* Spawn a new virtio-console bus on which the ports will ride as devices */
> +    virtcon_bus_new(dev);

s->bus = virtcon_bus_new(dev);
port0 = qdev_create(s->bus, "virtconsole");  /* console @ port0 for 
backward compat */
qdev_prop_set_*(port0, ...);
qdev_init(port0);

Or does that happen somewhere else?

> +typedef struct VirtConBus VirtConBus;
> +typedef struct VirtConPort VirtConPort;
> +typedef struct VirtConPortInfo VirtConPortInfo;
> +
> +typedef struct VirtConDevice {
> +    DeviceState qdev;
> +    VirtConPortInfo *info;
> +} VirtConDevice;

Leftover from old patch version?

> +/*
> + * This is the state that's shared between all the ports.  Some of the
> + * state is configurable via command-line options. Some of it can be
> + * set by individual devices in their initfn routines. Some of the
> + * state is set by the generic qdev device init routine.
> + */
> +struct VirtConPort {
> +    DeviceState dev;
> +    VirtConPortInfo *info;
> +
> +    /* State for the chardevice associated with this port */
> +    CharDriverState *chr;

That should go to the port driver if needed.

> +typedef int (*virtcon_port_qdev_initfn)(VirtConDevice *dev);
> +
> +struct VirtConPortInfo {
> +    DeviceInfo qdev;
> +    virtcon_port_qdev_initfn init;
> +
> +    /* Callbacks for guest events */
> +    void (*guest_open)(VirtConPort *port);
> +    void (*guest_close)(VirtConPort *port);
> +
> +    size_t (*have_data)(VirtConPort *port, const uint8_t *buf, size_t len);

Maybe it is a good idea to pass a VirtConPortBuffer here instead of 
buf+len, especially when it becomes the port drivers job to do any 
buffering if needed.

cheers,
   Gerd