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

[liu ping fan]

On Thu, Apr 18, 2013 at 10:01 PM, Stefan Hajnoczi <address@hidden> wrote:
> 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
>
Does G_IO_PRI only happen on read-in direction?

> 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.
>
I think you mean just prepare all of them, and let the dispatch decide
how to handle them, right?
>> +void event_source_release(EventGSource *src)
>> +{
>> +    g_source_destroy(&src->source);
>
> Leaks src.
>
All of the mem used by EventGSource are allocated by g_source_new, so
g_source_destroy can reclaim all of them.
>> +}
>> +
>> +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.
>
Thanks for pointing out. But see the reply to "unportable alloc_bmp" below.
>> +{
>> +    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.
>
Apply,
>> +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!
>
I had thought that resorting to bmp_sz to guarantee the bit-ops on
alloc_bmp. And if EventsGSource->pollfds is allocated with 64 instance
at initialize, it cost too much.   I can fix it with more fine code
when alloc_bmp's size growing.

> 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.
>
But as the case for slirp, owning to network's connection and
disconnection, the slirp's sockets can be dynamically changed quickly.
  The bitmap approach is something like slab, while glib container
type lacks such support (maybe using two GArray inuse[], free[]).

>> +/* 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.
Ok, that is more closed and reasonable.

Thanks, Pingfan