Re: [Qemu-devel] [PATCH v1 1/5] util: introduce gsource event abstraction

[liu ping fan]

On Fri, Aug 9, 2013 at 12:29 AM, Michael Roth <address@hidden> wrote:
> Quoting Liu Ping Fan (2013-08-08 01:26:07)
>> Introduce struct EventsGSource. It will ease the usage of GSource
>> associated with a group of files, which are dynamically allocated
>> and release, ex, slirp.
>>
>> Signed-off-by: Liu Ping Fan <address@hidden>
>> ---
>>  util/Makefile.objs   |  1 +
>>  util/event_gsource.c | 94 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  util/event_gsource.h | 37 +++++++++++++++++++++
>>  3 files changed, 132 insertions(+)
>>  create mode 100644 util/event_gsource.c
>>  create mode 100644 util/event_gsource.h
>>
>> diff --git a/util/Makefile.objs b/util/Makefile.objs
>> index dc72ab0..eec55bd 100644
>> --- a/util/Makefile.objs
>> +++ b/util/Makefile.objs
>> @@ -11,3 +11,4 @@ util-obj-y += iov.o aes.o qemu-config.o qemu-sockets.o 
>> uri.o notify.o
>>  util-obj-y += qemu-option.o qemu-progress.o
>>  util-obj-y += hexdump.o
>>  util-obj-y += crc32c.o
>> +util-obj-y += event_gsource.o
>> diff --git a/util/event_gsource.c b/util/event_gsource.c
>> new file mode 100644
>> index 0000000..4b9fa89
>> --- /dev/null
>> +++ b/util/event_gsource.c
>> @@ -0,0 +1,94 @@
>> +/*
>> + *  Copyright (C) 2013 IBM
>> + *
>> + *  This program is free software; you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License as published by
>> + *  the Free Software Foundation; under version 2 of the License.
>> + *
>> + *  This program is distributed in the hope that it will be useful,
>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *  GNU General Public License for more details.
>> + *
>> + *  You should have received a copy of the GNU General Public License
>> + *  along with this program; if not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include "event_gsource.h"
>> +#include "qemu/bitops.h"
>> +
>> +GPollFD *events_source_add_pollfd(EventsGSource *src, int fd)
>
> Small nit, but if the class is EventsGSource, the methods should
> use the events_gsource_* prefix. Or we can just call it EventsSource.
>
Will fix.
[...]

>> +
>> +EventsGSource *events_source_new(GPrepare prepare, GSourceFunc dispatch_cb,
>> +    void *opaque)
>> +{
>> +    EventsGSource *src;
>> +    GSourceFuncs *gfuncs = g_new0(GSourceFuncs, 1);
>> +    gfuncs->prepare = prepare;
>
> I'm not sure how useful this EventsGSource abstraction is if it requires users
> to implement a custom prepare() function that accesses EventsGSource fields
> directly.
>
> Either we should just make this SlirpGSource until another user comes
> along where we can look at pulling out common bits, or we should pass in a
> prepare() function that operates on the opaque data instead of the
> underlying EventsGSource.
>
Maybe SlirpGSource, since the prepare of slirp is too complicated.

> If you take the latter approach, you might consider having
> events_source_add_pollfd take a GPollFD* arg instead of an fd, then storing
> a pointer to the same GPollFD* in your opaque/Slirp object so you can do 
> things

The GPollFD* is stored in slirp's socket (each slirp-socket has a sock-fd ).
> like set the event masks for all the GPollFDs in the prepare cb prior to
> completing the GSource's prepare function (which could then do something 
> generic
> like return true if any GPollFDs have a non-zero event mask)
>
What is the aim of "masks for all the GPollFDs"?  We just poll the
GPollFD when so->state ask us to do that. Otherwise they are kept
clean.

Thanks and regards,
Pingfan
>> +    gfuncs->check = events_source_check,
>> +    gfuncs->dispatch = events_source_dispatch,
>> +
>> +    src = (EventsGSource *)g_source_new(gfuncs, sizeof(EventsGSource));
>> +    src->gfuncs = gfuncs;
>> +    src->pollfds_list = NULL;
>> +    src->opaque = opaque;
>> +    g_source_set_callback(&src->source, dispatch_cb, src, NULL);
>> +
>> +    return src;
>> +}
>> +
>> +void events_source_release(EventsGSource *src)
>> +{
>> +    assert(!src->pollfds_list);
>> +    g_free(src->gfuncs);
>> +    g_source_destroy(&src->source);
>> +}
>> diff --git a/util/event_gsource.h b/util/event_gsource.h
>> new file mode 100644
>> index 0000000..8755952
>> --- /dev/null
>> +++ b/util/event_gsource.h
>> @@ -0,0 +1,37 @@
>> +/*
>> + *  Copyright (C) 2013 IBM
>> + *
>> + *  This program is free software; you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License as published by
>> + *  the Free Software Foundation; under version 2 of the License.
>> + *
>> + *  This program is distributed in the hope that it will be useful,
>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *  GNU General Public License for more details.
>> + *
>> + *  You should have received a copy of the GNU General Public License
>> + *  along with this program; if not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#ifndef EVENT_GSOURCE_H
>> +#define EVENT_GSOURCE_H
>> +#include "qemu-common.h"
>> +
>> +typedef gboolean (*GPrepare)(GSource *source, gint *timeout_);
>> +
>> +/* multi fd drive GSource*/
>> +typedef struct EventsGSource {
>> +    GSource source;
>> +    /* a group of GPollFD which dynamically join or leave the GSource */
>> +    GList *pollfds_list;
>> +    GSourceFuncs *gfuncs;
>> +    void *opaque;
>> +} EventsGSource;
>> +
>> +EventsGSource *events_source_new(GPrepare prepare, GSourceFunc dispatch_cb,
>> +    void *opaque);
>> +void events_source_release(EventsGSource *src);
>> +GPollFD *events_source_add_pollfd(EventsGSource *src, int fd);
>> +void events_source_remove_pollfd(EventsGSource *src, GPollFD *pollfd);
>> +#endif
>> --
>> 1.8.1.4