[PATCH] tevent and threads - infrastructure improvements - version #2

[Simo Sorce]

On Fri, 2015-07-24 at 10:16 -0700, Jeremy Allison wrote:
> On Thu, Jul 23, 2015 at 04:50:37PM -0700, Jeremy Allison wrote:
> > 
> > FYI. I now have a working implementation of this
> > API - passes valgrind memcheck and drd !
> > 
> > Hurrah for me :-).
> > 
> > Will post an updated patch once I've finished
> > updating the tutorial.
> 
> Here it is. Passes valgrind --tool=drd and
> valgrind --tool=memcheck.
> 
> Metze please let me know if this is what
> you had in mind.
> 
> Everyone else just review :-).

I like this patchset, the general flow of code looks and feels good (I'm
using metze's lingo here :-).

I see no real issues with the architecture of the code after you applied
metze's requested changes.

However I wonder if we shouldn't make it easier to have a return
callback automatically called, instead of requesting the caller to
schedule back a return function.

This can be easily done later adding more APIs, but I think it would be
beneficial to do it straight away to avoid mixed style in the code base
later on.

The natural way people will want to use this is that they create a
thread pool at program initialization and then they just fire threaded
calls and expect a request handler to be fired back when the thread is
done. Doing this manually will be quite burdensome and repetitive so I
would expect to use a higher level API that goes something like:

void tevent_thread_run(tevent_thread_pool *pool, tevent_context *ev,
		       tevent_thread_handler *in_thread_handler,
		       tevent_thread_handler *return_callback,
		       void *private_data);

The pool could be folded into ev->thread_pool and we can do automatic
allocation tied to a specific event handler, probably better to keep it
private and per ev handler. 

We can add this later though if you think it is better to get your
foundation work in first.


On the patch itself I have 3 nitpicks.
1. can we add a DLINK_FOREACH() macro ?

2. in the pipe_read_handler you have
+       while (len == 64) {
+               char buf[64];
+               len = read(tp->read_fd, buf, 64);
+       };

Shouldn't this be:
while (len > 0) {
	char buf[64];
	size_t size = read(tp->read_fd, buf, 64);
	if (size == -1) break;
	len -= size;
} 
You could have short reads, but perhaps it doesn't matter anyway ...
Also, why 64 ?

3. In the tests you assign 'finished' and read it across threads, this
works only if assignments/reads are atomic, but this is not guaranteed
in your code, if we are unlucky it may cause flailing test failures on
some architectures. Also if I read it right 'finished' is on the stack
and you have a timeout that can make the thread terminate before the
master writes to it, this could cause the test to write on freed/reused
stack or segfault or similar causing also undebuggable testing failures.

HTH,
Simo.

-- 
Simo Sorce * Red Hat, Inc * New York