From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:41305) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1USpOq-0006k5-O7 for qemu-devel@nongnu.org; Thu, 18 Apr 2013 10:01:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1USpOl-0006E2-HN for qemu-devel@nongnu.org; Thu, 18 Apr 2013 10:01:20 -0400 Received: from mail-wi0-x22d.google.com ([2a00:1450:400c:c05::22d]:55433) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1USpOl-0006Dp-AD for qemu-devel@nongnu.org; Thu, 18 Apr 2013 10:01:15 -0400 Received: by mail-wi0-f173.google.com with SMTP id c10so1940826wiw.6 for ; Thu, 18 Apr 2013 07:01:14 -0700 (PDT) Date: Thu, 18 Apr 2013 16:01:11 +0200 From: Stefan Hajnoczi Message-ID: <20130418140111.GA22842@stefanha-thinkpad.redhat.com> References: <1366187964-14265-1-git-send-email-qemulist@gmail.com> <1366187964-14265-2-git-send-email-qemulist@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1366187964-14265-2-git-send-email-qemulist@gmail.com> Subject: Re: [Qemu-devel] [RFC PATCH v4 01/15] util: introduce gsource event abstration List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Liu Ping Fan Cc: Stefan Hajnoczi , Jan Kiszka , qemu-devel@nongnu.org, mdroth , Anthony Liguori , Paolo Bonzini 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.