Re: [Qemu-devel] [RFC PATCH v4 01/15] util: introduce gsource event abstration

[Stefan Hajnoczi <stefanha@gmail.com>]

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.