[SCM] Samba Shared Repository - branch master updated

Jeremy Allison jra at samba.org
Sat Aug 20 12:10:02 MDT 2011


The branch, master has been updated
       via  e25345a Ensure we never wait past absolute entime to do a get_cached_ldap_connect().
       via  6c3779c Remove the tortured logic in another_ldap_try() and turn it into get_cached_ldap_connect(), which much better describes it's function.
       via  ce8a1a2 Factor out the ldap_get_option calls into a function.
       via  832dce1 Simplify the logic on the another_ldap_try() loops by breaking early out of the loop on success.
       via  03bdb26 Move the alarm setup/teardown out of another_ldap_try() and into separate functions that bracket the another_ldap_try() loop. We now never leave a dangling alarm pending on success.
       via  a357d04 Allow the timeout pointer to ldap_search_ext_s() to be NULL if lp_ldap_timeout() == 0.
       via  da89f7e Make it clear the time here is an absolute endtime. Don't set the alarm if the LDAP timeout is zero.
       via  f00c6c8 Always remove the alarm before changing the handler, not the other way around.
       via  ec07aca Remove old_handler as alarms can't be nested. Use SIG_IGN instead.
       via  7cbcbee Change got_alarm from bool to the correct type of SIG_ATOMIC_T.
       via  92a655d If "ldap timeout" is non-zero, set the local search timeout to be one second longer than the remote search timeout (which is set to the "ldap timeout" value). This allows the remote search timeout to fire in preference.
      from  f6e3484 Re-arrange the optimization to reduce tdb fcntl calls if smbd is not clustered. procid_is_me() is much cheaper to test and can optimize up to 50% of the calls to serverid_exists(). Volker please check.

http://gitweb.samba.org/?p=samba.git;a=shortlog;h=master


- Log -----------------------------------------------------------------
commit e25345a7a68e93626066d83185ed1944f6bdd044
Author: Jeremy Allison <jra at samba.org>
Date:   Sat Aug 20 09:37:04 2011 -0700

    Ensure we never wait past absolute entime to do a get_cached_ldap_connect().
    
    Autobuild-User: Jeremy Allison <jra at samba.org>
    Autobuild-Date: Sat Aug 20 20:09:37 CEST 2011 on sn-devel-104

commit 6c3779c80bd2d657a88fb030e16dee10dca45f36
Author: Jeremy Allison <jra at samba.org>
Date:   Fri Aug 19 21:40:54 2011 -0700

    Remove the tortured logic in another_ldap_try() and turn it into
    get_cached_ldap_connect(), which much better describes it's function.
    
    Now we always break at the right places in the loop, we can replace
    the while (another_ldap_try(ldap_state, &rc, &attempts, abs_endtime))
    construct with simply while (1).

commit ce8a1a29ad195add38908785e7a2f4ddec3235ba
Author: Jeremy Allison <jra at samba.org>
Date:   Fri Aug 19 21:30:04 2011 -0700

    Factor out the ldap_get_option calls into a function.

commit 832dce1a0a3a1055d5536843101ae78b65d63cb3
Author: Jeremy Allison <jra at samba.org>
Date:   Fri Aug 19 21:27:18 2011 -0700

    Simplify the logic on the another_ldap_try() loops by breaking
    early out of the loop on success.

commit 03bdb26c5bf4e08d960e243d8a258e6f077a105e
Author: Jeremy Allison <jra at samba.org>
Date:   Fri Aug 19 21:19:28 2011 -0700

    Move the alarm setup/teardown out of another_ldap_try() and into separate
    functions that bracket the another_ldap_try() loop. We now never leave a
    dangling alarm pending on success.

commit a357d044d4484ed89e2cee0ddfafae7b2f128d97
Author: Jeremy Allison <jra at samba.org>
Date:   Fri Aug 19 21:08:10 2011 -0700

    Allow the timeout pointer to ldap_search_ext_s() to be NULL if lp_ldap_timeout() == 0.

commit da89f7e24c3f3c95f397c53dabf85f4156f5d708
Author: Jeremy Allison <jra at samba.org>
Date:   Fri Aug 19 21:01:49 2011 -0700

    Make it clear the time here is an absolute endtime. Don't set the alarm if the LDAP timeout is zero.

