[RFC] [WIP] tevent/glib integration
Ralph Boehme
rb at sernet.de
Tue Dec 22 19:55:22 UTC 2015
Hi Noel,
On Mon, Dec 21, 2015 at 03:07:36PM +0000, Noel Power wrote:
> On 16/12/15 14:08, Stefan Metzmacher wrote:
> > very interesting work! It would be really good to have this
> > problem solved cleanly in future.
> [...]
> > Instead we can use a tracecallback and hook into the
> > TEVENT_TRACE_BEFORE_WAIT (and maybe TEVENT_TRACE_AFTER_WAIT)
> > events.
I believe you meant the TEVENT_TRACE_BEFORE_LOOP_ONCE and
TEVENT_TRACE_BEFORE_AFTER_ONCE trace points.
> >
> > For samba this could be done within smbd_tevent_trace_callback().
> >
> > In order to make this more generic we could add a helper function
> > which can be called in a TEVENT_TRACE_BEFORE_WAIT hook.
> >
> > This function would maintain some metadata, it needs an array
> > of tevent_fd pointers and old array returned by g_main_context_query()
> > and have a 2nd array to get the new g_main_context_query() result.
> > Then it uses this information to work out which tevent_fd pointers need
> > to be added/modified/removed.
> so, I played around with the TRACE stuff for a bit, my impression is
> that it makes things more awkward and more complicated, unfortunately
> trying to do this generically via the trace_callback functionality does
> not seem to work easily for a number of reasons
>
> + imho it is not at all an intuitive api to use, that the fds of the
> poll function could be modified by these trace points isn't at all
> obvious even by name
afaict we're not modifyint the pollfds, are we?
> + position of the BEFORE and AFTER events, it seems that these have a
> specific purpose (in the smbd at least) for profiling, if we piggy back
> on top of these we add quite some extra overhead and that would seem
> quite unexpected
As you've discovered, there's prior art. :)
> + but.. its not just the overhead of the whatever we call from the trace
> events we more than likely also have to move more code (currently
> outside) the BEFORE and AFTER events to inbetween it
I guess not, when using the right trace proints (LOOP, not WAIT), see
attached patch.
> e.g. in the s3
> tevent backend at least 'event_add_to_poll_args' and maybe even
> 'run_events_poll'. In anycase I would prefer to process the foreign
> events separately for 2 reasons and that activity also needs to be done
> inbetween the 'BEFORE' and 'AFTER' trace points. My reasons for
> processing *all* foreign fds at once are
> a) I want the behaviour and results of the poll to be as close as
> possible to what the external code would expect if it polled the fds
> itself, that means if events for more than one fd are available I want
> them all and not one at a time
> b) I think it could be wise to ensure the sequence (add foreign fds,
> poll foreign fds, report foreign fd poll results, react to foreign fds)
> happens all together without normal 'tevent' events happening inbetween
See attached patch, I think the glib loop is only entered once in
s3_tevent_foreign_loop_process().
> + even in the s3 case I would expect that you would not want to enable
> glib integration in 'all' smbd processes but instead leave it up to
> specific children to do that, the problem then is how do these children
> enable this,
Hm. Afacit the process has to call s3_tevent_integrate_glib() to
enable glib integration, right?
> perhaps there could be some boolean in the
> smbd_tevent_trace_state data, however I don't see an easy way for code
> in other modules to influence that state without exposing more stuff
> from source3/smbd/process.c. Of course child processes could create
> their own tracepoint callback (and associated state) but then they need
> to be aware of the previous trace callback and would need to
> additionally take care to chain the new trackpoint callback to the old
> + the code we call from BEFORE/AFTER be it from trace method or custom
> callbacks) needs to transfer information to the loop_wait code at the
> very least a timeout, I don't see a way to do this, ok you could pollute
> the tevent_context but tevent_context is not available from any public
> header
see attached patch.
> + is using the the 2 callbacks (BEFORE_WAIT & AFTER_WAIT) really any
> more complex than adding a function to the tevent api to provide
> similarly 2 callbacks (but ones with more friendly signatures)
>
> But... I want to avoid more analysis paralysis at this point, my main
> goal is to integrate glib for smbd, it's probably wiser to ensure that
> this stuff even works before going to anymore trouble (and Ralf
> hopefully can help to verify that). With that in mind I am going to
> shelve the generic part of this (note: I haven't given up on it... just
> thinking let's come back to it) and instead will concentrate on the 's3'
> backend, I'll try of course in the implementation to keep a focus on
> writing code that could be easily moved out of the backend
>
> Please find attached a new version of the patch, I've removed all the
> lib/tevent specific stuff to concentrate on the s3 backend, the
> implementation has only changed a little, I am not using the trace point
> callbacks but my own custom callbacks (but not exposed externally), I
> still use the 'lazy' method of just removing the old and adding the new
> fds ;-) (I do intend to fix that)
> The example application now uses the 's3' backend and any glib
> integration functionality previously in the example has been removed
>
> > Instead of using g_main_context_default(), we should make use of
> > g_main_context_new(), g_main_loop_new() and
> > g_main_context_{push,pop}_thread_default()
>
> I wonder do I miss something about needing to use the above (note: I
> don't have any deep knowledge of glib, I'm pretty clueless about it
> really) but it seems to me from reading
> https://developer.gnome.org/glib/stable/glib-The-Main-Event-Loop.html
> and the sections referenced from 'Customizing the main loop iteration'
> that I am doing the correct thing so I am wondering what the specific
> concern is and why I should use those methods
We'll see. It's all about getting the callbacks coming in back in the
right context. Ymmv.
Cheerio!
-slow
--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de,mailto:kontakt@sernet.de
-------------- next part --------------
From 18f878c43901dab559325a9493ef670983a95369 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Tue, 22 Dec 2015 17:29:49 +0100
Subject: [PATCH] WIP: use TEVENT_TRACE_BEFORE_LOOP_ONCE and
TEVENT_TRACE_BEFORE_AFTER_ONCE
---
source3/include/event.h | 3 +
source3/lib/events.c | 219 ++++++++++++++++++++++++------------------
source3/smbd/process.c | 2 +
source3/utils/async-tracker.c | 31 ++++++
4 files changed, 163 insertions(+), 92 deletions(-)
diff --git a/source3/include/event.h b/source3/include/event.h
index b3c7110..bca2ffb 100644
--- a/source3/include/event.h
+++ b/source3/include/event.h
@@ -48,4 +48,7 @@ struct idle_event *event_add_idle(struct tevent_context *event_ctx,
#define FOREIGN_FD_ERR 8
bool s3_tevent_integrate_glib(TALLOC_CTX *mem_ctx, struct tevent_context *ev);
+void s3_tevent_foreign_loop_prepare(struct tevent_context *ev);
+void s3_tevent_foreign_loop_process(struct tevent_context *ev);
+
bool create_glib_context(TALLOC_CTX *mem_ctx);
diff --git a/source3/lib/events.c b/source3/lib/events.c
index 18915d3..30dc794 100644
--- a/source3/lib/events.c
+++ b/source3/lib/events.c
@@ -23,15 +23,17 @@
#include "../lib/util/select.h"
#include "system/select.h"
#ifdef WITH_SPOTLIGHT /* #FIXME we need separate 'WITH_GLIB' & 'WITH_TRACKER' I think */
+/*
+ * glib uses TRUE and FALSE which was redefined by "includes.h" to be
+ * unusable, undefine so glib can establish its own working
+ * replacement.
+ */
+#undef TRUE
+#undef FALSE
#include <glib.h>
#endif
-struct foreign_loop_handler
-{
- int (*prepare_poll)(struct tevent_context *ev, void *private_data);
- void (*check_poll)(void *private_data);
- void *private_data;
-};
+struct tevent_foreign_poll_ctx;
struct tevent_poll_private {
/*
@@ -43,7 +45,28 @@ struct tevent_poll_private {
* Cache for s3_event_loop_once to avoid reallocs
*/
struct pollfd *pfds;
- struct foreign_loop_handler *foreign_poll;
+
+ /*
+ * Foreign eventloop context
+ */
+ struct tevent_foreign_poll_ctx *foreign_poll_ctx;
+};
+
+struct tevent_foreign_poll_ctx
+{
+ int (*prepare_poll)(struct tevent_context *ev, void *private_data);
+ void (*check_poll)(void *private_data);
+
+ /*
+ * Pointer to private data used by the implementation of a
+ * foreign eventloop integration
+ */
+ void *private_data;
+
+ /*
+ * Timeout returned from a foreign eventloop prepare function
+ */
+ int foreign_poll_timeout;
};
static struct tevent_poll_private *tevent_get_poll_private(
@@ -62,6 +85,18 @@ static struct tevent_poll_private *tevent_get_poll_private(
return state;
}
+static struct tevent_foreign_poll_ctx *tevent_get_foreign_poll_ctx(
+ struct tevent_context *ev)
+{
+ struct tevent_poll_private *state = tevent_get_poll_private(ev);
+
+ if (state == NULL) {
+ return NULL;
+ }
+
+ return state->foreign_poll_ctx;
+}
+
static void count_fds(struct tevent_context *ev,
int *pnum_fds, int *pmax_fd)
{
@@ -295,59 +330,10 @@ struct timeval *get_timed_events_timeout(struct tevent_context *ev,
return to_ret;
}
-static void process_foreign_fds(struct tevent_context *ev, int num_pfds)
-{
- struct tevent_fd *fde = NULL;
- struct tevent_fd *next = NULL;
- struct tevent_poll_private *state = tevent_get_poll_private(ev);
- int *pollfd_idx;
- int i = 0;
- pollfd_idx = state->pollfd_idx;
-
- /*
- * #FIXME we should track/maintain a copy or separate list of the
- * tevent_fd(s) associated with foreign fds, we should *not* have to
- * iterate over the entire set of fds
- */
- for (fde = ev->fd_events; fde; fde = next) {
- struct pollfd *pfd;
- bool is_foreign_fd =
- (fde->flags & FOREIGN_FD);
-
- next = fde->next;
- if (!is_foreign_fd) {
- continue;
- }
-
- if (pollfd_idx[fde->fd] >= num_pfds) {
- /* need to error out or something */
- DEBUG(1, ("internal error: pollfd_idx[fde->fd] (%d) "
- ">= num_pfds (%d)\n", pollfd_idx[fde->fd],
- num_pfds));
- i = 0;
- break;
- }
- pfd = &state->pfds[pollfd_idx[fde->fd]];
- if (pfd->revents & (POLLHUP|POLLERR)) {
- fde->additional_flags |=
- ((pfd->revents & (POLLHUP|POLLERR)));
- }
- if (pfd->revents & POLLIN) {
- fde->additional_flags |= POLLIN;
- }
- if (pfd->revents & POLLOUT)
- fde->additional_flags |= POLLOUT;
- if (pfd->revents) {
- i++;
- }
- fde->handler(ev, fde, fde->flags, fde->private_data);
- }
-}
-
static int s3_event_loop_once(struct tevent_context *ev, const char *location)
{
struct tevent_poll_private *state;
- void *private_data = NULL;
+ struct tevent_foreign_poll_ctx *foreign_poll_ctx = NULL;
int timeout;
int num_pfds;
int ret;
@@ -361,24 +347,20 @@ static int s3_event_loop_once(struct tevent_context *ev, const char *location)
return -1;
}
- private_data = state->foreign_poll->private_data;
+ foreign_poll_ctx = tevent_get_foreign_poll_ctx(ev);
+
if (run_events_poll(ev, 0, NULL, 0)) {
return 0;
}
- num_pfds = 0;
- if (state->foreign_poll->prepare_poll) {
- int new_timeout =
- state->foreign_poll->prepare_poll(ev, private_data);
- if (new_timeout < timeout) {
- timeout = new_timeout;
- }
+ if (foreign_poll_ctx) {
+ timeout = foreign_poll_ctx->foreign_poll_timeout;
}
+
+ num_pfds = 0;
+
if (!event_add_to_poll_args(ev, state,
&state->pfds, &num_pfds, &timeout)) {
- if (state->foreign_poll->check_poll) {
- state->foreign_poll->check_poll(private_data);
- }
return -1;
}
@@ -387,12 +369,6 @@ static int s3_event_loop_once(struct tevent_context *ev, const char *location)
poll_errno = errno;
tevent_trace_point_callback(ev, TEVENT_TRACE_AFTER_WAIT);
errno = poll_errno;
- if (state->foreign_poll->check_poll) {
- if (ret > 0) {
- process_foreign_fds(ev, ret);
- }
- state->foreign_poll->check_poll(private_data);
- }
if (ret == -1 && errno != EINTR) {
tevent_debug(ev, TEVENT_DEBUG_FATAL,
@@ -642,10 +618,10 @@ static int prepare_poll(struct tevent_context *ev,
return glib_timeout;
}
-static void check_poll(void *private_data)
+static void glib_check_poll(void *private_data)
{
struct loop_integration_data *glib_data =
- (struct loop_integration_data *)private_data;
+ talloc_get_type_abort(private_data, struct loop_integration_data);
GMainContext *ctx = g_main_context_default();
DEBUG(1,("#### checkpoll for ctx %p nfds %d\n", ctx, glib_data->num_active_fds));
@@ -681,11 +657,12 @@ static void foreign_fd_handler(struct tevent_context *ev,
uint16_t flags,
void *private_data)
{
- GPollFD *ffd =
- (GPollFD *)fde->private_data;
-
+ GPollFD *glib_fd = (GPollFD *)fde->private_data;
uint16_t glib_flags = 0;
- DEBUG(1,("handler fired for foreign fd %d %d flags 0x%x convert events\n", fde->fd, ffd->fd, flags));
+
+ DEBUG(1,("handler fired for foreign fd %d %d flags 0x%x convert events\n",
+ fde->fd, glib_fd->fd, flags));
+
if (flags & FOREIGN_FD_READ) {
glib_flags |= G_IO_IN;
}
@@ -698,7 +675,8 @@ static void foreign_fd_handler(struct tevent_context *ev,
if (flags & FOREIGN_FD_ERR) {
glib_flags |= G_IO_ERR;
}
- ffd->revents = glib_flags;
+
+ glib_fd->revents = glib_flags;
}
static uint16_t cvt_from_glib(uint16_t glib_evt)
@@ -744,11 +722,11 @@ static void add_foreignfds(struct tevent_context *ev,
}
}
-static int do_prepare_poll(struct tevent_context *ev,
- void *private_data)
+static int glib_prepare_poll(struct tevent_context *ev,
+ void *private_data)
{
struct loop_integration_data *glib_data =
- (struct loop_integration_data *)private_data;
+ talloc_get_type_abort(private_data, struct loop_integration_data);
int timeout;
/*
* #FIXME we need to do this properly, e.g. track previous
@@ -762,26 +740,83 @@ static int do_prepare_poll(struct tevent_context *ev,
}
#endif
+static void noop_fd_handler(struct tevent_context *ev,
+ struct tevent_fd *fde,
+ uint16_t flags,
+ void *private_data)
+{
+ return;
+}
+
bool s3_tevent_integrate_glib(TALLOC_CTX *mem_ctx, struct tevent_context *ev)
{
struct loop_integration_data *glib_data = NULL;
struct tevent_poll_private * state = tevent_get_poll_private(ev);
- struct foreign_loop_handler *handler;
-
+ struct tevent_foreign_poll_ctx *foreign_poll_ctx = NULL;
bool result = false;
int fd;
+
#ifdef WITH_SPOTLIGHT
- handler = talloc_zero(mem_ctx, struct foreign_loop_handler);
- state->foreign_poll = handler;
+ foreign_poll_ctx = talloc_zero(mem_ctx, struct tevent_foreign_poll_ctx);
+ if (foreign_poll_ctx == NULL) {
+ return false;
+ }
+ state->foreign_poll_ctx = foreign_poll_ctx;
+
glib_data = talloc_zero(mem_ctx, struct loop_integration_data);
- state->foreign_poll->prepare_poll = do_prepare_poll;
- state->foreign_poll->check_poll = check_poll;
- state->foreign_poll->private_data = glib_data;
+ if (glib_data == NULL) {
+ return false;
+ }
+
+ state->foreign_poll_ctx->private_data = glib_data;
+ state->foreign_poll_ctx->prepare_poll = glib_prepare_poll;
+ state->foreign_poll_ctx->check_poll = glib_check_poll;
+
/* create a bogus fd to allow the wait loop to be entered */
fd = dup(0);
- glib_data->starter_fd = tevent_add_fd(ev, mem_ctx, fd, TEVENT_FD_READ | TEVENT_FD_WRITE, NULL, NULL);
+ glib_data->starter_fd = tevent_add_fd(ev, mem_ctx, fd,
+ TEVENT_FD_READ | TEVENT_FD_WRITE,
+ noop_fd_handler, NULL);
tevent_fd_set_auto_close(glib_data->starter_fd);
result = true;
#endif
+
return result;
}
+
+void s3_tevent_foreign_loop_prepare(struct tevent_context *ev)
+{
+ struct tevent_foreign_poll_ctx *foreign_poll_ctx = NULL;
+
+ foreign_poll_ctx = tevent_get_foreign_poll_ctx(ev);
+ if (foreign_poll_ctx == NULL) {
+ return;
+ }
+
+ if (foreign_poll_ctx->prepare_poll == NULL) {
+ return;
+ }
+
+ foreign_poll_ctx->foreign_poll_timeout =
+ foreign_poll_ctx->prepare_poll(ev, foreign_poll_ctx->private_data);
+
+ return;
+}
+
+void s3_tevent_foreign_loop_process(struct tevent_context *ev)
+{
+ struct tevent_foreign_poll_ctx *foreign_poll_ctx = NULL;
+
+ foreign_poll_ctx = tevent_get_foreign_poll_ctx(ev);
+ if (foreign_poll_ctx == NULL) {
+ return;
+ }
+
+ if (foreign_poll_ctx->check_poll == NULL) {
+ return;
+ }
+
+ foreign_poll_ctx->check_poll(foreign_poll_ctx->private_data);
+
+ return;
+}
diff --git a/source3/smbd/process.c b/source3/smbd/process.c
index c99c75e..b2ce387 100644
--- a/source3/smbd/process.c
+++ b/source3/smbd/process.c
@@ -3564,8 +3564,10 @@ static void smbd_tevent_trace_callback(enum tevent_trace_point point,
case TEVENT_TRACE_BEFORE_LOOP_ONCE:
TALLOC_FREE(state->frame);
state->frame = talloc_stackframe_pool(8192);
+ s3_tevent_foreign_loop_prepare(state->ev);
break;
case TEVENT_TRACE_AFTER_LOOP_ONCE:
+ s3_tevent_foreign_loop_process(state->ev);
TALLOC_FREE(state->frame);
break;
}
diff --git a/source3/utils/async-tracker.c b/source3/utils/async-tracker.c
index d1a96bc..dd8bf51 100644
--- a/source3/utils/async-tracker.c
+++ b/source3/utils/async-tracker.c
@@ -35,6 +35,10 @@
*/
static int loop_type = 0;
+struct glib_tevent_trace_state {
+ struct tevent_context *ev;
+};
+
typedef struct {
TrackerSparqlConnection *connection;
GCancellable *cancellable;
@@ -186,6 +190,25 @@ connection_cb (GObject *object,
}
}
+static void glib_tevent_trace_callback(enum tevent_trace_point point,
+ void *private_data)
+{
+ struct glib_tevent_trace_state *state =
+ (struct glib_tevent_trace_state *)private_data;
+
+ switch (point) {
+ case TEVENT_TRACE_BEFORE_WAIT:
+ case TEVENT_TRACE_AFTER_WAIT:
+ break;
+ case TEVENT_TRACE_BEFORE_LOOP_ONCE:
+ s3_tevent_foreign_loop_prepare(state->ev);
+ break;
+ case TEVENT_TRACE_AFTER_LOOP_ONCE:
+ s3_tevent_foreign_loop_process(state->ev);
+ break;
+ }
+}
+
static void usage(void)
{
printf("tevent_glib [-t|-h|-g]\n"
@@ -202,7 +225,10 @@ main (gint argc, gchar *argv[])
TALLOC_CTX *mem_ctx = talloc_new(NULL); //parent
struct tevent_context *event_ctx;
int i;
+ struct glib_tevent_trace_state trace_state = { 0 };
+
md = g_new0 (MyData, 1);
+
/*
* hacky cmd line processing
* -t = use tevent event loop
@@ -239,6 +265,11 @@ main (gint argc, gchar *argv[])
printf("oh dear :-(\n");
exit(1);
}
+
+ trace_state.ev = event_ctx;
+ tevent_set_trace_callback(event_ctx, glib_tevent_trace_callback,
+ &trace_state);
+
printf("entering tevent_loop_wait\n");
//while (!tevent_loop_once(event_ctx));
tevent_loop_wait(event_ctx);
--
2.5.0
More information about the samba-technical
mailing list