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

[Michael Roth <mdroth@linux.vnet.ibm.com>]

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 <pingfank@linux.vnet.ibm.com>
> ---
>  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.

> +{
> +    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) {
> +        g_source_add_poll(&src->source, retfd);
> +    }
> +
> +    return retfd;
> +}
> +
> +void events_source_remove_pollfd(EventsGSource *src, GPollFD *pollfd)
> +{
> +    g_source_remove_poll(&src->source, pollfd);
> +    src->pollfds_list = g_list_remove(src->pollfds_list, pollfd);
> +    g_slice_free(GPollFD, pollfd);
> +}
> +
> +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)) {
> +            return true;
> +        }
> +        cur = g_list_next(cur);
> +    }
> +
> +    return false;
> +}
> +
> +static gboolean events_source_dispatch(GSource *src, GSourceFunc cb,
> +    gpointer data)
> +{
> +    gboolean ret = false;
> +
> +    if (cb) {
> +        ret = cb(data);
> +    }
> +    return ret;
> +}
> +
> +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.

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
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)

> +    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