commit f00c6c85612d954ceeb9fd9fa37c91fabd923f6d
Author: Jeremy Allison <jra at samba.org>
Date:   Fri Aug 19 20:55:41 2011 -0700

    Always remove the alarm before changing the handler, not the other way around.

commit ec07aca7d5257d997137b1771593a1e1ead51534
Author: Jeremy Allison <jra at samba.org>
Date:   Fri Aug 19 20:54:41 2011 -0700

    Remove old_handler as alarms can't be nested. Use SIG_IGN instead.

commit 7cbcbee484e2f4c4a2bb6a8c14cef86508f53a69
Author: Jeremy Allison <jra at samba.org>
Date:   Fri Aug 19 20:53:49 2011 -0700

    Change got_alarm from bool to the correct type of SIG_ATOMIC_T.

commit 92a655da8633ab99c2a3fa75b3a55ffd8b4ef430
Author: Jeremy Allison <jra at samba.org>
Date:   Fri Aug 19 18:43:51 2011 -0700

    If "ldap timeout" is non-zero, set the local search timeout to
    be one second longer than the remote search timeout (which is
    set to the "ldap timeout" value). This allows the remote search
    timeout to fire in preference.
    
    Allow lp_ldap_timeout() to be zero. Don't set the any local alarm
    if so.

-----------------------------------------------------------------------

Summary of changes:
 source3/lib/smbldap.c |  420 +++++++++++++++++++++++++++----------------------
 source3/libads/ldap.c |   55 +++++---
 2 files changed, 266 insertions(+), 209 deletions(-)


Changeset truncated at 500 lines:

diff --git a/source3/lib/smbldap.c b/source3/lib/smbldap.c
index dcb99c9..b333f30 100644
--- a/source3/lib/smbldap.c
+++ b/source3/lib/smbldap.c
@@ -1345,77 +1345,113 @@ static NTSTATUS smbldap_close(struct smbldap_state *ldap_state)
 	return NT_STATUS_OK;
 }
 
-static bool got_alarm;
-
-static void (*old_handler)(int);
+static SIG_ATOMIC_T got_alarm;
 
 static void gotalarm_sig(int dummy)
 {
-	got_alarm = True;
+	got_alarm = 1;
 }
 
