git ref fd06cf379ded801adf830499c24875a7c60280be in master - should be in 3.6.1 ?

[Jeremy Allison]

On Thu, Aug 18, 2011 at 08:17:30PM -0400, simo wrote:
> On Thu, 2011-08-18 at 17:02 -0700, Jeremy Allison wrote:
> > Hi Bjorn,
> > 
> > This fix :
> > 
> > commit fd06cf379ded801adf830499c24875a7c60280be
> > Author: Björn Jacke <bj at sernet.de>
> > Date:   Thu Aug 4 16:42:37 2011 +0200
> > 
> >     s3/ldap: delay the ldap search alarm termination a bit
> > 
> >     do the alarm termination of the the ldap search a bit delayed so the LDAP
> >     server has a chance to tell us that the time limit was reached and the
> >     search was abandoned. If the search is terminated this way we also get
> >     the correct LDAP return code in the logs. If alarm() stops the search the ldap
> >     search routine will report that the LDAP server is down which would trigger us
> >     to rebind to the server needlessly which we also want to avoid.
> > 
> > 
> > looks like it should be something we have in 3.6.1. If you agree
> > can you create a bug so we can get it back ported ?
> 
> I am not sure I understand the point of this code.
> So if I set a custom ldap timeout I have to know that I shoud set it
> actually 2 seconds shorter ? Seem kinda wrong to me.
> 
> How do we know that lp_ldap_timeout() is in any way related to the
> server operation timeout configuration ?

This patch doesn't work for another reason. Here's the message I added
to the bug report.

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().

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.

Jeremy.