I'm ok with patch dcb5720ad01fabac300da478368c140e2b6313d2.

Patch fd06cf379ded801adf830499c24875a7c60280be doesn't make sense to me.

Inside smbldap_search_ext() we setup an alarm timeout for the local process using:

        int             attempts = 0;
        time_t          endtime = time_mono(NULL)+lp_ldap_timeout();

        got_alarm = 0;
        CatchSignal(SIGALRM, gotalarm_sig);
        alarm_timer = lp_ldap_timeout();
        if (alarm_timer > 0) {
                alarm_timer += 2;
        }
        alarm(alarm_timer);

Then do the LDAP call using:

        while (another_ldap_try(ldap_state, &rc, &attempts, endtime)) {
               .... do stuff....
        }

Then terminate the alarm using:

        /* Teardown timeout. */
        CatchSignal(SIGALRM, SIG_IGN);
        alarm(0);

However - look inside the function another_ldap_try(). Note that inside smbldap_search_ext() we *always* call this initially with attempts == 0. We have:

        time_t now = time_mono(NULL);

        if (*attempts == 0) {
                got_alarm = False;
                old_handler = CatchSignal(SIGALRM, gotalarm_sig);
                alarm(endtime - now);

                if (ldap_state->pid != sys_getpid())
                        smbldap_close(ldap_state);
        }

This means we immediately overwrite the alarm we carefully set of "alarm_timer += 2" (which is equivalent to lp_ldap_timeout() + 2) seconds with a new alarm of "endtime - now" seconds. endtime is calculated inside smbldap_search_ext() to be time_mono(NULL)+lp_ldap_timeout(); Calls to alarm() don't stack - the next call overwrites the previous alarm, so the only one that is ever used is 

So I don't see how the alarm of "lp_ldap_timeout() + 2" seconds in this patch is ever used - it's immediately set to "endtime - now" == "time_mono(NULL)+lp_ldap_timeout() -  time_mono(NULL)" == lp_ldap_timeout()

All other calls to :

        while (another_ldap_try(ldap_state, &rc, &attempts, endtime)) {
               .... do stuff....
        }

inside the file source3/lib/smbldap.c omit this setup call to alarm(alarm_timer), leaving it to the code inside another_ldap_try(). I'm going to change this code inside master, and add an updated patch for 3.6.1 to this bug report for Bjorn to evaluate.

Jeremy.

Also, calling another_ldap_try() has a side effect in that it leaves a dangling alarm pending, which isn't always canceled when an LDAP operation called inside an:

        while (another_ldap_try(ldap_state, &rc, &attempts, endtime)) {
               .... do stuff....
        }

loop succeeds. The function can exit with a alarm still active - all functions using the code:

        while (another_ldap_try(ldap_state, &rc, &attempts, endtime)) {
               .... do stuff....
        }

should terminate the alarm on exit. I'll fix this in master.

Jeremy.

Created attachment 6793 [details]
git-am fix for 3.6.1

Bjorn - here is a patchset containing the complete set of changes (yours and mine) that have gone into master, back-ported for 3.6.1. Please test against your LDAP setup and let me know if this works for you.

Thanks,

Jeremy.

Re-assigning to Björn.

Comment on attachment 6793 [details]
git-am fix for 3.6.1

it works for me but it's hard to test corner and failure cases. The changes are so huge, that I'd propose to have another developer review it before getting this to 3.6. Volker, can you have another look at this?

Putting this on my long todo list. Those changes are really intrusive. I am not certain that I like that for a stable release stream. I might review it later when I find time.

The total effect looks intrusive, but if you look carefully it's broken into individual micro-commits that are easily understood.

They fix a long-standing problem we have had with leaving dangling alarm signals (which we *probably* ignore, but who can tell, right ? :-).

Jeremy.

Comment on attachment 6793 [details]
git-am fix for 3.6.1

Not giving an ack for this one. It contains far too many not strictly required refactorings for my taste. Also not blocking it, please find someone else to ack it.

I though it would be a good idea to tripple check this, maybe this isn't needed. Jeremy I leave the decision up to you, whether or not we should wait for another review for this.

Comment on attachment 6793 [details]
git-am fix for 3.6.1

Metze - please review for 3.6.1.
Thanks !
Jeremy.

3.6 is not supported anymore. It was fixed with 4.0.0