-static int another_ldap_try(struct smbldap_state *ldap_state, int *rc,
-			    int *attempts, time_t endtime)
+static time_t calc_ldap_abs_endtime(int ldap_to)
+{
+	if (ldap_to == 0) {
+		/* No timeout - don't
+		   return a value for
+		   the alarm. */
+		return (time_t)0;
+	}
+
+	/* Make the alarm time one second beyond
+	   the timout we're setting for the
+	   remote search timeout, to allow that
+	   to fire in preference. */
+
+	return time_mono(NULL)+ldap_to+1;
+}
+
+static int end_ldap_local_alarm(time_t absolute_endtime, int rc)
+{
+	if (absolute_endtime) {
+		alarm(0);
+		CatchSignal(SIGALRM, SIG_IGN);
+		if (got_alarm) {
+			/* Client timeout error code. */
+			got_alarm = 0;
+			return LDAP_TIMEOUT;
+		}
+	}
+	return rc;
+}
+
+static void setup_ldap_local_alarm(struct smbldap_state *ldap_state, time_t absolute_endtime)
 {
 	time_t now = time_mono(NULL);
-	int open_rc = LDAP_SERVER_DOWN;
 
-	if (*rc != LDAP_SERVER_DOWN)
-		goto no_next;
+	if (absolute_endtime) {
+		got_alarm = 0;
+		CatchSignal(SIGALRM, gotalarm_sig);
+		alarm(absolute_endtime - now);
+	}
 
-	if (now >= endtime) {
+	if (ldap_state->pid != sys_getpid()) {
 		smbldap_close(ldap_state);
-		*rc = LDAP_TIMEOUT;
-		goto no_next;
 	}
+}
+
+static void get_ldap_errs(struct smbldap_state *ldap_state, char **pp_ld_error, int *p_ld_errno)
+{
+	ldap_get_option(ldap_state->ldap_struct,
+			LDAP_OPT_ERROR_NUMBER, p_ld_errno);
 
-	if (*attempts == 0) {
-		got_alarm = False;
-		old_handler = CatchSignal(SIGALRM, gotalarm_sig);
-		alarm(endtime - now);
+	ldap_get_option(ldap_state->ldap_struct,
+			LDAP_OPT_ERROR_STRING, pp_ld_error);
+}
 
-		if (ldap_state->pid != sys_getpid())
-			smbldap_close(ldap_state);
-	}
+static int get_cached_ldap_connect(struct smbldap_state *ldap_state, time_t abs_endtime)
+{
+	int attempts = 0;
 
 	while (1) {
+		int rc;
+		time_t now;
 
-		if (*attempts != 0)
-			smb_msleep(1000);
+		now = time_mono(NULL);
+		ldap_state->last_use = now;
 
-		*attempts += 1;
+		if (abs_endtime && now > abs_endtime) {
+			smbldap_close(ldap_state);
+			return LDAP_TIMEOUT;
+		}
 
-		open_rc = smbldap_open(ldap_state);
+		rc = smbldap_open(ldap_state);
 
-		if (open_rc == LDAP_SUCCESS) {
-			ldap_state->last_use = now;
-			return True;
+		if (rc == LDAP_SUCCESS) {
+			return LDAP_SUCCESS;
 		}
 
-		if (open_rc == LDAP_INSUFFICIENT_ACCESS) {
+		attempts++;
+		DEBUG(1, ("Connection to LDAP server failed for the "
+			"%d try!\n", attempts));
+
+		if (rc == LDAP_INSUFFICIENT_ACCESS) {
 			/* The fact that we are non-root or any other
 			 * access-denied condition will not change in the next
 			 * round of trying */
-			*rc = open_rc;
-			break;
+			return rc;
 		}
 
 		if (got_alarm) {
-			*rc = LDAP_TIMEOUT;
-			break;
+			smbldap_close(ldap_state);
+			return LDAP_TIMEOUT;
 		}
 
-		if (open_rc != LDAP_SUCCESS) {
-			DEBUG(1, ("Connection to LDAP server failed for the "
-				  "%d try!\n", *attempts));
+		smb_msleep(1000);
+
+		if (got_alarm) {
+			smbldap_close(ldap_state);
+			return LDAP_TIMEOUT;
 		}
 	}
-
- no_next:
-	CatchSignal(SIGALRM, old_handler);
-	alarm(0);
-	ldap_state->last_use = now;
-	return False;
 }
 
 /*********************************************************************
@@ -1428,11 +1464,11 @@ static int smbldap_search_ext(struct smbldap_state *ldap_state,
 			      int sizelimit, LDAPMessage **res)
 {
 	int 		rc = LDAP_SERVER_DOWN;
-	int 		attempts = 0;
 	char           *utf8_filter;
-	time_t		endtime = time_mono(NULL)+lp_ldap_timeout();
+	int		to = lp_ldap_timeout();
+	time_t		abs_endtime = calc_ldap_abs_endtime(to);
 	struct		timeval timeout;
-	int		alarm_timer;
+	struct		timeval *timeout_ptr = NULL;
 	size_t		converted_size;
 
 	SMB_ASSERT(ldap_state);
@@ -1469,70 +1505,50 @@ static int smbldap_search_ext(struct smbldap_state *ldap_state,
 		return LDAP_NO_MEMORY;
 	}
 
-	/* Setup timeout for the ldap_search_ext_s call - local and remote. */
-	timeout.tv_sec = lp_ldap_timeout();
-	timeout.tv_usec = 0;
-
-	/* Setup alarm timeout.... Do we need both of these ? JRA.
-	 * Yes, I think we do need both of these. The server timeout only
-	 * covers the case where the server's operation takes too long. It
-	 * does not cover the case where the request hangs on its way to the
-	 * server. The server side timeout is not strictly necessary, it's
-	 * just a bit more kind to the server. VL.
-	 * We prefer to get the server termination though because otherwise we
-	 * have to rebind to the LDAP server aѕ we get a LDAP_SERVER_DOWN in the
-	 * alarm termination case. So let's call the alarm 2s later, which
-	 * should be enough for the server to report the timelimit exceeded.
-	 * in case the ldal timeout is 0 (no limit) we set the _no_ alarm
-	 * accordingly. BJ. */
-
-	got_alarm = 0;
-	CatchSignal(SIGALRM, gotalarm_sig);
-	alarm_timer = lp_ldap_timeout();
-	if (alarm_timer > 0) {
-		alarm_timer += 2;
-	}
-	alarm(alarm_timer);
-	/* End setup timeout. */
-
-	while (another_ldap_try(ldap_state, &rc, &attempts, endtime)) {
+	/* Setup remote timeout for the ldap_search_ext_s call. */
+	if (to) {
+		timeout.tv_sec = to;
+		timeout.tv_usec = 0;
+		timeout_ptr = &timeout;
+	}
+
+	setup_ldap_local_alarm(ldap_state, abs_endtime);
+
+	while (1) {
+		char *ld_error = NULL;
+		int ld_errno;
+
+		rc = get_cached_ldap_connect(ldap_state, abs_endtime);
+		if (rc != LDAP_SUCCESS) {
+			break;
+		}
+
 		rc = ldap_search_ext_s(ldap_state->ldap_struct, base, scope, 
 				       utf8_filter,
 				       discard_const_p(char *, attrs),
-				       attrsonly, sctrls, cctrls, &timeout,
+				       attrsonly, sctrls, cctrls, timeout_ptr,
 				       sizelimit, res);
-		if (rc != LDAP_SUCCESS) {
-			char *ld_error = NULL;
-			int ld_errno;
-
-			ldap_get_option(ldap_state->ldap_struct,
-					LDAP_OPT_ERROR_NUMBER, &ld_errno);
-
-			ldap_get_option(ldap_state->ldap_struct,
-					LDAP_OPT_ERROR_STRING, &ld_error);
-			DEBUG(10, ("Failed search for base: %s, error: %d (%s) "
-				   "(%s)\n", base, ld_errno,
-				   ldap_err2string(rc),
-				   ld_error ? ld_error : "unknown"));
-			SAFE_FREE(ld_error);
-
-			if (ld_errno == LDAP_SERVER_DOWN) {
-				ldap_unbind(ldap_state->ldap_struct);
-				ldap_state->ldap_struct = NULL;
-			}
+		if (rc == LDAP_SUCCESS) {
+			break;
 		}
-	}
 
-	TALLOC_FREE(utf8_filter);
+		get_ldap_errs(ldap_state, &ld_error, &ld_errno);
 
-	/* Teardown timeout. */
-	CatchSignal(SIGALRM, SIG_IGN);
-	alarm(0);
+		DEBUG(10, ("Failed search for base: %s, error: %d (%s) "
+			   "(%s)\n", base, ld_errno,
+			   ldap_err2string(rc),
+			   ld_error ? ld_error : "unknown"));
+		SAFE_FREE(ld_error);
 
-	if (got_alarm != 0)
-		return LDAP_TIMELIMIT_EXCEEDED;
+		if (ld_errno != LDAP_SERVER_DOWN) {
+			break;
+		}
+		ldap_unbind(ldap_state->ldap_struct);
+		ldap_state->ldap_struct = NULL;
+	}
 
-	return rc;
+	TALLOC_FREE(utf8_filter);
+	return end_ldap_local_alarm(abs_endtime, rc);
 }
 
 int smbldap_search(struct smbldap_state *ldap_state, 
@@ -1638,9 +1654,8 @@ done:
 int smbldap_modify(struct smbldap_state *ldap_state, const char *dn, LDAPMod *attrs[])
 {
 	int 		rc = LDAP_SERVER_DOWN;
-	int 		attempts = 0;
 	char           *utf8_dn;
-	time_t		endtime = time_mono(NULL)+lp_ldap_timeout();
+	time_t		abs_endtime = calc_ldap_abs_endtime(lp_ldap_timeout());
 	size_t		converted_size;
 
 	SMB_ASSERT(ldap_state);
@@ -1651,40 +1666,46 @@ int smbldap_modify(struct smbldap_state *ldap_state, const char *dn, LDAPMod *at
 		return LDAP_NO_MEMORY;
 	}
 
-	while (another_ldap_try(ldap_state, &rc, &attempts, endtime)) {
-		rc = ldap_modify_s(ldap_state->ldap_struct, utf8_dn, attrs);
+	setup_ldap_local_alarm(ldap_state, abs_endtime);
+
+	while (1) {
+		char *ld_error = NULL;
+		int ld_errno;
+
+		rc = get_cached_ldap_connect(ldap_state, abs_endtime);
 		if (rc != LDAP_SUCCESS) {
-			char *ld_error = NULL;
-			int ld_errno;
-
-			ldap_get_option(ldap_state->ldap_struct,
-					LDAP_OPT_ERROR_NUMBER, &ld_errno);
-
-			ldap_get_option(ldap_state->ldap_struct,
-					LDAP_OPT_ERROR_STRING, &ld_error);
-			DEBUG(10, ("Failed to modify dn: %s, error: %d (%s) "
-				   "(%s)\n", dn, ld_errno,
-				   ldap_err2string(rc),
-				   ld_error ? ld_error : "unknown"));
-			SAFE_FREE(ld_error);
-
-			if (ld_errno == LDAP_SERVER_DOWN) {
-				ldap_unbind(ldap_state->ldap_struct);
-				ldap_state->ldap_struct = NULL;
-			}
+			break;
+		}
+
+		rc = ldap_modify_s(ldap_state->ldap_struct, utf8_dn, attrs);
+		if (rc == LDAP_SUCCESS) {
+			break;
 		}
+
+		get_ldap_errs(ldap_state, &ld_error, &ld_errno);
+
+		DEBUG(10, ("Failed to modify dn: %s, error: %d (%s) "
+			   "(%s)\n", dn, ld_errno,
+			   ldap_err2string(rc),
+			   ld_error ? ld_error : "unknown"));
+		SAFE_FREE(ld_error);
+
+		if (ld_errno != LDAP_SERVER_DOWN) {
+			break;
+		}
+		ldap_unbind(ldap_state->ldap_struct);
+		ldap_state->ldap_struct = NULL;
 	}
 
 	TALLOC_FREE(utf8_dn);
-	return rc;
+	return end_ldap_local_alarm(abs_endtime, rc);
 }
 
 int smbldap_add(struct smbldap_state *ldap_state, const char *dn, LDAPMod *attrs[])
 {
 	int 		rc = LDAP_SERVER_DOWN;
-	int 		attempts = 0;
 	char           *utf8_dn;
-	time_t		endtime = time_mono(NULL)+lp_ldap_timeout();
+	time_t		abs_endtime = calc_ldap_abs_endtime(lp_ldap_timeout());
 	size_t		converted_size;
 
 	SMB_ASSERT(ldap_state);
@@ -1695,40 +1716,46 @@ int smbldap_add(struct smbldap_state *ldap_state, const char *dn, LDAPMod *attrs
 		return LDAP_NO_MEMORY;
 	}
 
-	while (another_ldap_try(ldap_state, &rc, &attempts, endtime)) {
-		rc = ldap_add_s(ldap_state->ldap_struct, utf8_dn, attrs);
+	setup_ldap_local_alarm(ldap_state, abs_endtime);
+
+	while (1) {
+		char *ld_error = NULL;
+		int ld_errno;
+
+		rc = get_cached_ldap_connect(ldap_state, abs_endtime);
 		if (rc != LDAP_SUCCESS) {
-			char *ld_error = NULL;
-			int ld_errno;
-
-			ldap_get_option(ldap_state->ldap_struct,
-					LDAP_OPT_ERROR_NUMBER, &ld_errno);
-
-			ldap_get_option(ldap_state->ldap_struct,
-					LDAP_OPT_ERROR_STRING, &ld_error);
-			DEBUG(10, ("Failed to add dn: %s, error: %d (%s) "
-				   "(%s)\n", dn, ld_errno,
-				   ldap_err2string(rc),
-				   ld_error ? ld_error : "unknown"));
-			SAFE_FREE(ld_error);
-
-			if (ld_errno == LDAP_SERVER_DOWN) {
-				ldap_unbind(ldap_state->ldap_struct);
-				ldap_state->ldap_struct = NULL;
-			}
+			break;
+		}
+
+		rc = ldap_add_s(ldap_state->ldap_struct, utf8_dn, attrs);
+		if (rc == LDAP_SUCCESS) {
+			break;
 		}
+
+		get_ldap_errs(ldap_state, &ld_error, &ld_errno);
+
+		DEBUG(10, ("Failed to add dn: %s, error: %d (%s) "
+			   "(%s)\n", dn, ld_errno,
+			   ldap_err2string(rc),
+			   ld_error ? ld_error : "unknown"));
+		SAFE_FREE(ld_error);
+
+		if (ld_errno != LDAP_SERVER_DOWN) {
+			break;
+		}
+		ldap_unbind(ldap_state->ldap_struct);
+		ldap_state->ldap_struct = NULL;
 	}
 
 	TALLOC_FREE(utf8_dn);
-	return rc;
+	return end_ldap_local_alarm(abs_endtime, rc);
 }
 
 int smbldap_delete(struct smbldap_state *ldap_state, const char *dn)
 {
 	int 		rc = LDAP_SERVER_DOWN;
-	int 		attempts = 0;
 	char           *utf8_dn;
-	time_t		endtime = time_mono(NULL)+lp_ldap_timeout();
+	time_t		abs_endtime = calc_ldap_abs_endtime(lp_ldap_timeout());
 	size_t		converted_size;
 
 	SMB_ASSERT(ldap_state);
@@ -1739,32 +1766,39 @@ int smbldap_delete(struct smbldap_state *ldap_state, const char *dn)
 		return LDAP_NO_MEMORY;
 	}
 
-	while (another_ldap_try(ldap_state, &rc, &attempts, endtime)) {
-		rc = ldap_delete_s(ldap_state->ldap_struct, utf8_dn);
+	setup_ldap_local_alarm(ldap_state, abs_endtime);
+
+	while (1) {
+		char *ld_error = NULL;
+		int ld_errno;
+
+		rc = get_cached_ldap_connect(ldap_state, abs_endtime);
 		if (rc != LDAP_SUCCESS) {
-			char *ld_error = NULL;
-			int ld_errno;
-
-			ldap_get_option(ldap_state->ldap_struct,
-					LDAP_OPT_ERROR_NUMBER, &ld_errno);
-
-			ldap_get_option(ldap_state->ldap_struct,
-					LDAP_OPT_ERROR_STRING, &ld_error);
-			DEBUG(10, ("Failed to delete dn: %s, error: %d (%s) "
-				   "(%s)\n", dn, ld_errno,
-				   ldap_err2string(rc),
-				   ld_error ? ld_error : "unknown"));
-			SAFE_FREE(ld_error);
-
-			if (ld_errno == LDAP_SERVER_DOWN) {
-				ldap_unbind(ldap_state->ldap_struct);
-				ldap_state->ldap_struct = NULL;
-			}
+			break;
 		}
+
+		rc = ldap_delete_s(ldap_state->ldap_struct, utf8_dn);
+		if (rc == LDAP_SUCCESS) {
+			break;
+		}
+
+		get_ldap_errs(ldap_state, &ld_error, &ld_errno);
+
+		DEBUG(10, ("Failed to delete dn: %s, error: %d (%s) "
+			   "(%s)\n", dn, ld_errno,
+			   ldap_err2string(rc),
+			   ld_error ? ld_error : "unknown"));
+		SAFE_FREE(ld_error);
+
+		if (ld_errno != LDAP_SERVER_DOWN) {
+			break;
+		}
+		ldap_unbind(ldap_state->ldap_struct);
+		ldap_state->ldap_struct = NULL;
 	}
 
 	TALLOC_FREE(utf8_dn);
-	return rc;
+	return end_ldap_local_alarm(abs_endtime, rc);
 }
 
 int smbldap_extended_operation(struct smbldap_state *ldap_state, 
@@ -1773,39 +1807,45 @@ int smbldap_extended_operation(struct smbldap_state *ldap_state,
 			       char **retoidp, struct berval **retdatap)
 {
 	int 		rc = LDAP_SERVER_DOWN;
-	int 		attempts = 0;
-	time_t		endtime = time_mono(NULL)+lp_ldap_timeout();
+	time_t		abs_endtime = calc_ldap_abs_endtime(lp_ldap_timeout());
 
 	if (!ldap_state)


-- 
Samba Shared Repository


More information about the samba-cvs mailing list