[PATCH] tevent-glib-glue

[Noel Power]

Hi Ralph
On 22/03/2019 15:21, Ralph Böhme wrote:
> Hi Noel,
>
> On Fri, Mar 22, 2019 at 11:08:33AM +0000, Noel Power wrote:
>> 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.
>
> Thanks a *lot*!
>
>> + 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)
>
> ah, drat, retrofitting something is a recipe for disaster. :( Fixed in
> attached patchset.
this stuff is tricky :-)
>
>> + '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
>
> Also fixed by using an explicit state flag in the glue.
yes, I tried to avoid another tricky flag by suggesting using
tevent_glib_process, however that was probably incorrect if you want to
ensure just one event per loop iteration (I forgot that this was a goal)

>> 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
>
> Hm, works here.
I believe you!! I will have to figure out why this doesn't run here, it
must be something specific to my environment as it clearly runs in the CI
>
>> 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.
>
> Yeah, good point. I've changed the test runner script to mark the test
> skipped if not run.
>
> Also, tevent_glib_glue is now only dependent on glib2-devel, not
> --enable-spotlight. Hopefully we can get glib2-devel in the gitlab
> image, I've requested this via the mailing list.
super
>
> All changes are in the FIXUP commits, other commit are unchanged
> compared to the previous iteration. 


I find the do_glib_refresh checking a bit confusing


to me

    if (point == TEVENT_TRACE_AFTER_LOOP_ONCE) {
        if (glue->skip_glib_refresh == false) {
            tevent_glib_prepare(glue);
    }
}

or

    if (point == TEVENT_TRACE_AFTER_LOOP_ONCE &&
        glue->skip_glib_refresh == false) {
        tevent_glib_prepare(glue);
    }

is much simpler to read then

    bool do_glib_refresh = false;

    if (point == TEVENT_TRACE_AFTER_LOOP_ONCE) {
        do_glib_refresh = true;
    }
    if (glue->skip_glib_refresh) {
        /*
         * Don't call tevent_glib_prepare() if explicity told so. This
         * is an optimisation for the case that glib event sources are
         * created from glib event callbacks.
         */
        do_glib_refresh = false;
    }
    if (do_glib_refresh) {
        tevent_glib_prepare(glue);
    }

Also I'd feel safer if glue->skip_glib_refresh was reset inside the 'if
(point == TEVENT_TRACE_AFTER_LOOP_ONCE) ' check, maybe I am being
paranoid but I am not sure for example what way the trace event order
might be if for example the discouraged (but still used) nested tevent
processing happens

I had a look at the remainder mdssvc changes so

[PATCH 7/9] s3-mdssvc: add tevent context arg to mds_init_ctx

[PATCH 8/9] s3-mdssvc: call [un]become_authenticated_pipe_user()

af as I understand the changes these look good, the
'become_authenticated_pipe_user' would have been necessary anyway 
regardless of the glue changes. right ?

[[PATCH 9/9] s3-mdssvc: use tevent_glib_glue in mdssvc RPC service

These all look fine to me except I confess that I don't understand the
reason for using g_main_context_new() rather than the default context
and then additional use of g_main_context_push_thread_default() From
what I read I understood this is something you may want to do in a
multithreaded environment to ensure gio threads for example to deliver
events to thread default context (and associated main loop) rather than
the global default context. If we are not multitheaded should we just
not use the default global context anyway (and avoid
g_main_context_push/pop_thread_default context calls) To be honest this
stuff is pretty unfamiliar to me so I actually have no clue :-)

oh and there is a minor typo there in the comments 'gioi worker threads'
instead of 'gio worker threads'

so all and all this imho is looking pretty good

Noel