[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH v4 01/15] util: introduce gsource event abst
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [RFC PATCH v4 01/15] util: introduce gsource event abstration |
Date: |
Thu, 18 Apr 2013 16:01:11 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Wed, Apr 17, 2013 at 04:39:10PM +0800, Liu Ping Fan wrote:
> +static gboolean prepare(GSource *src, gint *time)
> +{
> + EventGSource *nsrc = (EventGSource *)src;
> + int events = 0;
> +
> + if (!nsrc->readable && !nsrc->writable) {
> + return false;
> + }
> + if (nsrc->readable && nsrc->readable(nsrc->opaque)) {
> + events |= G_IO_IN;
> + }
> + if ((nsrc->writable) && nsrc->writable(nsrc->opaque)) {
> + events |= G_IO_OUT;
> + }
G_IO_ERR, G_IO_HUP, G_IO_PRI?
Here is the select(2) to GCondition mapping:
rfds -> G_IO_IN | G_IO_HUP | G_IO_ERR
wfds -> G_IO_OUT | G_IO_ERR
xfds -> G_IO_PRI
In other words, we're missing events by just using G_IO_IN and G_IO_OUT.
Whether that matters depends on EventGSource users. For sockets it can
matter.
> +void event_source_release(EventGSource *src)
> +{
> + g_source_destroy(&src->source);
Leaks src.
> +}
> +
> +GPollFD *events_source_get_gfd(EventsGSource *src, int fd)
events_source_add_fd() seems like a better name since this function
always allocates a new GPollFD, it never "gets" an existing one.
> +{
> + GPollFD *retfd;
> + unsigned long idx;
> +
> + idx = find_first_zero_bit(src->alloc_bmp, src->bmp_sz);
> + if (idx == src->bmp_sz) {
> + //idx = src->bmp_sz;
Commented out line.
> +void events_source_close_gfd(EventsGSource *src, GPollFD *pollfd)
"close" usually means close(2). I suggest "remove" instead.
> +EventsGSource *events_source_new(GSourceFuncs *funcs, GSourceFunc
> dispatch_cb, void *opaque)
> +{
> + EventsGSource *src = (EventsGSource *)g_source_new(funcs,
> sizeof(EventsGSource));
> +
> + /* 8bits size at initial */
> + src->bmp_sz = 8;
> + src->alloc_bmp = g_malloc0(src->bmp_sz >> 3);
This is unportable. alloc_bmp is unsigned long, you are allocating just
one byte!
Please drop the bitmap approach and use a doubly-linked list or another
glib container type of your choice. It needs 3 operations: add, remove,
and iterate.
> +/* multi fd drive gsource*/
> +typedef struct EventsGSource {
> + GSource source;
> + /* 8 for initial, stand for 8 pollfds */
> + unsigned int bmp_sz;
> + unsigned long *alloc_bmp;
> + GPollFD *pollfds;
> + void *opaque;
> +} EventsGSource;
> +
> +EventsGSource *events_source_new(GSourceFuncs *funcs, GSourceFunc
> dispatch_cb, void *opaque);
> +void events_source_release(EventsGSource *src);
> +gboolean events_source_check(GSource *src);
> +gboolean events_source_dispatch(GSource *src, GSourceFunc cb, gpointer data);
> +GPollFD *events_source_get_gfd(EventsGSource *src, int fd);
> +void events_source_close_gfd(EventsGSource *src, GPollFD *pollfd);
Why are check/dispatch public? Perhaps events_source_new() just needs a
prepare() argument instead of exposing GSourceFuncs.
[Qemu-devel] [RFC PATCH v4 02/15] net: introduce bind_ctx to NetClientInfo, Liu Ping Fan, 2013/04/17
[Qemu-devel] [RFC PATCH v4 03/15] net: port tap onto GSource, Liu Ping Fan, 2013/04/17
[Qemu-devel] [RFC PATCH v4 04/15] net: resolve race of tap backend and its peer, Liu Ping Fan, 2013/04/17
[Qemu-devel] [RFC PATCH v4 05/15] net: port vde onto GSource, Liu Ping Fan, 2013/04/17
[Qemu-devel] [RFC PATCH v4 06/15] net: port socket to GSource, Liu Ping Fan, 2013/04/17