[PATCH] tevent-glib-glue

[Noel Power]

Hi Ralf,

A little later than I expected. So still I haven't looked at the mdssvc
parts. About the glue code at least I had a pretty good look.

+ in 'tevent_glib_glue_trace_callback' we currently are calling
tevent_glib_prepare potentially every trace point so possibly 4 times
per tevent loop iteration (depending on what fires)

+ 'g_main_context_acquire' & 'g_main_context_release'  calls need to be
matched (currently they are not) otherwise the gmain_ctx owner_counter
will runaway making it impossible for a different thread acquire the context

note: from glib doc

    "g_main_context_release ()

       void g_main_context_release (GMainContext *context);

    Releases ownership of a context previously acquired by this thread
with g_main_context_acquire(). If the context was acquired multiple
times, the ownership will be released only when g_main_context_release()
is called as many times as it was acquired."

I think similar handling to skip_glue_trace_event I had and using just
TEVENT_TRACE_AFTER_LOOP_ONCE with tevent_glib_process (instead of
tevent_glib_prepare) should ensure the correct ordering of
acquire/release (see attached patch to your which simplifies my previous
approach with this and the other issues mentioned here and below)

+ After calling tevent_glib_glue_cleanup we need to re-initialiase/re-alloc
  some glue context items to avoid using freed memory

+ Similarly we need to initialise some additional glue context members,
num_maps
  definitely needs to be reset to 0 (and probably gtimeout, gpriority etc_

I haven't had a chance to run your latest patches (or my adjustments to
those patches) on WSP yet (I will try to do that today)

I tried to run make test TESTS=samba3.tevent_glib_glue with no luck,
doesn't seem to think the test exists (I don't know what I did wrong :/)
Note: I can run the test manually from test_tevent_glib_glue.sh

Also I noticed that the tests should run regardless of whether glib_glue
stuff is built (via fake tevent_glib_glue_test generated in the test) I
don't love that approach as it looks like the glue test has run and
passed, I mean the current CI looks like it is running with the
glib_glue stuff but you need --enable-spotlight for the glib_glue stuff
to be built and afaics none of the CI (or autobuild) test jobs enable that.

I'll try to get to the main mdssvc patches after I try and test with WSP
again

thanks,
Noel

On 20/03/2019 10:14, Noel Power wrote:
> On 18/03/2019 10:55, Ralph Böhme wrote:
>> Hi Noel, sorry for the delay, had two days off. 
> No problem, I am just back today from even more days than that off
> :->>> I see below (snipped) you have a new patch set, I'll get to it
> asap, just beware the usual post days off mail mountain will interfere
> a bit :-)
>> On Thu, Mar 14, 2019 at 05:15:07PM +0000, Noel Power wrote: 
> [...]
>>> The problem I see is that if this glib tracker api results in any
>>> changes to the set of glib fd(s) or timeouts that need to be
>>> monitored then the glue code may not be aware of these changes
>>> (because the prepare phase is only called from glib glue tevent
>>> handlers) See attached async_tracker.c patch which attempts to fake
>>> these scenarios. [1] 
>> ah, finally got it. :) This should be addressed in the attached
>> patchset. I'm basically taking you tevent tracepoint approach with
>> some simplifications. I've also added tests to the testsuite that
>> cover both cases: adding a new glib event source from a glib event
>> handler as well as adding one from a tevent event handler. Updated
>> patchset attached. CI:
>> https://gitlab.com/samba-team/devel/samba/pipelines/52336817 -slow 
> cool, will start to get into this hopefully end of today, thanks alot
> Noel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Some-review-fixups.patch
Type: text/x-patch
Size: 2505 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20190322/284693e5/0001-Some-review-fixups.bin>