From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Msh4B-00046W-VH for qemu-devel@nongnu.org; Tue, 29 Sep 2009 14:04:48 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Msh46-0003yW-AE for qemu-devel@nongnu.org; Tue, 29 Sep 2009 14:04:46 -0400 Received: from [199.232.76.173] (port=41662 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Msh46-0003y8-5F for qemu-devel@nongnu.org; Tue, 29 Sep 2009 14:04:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:8752) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Msh45-0007Zk-40 for qemu-devel@nongnu.org; Tue, 29 Sep 2009 14:04:41 -0400 Received: from int-mx05.intmail.prod.int.phx2.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.18]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n8TI4drn001230 for ; Tue, 29 Sep 2009 14:04:39 -0400 Message-ID: <4AC24C34.2080609@redhat.com> Date: Tue, 29 Sep 2009 20:04:36 +0200 From: Gerd Hoffmann MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 3/6] virtio-console: Add a virtio-console bus, support for multiple ports References: <1254225888-17093-1-git-send-email-amit.shah@redhat.com> <1254225888-17093-2-git-send-email-amit.shah@redhat.com> <1254225888-17093-3-git-send-email-amit.shah@redhat.com> <1254225888-17093-4-git-send-email-amit.shah@redhat.com> In-Reply-To: <1254225888-17093-4-git-send-email-amit.shah@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amit Shah Cc: qemu-devel@nongnu.org > +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