Re: [Qemu-devel] [RFC PATCH v5 01/14] util: introduce gsource event abstraction

[liu ping fan]

On Fri, Apr 26, 2013 at 5:19 PM, Stefan Hajnoczi <address@hidden> wrote:
> On Fri, Apr 26, 2013 at 10:47:22AM +0800, Liu Ping Fan wrote:
>> +GPollFD *events_source_add_gfd(EventsGSource *src, int fd)
>> +{
>> +    GPollFD *retfd;
>> +
>> +    retfd = g_slice_alloc(sizeof(GPollFD));
>> +    retfd->events = 0;
>> +    retfd->fd = fd;
>> +    src->pollfds_list = g_list_append(src->pollfds_list, retfd);
>> +    if (fd > 0) {
>
> 0 (stdin) is a valid fd number.  Maybe just assert(fd >= 0)?
>
Yes, 0 should be allowed.   Here, the reason to use check instead of
assert , is that socreate() in slirp is a good place to call
_add_gfd,  but unfortunately, at that time, its socket handler so->s =
-1;   So we create the GPollFD ahead, and delay to call
g_source_add_poll()

>> +static gboolean events_source_check(GSource *src)
>> +{
>> +    EventsGSource *nsrc = (EventsGSource *)src;
>> +    GList *cur;
>> +    GPollFD *gfd;
>> +
>> +    cur = nsrc->pollfds_list;
>> +    while (cur) {
>> +        gfd = cur->data;
>> +        if (gfd->fd > 0 && (gfd->revents & gfd->events)) {
>
> revents will always be 0 if fd is invalid, since we didn't call
> g_source_add_poll().  Is there a reason to perform the fd > 0 check
> again?
>
As explained above, we should skip the case gfd->fd=-1, which may
occur in pollfds_list.

>> +void events_source_release(EventsGSource *src)
>> +{
>
> assert that pollfds_list is empty?  We don't g_slice_free() GPollFDs so
> it must be empty here.

Do you mean that   events_source_add_gfd/events_source_remove_gfd are
paired, so here, we ensure that pollfds_list==NULL ?

Thanks
Pingfan