diff mbox

[RFC,v5,01/14] util: introduce gsource event abstraction

Message ID 1366944455-14239-2-git-send-email-qemulist@gmail.com
State New
Headers show

Commit Message

pingfan liu April 26, 2013, 2:47 a.m. UTC
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

Introduce two structs EventGSource, EventsGSource
EventGSource is used to abstract the event with single backend file.
EventsGSource is used to abstract the event with dynamically changed
backend file, ex, slirp.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 util/Makefile.objs   |    1 +
 util/event_gsource.c |  158 ++++++++++++++++++++++++++++++++++++++++++++++++++
 util/event_gsource.h |   49 +++++++++++++++
 3 files changed, 208 insertions(+), 0 deletions(-)
 create mode 100644 util/event_gsource.c
 create mode 100644 util/event_gsource.h

Comments

Stefan Hajnoczi April 26, 2013, 9:19 a.m. UTC | #1
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)?

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

> +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.
pingfan liu April 27, 2013, 2:11 a.m. UTC | #2
On Fri, Apr 26, 2013 at 5:19 PM, Stefan Hajnoczi <stefanha@redhat.com> 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
Stefan Hajnoczi April 29, 2013, 8 a.m. UTC | #3
On Sat, Apr 27, 2013 at 10:11:40AM +0800, liu ping fan wrote:
> On Fri, Apr 26, 2013 at 5:19 PM, Stefan Hajnoczi <stefanha@redhat.com> 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()

ok

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

ok

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

Yes.  It is an error to hold a GPollFD across events_source_release() so
you could place an assert() here to check.

Stefan
diff mbox

Patch

diff --git a/util/Makefile.objs b/util/Makefile.objs
index 495a178..a676d7d 100644
--- a/util/Makefile.objs
+++ b/util/Makefile.objs
@@ -8,3 +8,4 @@  util-obj-y += error.o qemu-error.o
 util-obj-$(CONFIG_POSIX) += compatfd.o
 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 += event_gsource.o
diff --git a/util/event_gsource.c b/util/event_gsource.c
new file mode 100644
index 0000000..9cfdb4a
--- /dev/null
+++ b/util/event_gsource.c
@@ -0,0 +1,158 @@ 
+/*
+ *  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"
+
+static gboolean prepare(GSource *src, gint *time)
+{
+    EventGSource *nsrc = (EventGSource *)src;
+    int events = 0;
+
+    if (!nsrc->readable && !nsrc->writable) {
+        return false;
+    }
+    if (nsrc->readable) {
+        events = nsrc->readable(nsrc->opaque);
+    }
+    if ((nsrc->writable)) {
+        events |= nsrc->writable(nsrc->opaque);
+    }
+    nsrc->gfd.events = events;
+
+    return false;
+}
+
+static gboolean check(GSource *src)
+{
+    EventGSource *nsrc = (EventGSource *)src;
+
+    if (nsrc->gfd.revents & nsrc->gfd.events) {
+        return true;
+    }
+    return false;
+}
+
+static gboolean dispatch(GSource *src, GSourceFunc cb, gpointer data)
+{
+    gboolean ret = false;
+
+    if (cb) {
+        ret = cb(data);
+    }
+    return ret;
+}
+
+static GSourceFuncs net_gsource_funcs = {
+    prepare,
+    check,
+    dispatch,
+    NULL
+};
+
+EventGSource *event_source_new(int fd, GSourceFunc dispatch_cb, void *opaque)
+{
+    EventGSource *nsrc = (EventGSource *)g_source_new(&net_gsource_funcs,
+                                                    sizeof(EventGSource));
+    nsrc->gfd.fd = fd;
+    nsrc->opaque = opaque;
+    g_source_set_callback(&nsrc->source, dispatch_cb, nsrc, NULL);
+    g_source_add_poll(&nsrc->source, &nsrc->gfd);
+
+    return nsrc;
+}
+
+void event_source_release(EventGSource *src)
+{
+    g_source_destroy(&src->source);
+}
+
+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) {
+        g_source_add_poll(&src->source, retfd);
+    }
+
+    return retfd;
+}
+
+void events_source_remove_gfd(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;
+    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)
+{
+    g_list_free(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..25b15d7
--- /dev/null
+++ b/util/event_gsource.h
@@ -0,0 +1,49 @@ 
+/*
+ *  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 gushort (*Pollable)(void *opaque);
+typedef gboolean (*GPrepare)(GSource *source, gint *timeout_);
+
+/* single fd drive gsource */
+typedef struct EventGSource {
+    GSource source;
+    GPollFD gfd;
+    Pollable readable;
+    Pollable writable;
+    void *opaque;
+} EventGSource;
+
+EventGSource *event_source_new(int fd, GSourceFunc dispatch_cb, void *opaque);
+void event_source_release(EventGSource *src);
+
+/* multi fd drive gsource*/
+typedef struct EventsGSource {
+    GSource source;
+    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_gfd(EventsGSource *src, int fd);
+void events_source_remove_gfd(EventsGSource *src, GPollFD *pollfd);
+#endif