From 229f61f5f54aeb9e1a1756f731dfe7bcedbf148c Mon Sep 17 00:00:00 2001

From: Mark Reynolds <mreynolds@redhat.com>

Date: Fri, 13 Oct 2017 07:09:08 -0400

Subject: [PATCH 06/10] Ticket 48235 - Remove memberOf global lock

Bug Description:  The memberOf global lock no longer servers a purpose since

                  the plugin is BETXN.  This was causing potential deadlocks

                  when multiple backends are used.

Fix Description:  Remove the lock, and rework the fixup/ancestors caches/hashtables.

                  Instead of reusing a single cache, we create a fresh cache

                  when we copy the plugin config (which only happens at the start

                  of an operation).  Then we destroy the caches when we free

                  the config.

https://pagure.io/389-ds-base/issue/48235

Reviewed by: tbordaz & firstyear(Thanks!!)

---

 ldap/servers/plugins/memberof/memberof.c        | 312 +++---------------------

 ldap/servers/plugins/memberof/memberof.h        |  17 ++

 ldap/servers/plugins/memberof/memberof_config.c | 152 +++++++++++-

 3 files changed, 200 insertions(+), 281 deletions(-)

diff --git a/ldap/servers/plugins/memberof/memberof.c b/ldap/servers/plugins/memberof/memberof.c

index 9bbe13c9c..bbf47dd49 100644

--- a/ldap/servers/plugins/memberof/memberof.c

+++ b/ldap/servers/plugins/memberof/memberof.c

@@ -49,13 +49,10 @@ static void* _PluginID = NULL;

 static Slapi_DN* _ConfigAreaDN = NULL;

 static Slapi_RWLock *config_rwlock = NULL;

 static Slapi_DN* _pluginDN = NULL;

-static PRMonitor *memberof_operation_lock = 0;

 MemberOfConfig *qsortConfig = 0;

 static int usetxn = 0;

 static int premodfn = 0;

-#define MEMBEROF_HASHTABLE_SIZE 1000

-static PLHashTable *fixup_entry_hashtable = NULL; /* global hash table protected by memberof_lock (memberof_operation_lock) */

-static PLHashTable *group_ancestors_hashtable = NULL; /* global hash table protected by memberof_lock (memberof_operation_lock) */

+

 typedef struct _memberofstringll

 {

@@ -73,18 +70,7 @@ typedef struct _memberof_get_groups_data

         PRBool use_cache;

 } memberof_get_groups_data;

-/* The key to access the hash table is the normalized DN

- * The normalized DN is stored in the value because:

- *  - It is used in slapi_valueset_find

- *  - It is used to fill the memberof_get_groups_data.group_norm_vals

- */

-typedef struct _memberof_cached_value

-{

-	char *key;

-	char *group_dn_val;

-        char *group_ndn_val;

-	int valid;

-} memberof_cached_value;

+

 struct cache_stat

 {

 	int total_lookup;

@@ -189,14 +175,9 @@ static int memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data);

 static int memberof_entry_in_scope(MemberOfConfig *config, Slapi_DN *sdn);

 static int memberof_add_objectclass(char *auto_add_oc, const char *dn);

 static int memberof_add_memberof_attr(LDAPMod **mods, const char *dn, char *add_oc);

-static PLHashTable *hashtable_new();

-static void fixup_hashtable_empty(char *msg);

-static PLHashTable *hashtable_new();

-static void ancestor_hashtable_empty(char *msg);

-static void ancestor_hashtable_entry_free(memberof_cached_value *entry);

-static memberof_cached_value *ancestors_cache_lookup(const char *ndn);

-static PRBool ancestors_cache_remove(const char *ndn);

-static PLHashEntry *ancestors_cache_add(const void *key, void *value);

+static memberof_cached_value *ancestors_cache_lookup(MemberOfConfig *config, const char *ndn);

+static PRBool ancestors_cache_remove(MemberOfConfig *config, const char *ndn);

+static PLHashEntry *ancestors_cache_add(MemberOfConfig *config, const void *key, void *value);

 /*** implementation ***/

@@ -375,12 +356,6 @@ int memberof_postop_start(Slapi_PBlock *pb)

 	slapi_log_err(SLAPI_LOG_TRACE, MEMBEROF_PLUGIN_SUBSYSTEM,

 		"--> memberof_postop_start\n" );

-	memberof_operation_lock = PR_NewMonitor();

-	if(0 == memberof_operation_lock)

-	{

-		rc = -1;

-		goto bail;

-	}

 	if(config_rwlock == NULL){

 		if((config_rwlock = slapi_new_rwlock()) == NULL){

 			rc = -1;

@@ -388,9 +363,6 @@ int memberof_postop_start(Slapi_PBlock *pb)

 		}

 	}

-	fixup_entry_hashtable = hashtable_new();

-	group_ancestors_hashtable = hashtable_new();

-

 	/* Set the alternate config area if one is defined. */

 	slapi_pblock_get(pb, SLAPI_PLUGIN_CONFIG_AREA, &config_area);

 	if (config_area)

@@ -482,18 +454,7 @@ int memberof_postop_close(Slapi_PBlock *pb)

 	slapi_sdn_free(&_pluginDN);

 	slapi_destroy_rwlock(config_rwlock);

 	config_rwlock = NULL;

-	PR_DestroyMonitor(memberof_operation_lock);

-	memberof_operation_lock = NULL;

-

-	if (fixup_entry_hashtable) {

-		fixup_hashtable_empty("memberof_postop_close empty fixup_entry_hastable");

-		PL_HashTableDestroy(fixup_entry_hashtable);

-	}

-	if (group_ancestors_hashtable) {

-		ancestor_hashtable_empty("memberof_postop_close empty group_ancestors_hashtable");

-		PL_HashTableDestroy(group_ancestors_hashtable);

-	}

 	slapi_log_err(SLAPI_LOG_TRACE, MEMBEROF_PLUGIN_SUBSYSTEM,

 		     "<-- memberof_postop_close\n" );

 	return 0;

@@ -554,7 +515,7 @@ int memberof_postop_del(Slapi_PBlock *pb)

 {

 	int ret = SLAPI_PLUGIN_SUCCESS;

 	MemberOfConfig *mainConfig = NULL;

-	MemberOfConfig configCopy = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};

+	MemberOfConfig configCopy = {0};

 	Slapi_DN *sdn;

 	void *caller_id = NULL;

@@ -583,9 +544,6 @@ int memberof_postop_del(Slapi_PBlock *pb)

 		}

 		memberof_copy_config(&configCopy, memberof_get_config());

 		memberof_unlock_config();

-

-		/* get the memberOf operation lock */

-		memberof_lock();

 		/* remove this DN from the

 		 * membership lists of groups

@@ -594,7 +552,6 @@ int memberof_postop_del(Slapi_PBlock *pb)

 			slapi_log_err(SLAPI_LOG_ERR, MEMBEROF_PLUGIN_SUBSYSTEM,

 			                "memberof_postop_del - Error deleting dn (%s) from group. Error (%d)\n",

 			                slapi_sdn_get_dn(sdn),ret);

-			memberof_unlock();

 			goto bail;

 		}

@@ -618,7 +575,6 @@ int memberof_postop_del(Slapi_PBlock *pb)

 				}

 			}

 		}

-		memberof_unlock();

 bail:

 		memberof_free_config(&configCopy);

 	}

@@ -813,7 +769,7 @@ memberof_call_foreach_dn(Slapi_PBlock *pb __attribute__((unused)), Slapi_DN *sdn

 		memberof_cached_value *ht_grp = NULL;

 		const char *ndn = slapi_sdn_get_ndn(sdn);

-		ht_grp = ancestors_cache_lookup((const void *) ndn);

+		ht_grp = ancestors_cache_lookup(config, (const void *) ndn);

 		if (ht_grp) {

 #if MEMBEROF_CACHE_DEBUG

 			slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_call_foreach_dn: Ancestors of %s already cached (%x)\n", ndn, ht_grp);

@@ -960,7 +916,7 @@ int memberof_postop_modrdn(Slapi_PBlock *pb)

 	if(memberof_oktodo(pb))

 	{

 		MemberOfConfig *mainConfig = 0;

-		MemberOfConfig configCopy = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};

+		MemberOfConfig configCopy = {0};

 		struct slapi_entry *pre_e = NULL;

 		struct slapi_entry *post_e = NULL;

 		Slapi_DN *pre_sdn = 0;

@@ -988,8 +944,6 @@ int memberof_postop_modrdn(Slapi_PBlock *pb)

 			goto bail;

 		}

-		memberof_lock();

-

 		/*  update any downstream members */

 		if(pre_sdn && post_sdn && configCopy.group_filter &&

 		   0 == slapi_filter_test_simple(post_e, configCopy.group_filter))

@@ -1060,7 +1014,6 @@ int memberof_postop_modrdn(Slapi_PBlock *pb)

 				}

 			}

 		}

-		memberof_unlock();

 bail:

 		memberof_free_config(&configCopy);

 	}

@@ -1220,7 +1173,7 @@ int memberof_postop_modify(Slapi_PBlock *pb)

 	{

 		int config_copied = 0;

 		MemberOfConfig *mainConfig = 0;

-		MemberOfConfig configCopy = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};

+		MemberOfConfig configCopy = {0};

 		/* get the mod set */

 		slapi_pblock_get(pb, SLAPI_MODIFY_MODS, &mods;;

@@ -1267,8 +1220,6 @@ int memberof_postop_modify(Slapi_PBlock *pb)

 			{

 				int op = slapi_mod_get_operation(smod);

-				memberof_lock();

-

 				/* the modify op decides the function */

 				switch(op & ~LDAP_MOD_BVALUES)

 				{

@@ -1280,7 +1231,6 @@ int memberof_postop_modify(Slapi_PBlock *pb)

 								"memberof_postop_modify - Failed to add dn (%s) to target.  "

 								"Error (%d)\n", slapi_sdn_get_dn(sdn), ret );

 							slapi_mod_done(next_mod);

-							memberof_unlock();

 							goto bail;

 						}

 						break;

@@ -1299,7 +1249,6 @@ int memberof_postop_modify(Slapi_PBlock *pb)

 									"memberof_postop_modify - Failed to replace list (%s).  "

 									"Error (%d)\n", slapi_sdn_get_dn(sdn), ret );

 								slapi_mod_done(next_mod);

-								memberof_unlock();

 								goto bail;

 							}

 						}

@@ -1311,7 +1260,6 @@ int memberof_postop_modify(Slapi_PBlock *pb)

 									"memberof_postop_modify: failed to remove dn (%s).  "

 									"Error (%d)\n", slapi_sdn_get_dn(sdn), ret );

 								slapi_mod_done(next_mod);

-								memberof_unlock();

 								goto bail;

 							}

 						}

@@ -1326,7 +1274,6 @@ int memberof_postop_modify(Slapi_PBlock *pb)

 								"memberof_postop_modify - Failed to replace values in  dn (%s).  "

 								"Error (%d)\n", slapi_sdn_get_dn(sdn), ret );

 							slapi_mod_done(next_mod);

-							memberof_unlock();

 							goto bail;

 						}

 						break;

@@ -1342,8 +1289,6 @@ int memberof_postop_modify(Slapi_PBlock *pb)

 						break;

 					}

 				}

-

-				memberof_unlock();

 			}

 			slapi_mod_done(next_mod);

@@ -1398,7 +1343,7 @@ int memberof_postop_add(Slapi_PBlock *pb)

 	if(memberof_oktodo(pb) && (sdn = memberof_getsdn(pb)))

 	{

 		struct slapi_entry *e = NULL;

-		MemberOfConfig configCopy = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};

+		MemberOfConfig configCopy = {0};

 		MemberOfConfig *mainConfig;

 		slapi_pblock_get( pb, SLAPI_ENTRY_POST_OP, &e );

@@ -1424,8 +1369,6 @@ int memberof_postop_add(Slapi_PBlock *pb)

 			int i = 0;

 			Slapi_Attr *attr = 0;

-			memberof_lock();

-

 			for (i = 0; configCopy.groupattrs && configCopy.groupattrs[i]; i++)

 			{

 				if(0 == slapi_entry_attr_find(e, configCopy.groupattrs[i], &attr))

@@ -1438,8 +1381,6 @@ int memberof_postop_add(Slapi_PBlock *pb)

 					}

 				}

 			}

-

-			memberof_unlock();

 			memberof_free_config(&configCopy);

 		}

 	}

@@ -2201,7 +2142,7 @@ dump_cache_entry(memberof_cached_value *double_check, const char *msg)

  * the firsts elements of the array has 'valid=1' and the dn/ndn of group it belong to

  */

 static void

-cache_ancestors(Slapi_Value **member_ndn_val, memberof_get_groups_data *groups)

+cache_ancestors(MemberOfConfig *config, Slapi_Value **member_ndn_val, memberof_get_groups_data *groups)

 {

 	Slapi_ValueSet *groupvals = *((memberof_get_groups_data*)groups)->groupvals;

 	Slapi_Value *sval;

@@ -2298,14 +2239,14 @@ cache_ancestors(Slapi_Value **member_ndn_val, memberof_get_groups_data *groups)

 #if MEMBEROF_CACHE_DEBUG

 	dump_cache_entry(cache_entry, key);

 #endif

-	if (ancestors_cache_add((const void*) key_copy, (void *) cache_entry) == NULL) {

+	if (ancestors_cache_add(config, (const void*) key_copy, (void *) cache_entry) == NULL) {

 		slapi_log_err( SLAPI_LOG_FATAL, MEMBEROF_PLUGIN_SUBSYSTEM, "cache_ancestors: Failed to cache ancestor of %s\n", key);

 		ancestor_hashtable_entry_free(cache_entry);

 		slapi_ch_free ((void**)&cache_entry);

 		return;

 	}

 #if MEMBEROF_CACHE_DEBUG

-	if (double_check = ancestors_cache_lookup((const void*) key)) {

+	if (double_check = ancestors_cache_lookup(config, (const void*) key)) {

 		dump_cache_entry(double_check, "read back");

 	}

 #endif

@@ -2390,9 +2331,9 @@ memberof_get_groups_r(MemberOfConfig *config, Slapi_DN *member_sdn,

 		memberof_get_groups_callback, &member_data, &cached, member_data.use_cache);

 	merge_ancestors(&member_ndn_val, &member_data, data);

-	if (!cached && member_data.use_cache)

-		cache_ancestors(&member_ndn_val, &member_data);

-

+	if (!cached && member_data.use_cache) {

+		cache_ancestors(config, &member_ndn_val, &member_data);

+	}

 	slapi_value_free(&member_ndn_val);

 	slapi_valueset_free(groupvals);

@@ -2969,46 +2910,9 @@ int memberof_qsort_compare(const void *a, const void *b)

 	                                val1, val2);

 }

-/* betxn: This locking mechanism is necessary to guarantee the memberof

- * consistency */

-void memberof_lock()

-{

-	if (usetxn) {

-		PR_EnterMonitor(memberof_operation_lock);

-	}

-	if (fixup_entry_hashtable) {

-		fixup_hashtable_empty("memberof_lock");

-	}

-	if (group_ancestors_hashtable) {

-		ancestor_hashtable_empty("memberof_lock empty group_ancestors_hashtable");

-		memset(&cache_stat, 0, sizeof(cache_stat));

-	}

-}

-

-void memberof_unlock()

-{

-	if (group_ancestors_hashtable) {

-		ancestor_hashtable_empty("memberof_unlock empty group_ancestors_hashtable");

-#if MEMBEROF_CACHE_DEBUG

-		slapi_log_err(SLAPI_LOG_FATAL, MEMBEROF_PLUGIN_SUBSYSTEM, "cache statistics: total lookup %d (success %d), add %d, remove %d, enum %d\n",

-			cache_stat.total_lookup, cache_stat.successfull_lookup,

-			cache_stat.total_add, cache_stat.total_remove, cache_stat.total_enumerate);

-		slapi_log_err(SLAPI_LOG_FATAL, MEMBEROF_PLUGIN_SUBSYSTEM, "cache statistics duration: lookup %ld, add %ld, remove %ld, enum %ld\n",

-			cache_stat.cumul_duration_lookup, cache_stat.cumul_duration_add,

-			cache_stat.cumul_duration_remove, cache_stat.cumul_duration_enumerate);

-#endif

-	}

-	if (fixup_entry_hashtable) {

-		fixup_hashtable_empty("memberof_lock");

-	}

-	if (usetxn) {

-		PR_ExitMonitor(memberof_operation_lock);

-	}

-}

-

 void memberof_fixup_task_thread(void *arg)

 {

-	MemberOfConfig configCopy = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};

+	MemberOfConfig configCopy = {0};

 	Slapi_Task *task = (Slapi_Task *)arg;

 	task_data *td = NULL;

 	int rc = 0;

@@ -3068,14 +2972,8 @@ void memberof_fixup_task_thread(void *arg)

 		}

 	}

-	/* get the memberOf operation lock */

-	memberof_lock();

-

 	/* do real work */

 	rc = memberof_fix_memberof(&configCopy, task, td);

- 

-	/* release the memberOf operation lock */

-	memberof_unlock();

 done:

 	if (usetxn && fixup_pb) {

@@ -3240,7 +3138,7 @@ int memberof_fix_memberof(MemberOfConfig *config, Slapi_Task *task, task_data *t

 }

 static memberof_cached_value *

-ancestors_cache_lookup(const char *ndn)

+ancestors_cache_lookup(MemberOfConfig *config, const char *ndn)

 {

 	memberof_cached_value *e;

 #if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)

@@ -3258,7 +3156,7 @@ ancestors_cache_lookup(const char *ndn)

 	}

 #endif

-	e = (memberof_cached_value *) PL_HashTableLookupConst(group_ancestors_hashtable, (const void *) ndn);

+	e = (memberof_cached_value *) PL_HashTableLookupConst(config->ancestors_cache, (const void *) ndn);

 #if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)

 	if (start) {

@@ -3274,7 +3172,7 @@ ancestors_cache_lookup(const char *ndn)

 }

 static PRBool

-ancestors_cache_remove(const char *ndn)

+ancestors_cache_remove(MemberOfConfig *config, const char *ndn)

 {

 	PRBool rc;

 #if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)

@@ -3292,7 +3190,7 @@ ancestors_cache_remove(const char *ndn)

 	}

 #endif

-	rc = PL_HashTableRemove(group_ancestors_hashtable, (const void *) ndn);

+	rc = PL_HashTableRemove(config->ancestors_cache, (const void *) ndn);

 #if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)

 	if (start) {

@@ -3305,7 +3203,7 @@ ancestors_cache_remove(const char *ndn)

 }

 static PLHashEntry *

-ancestors_cache_add(const void *key, void *value)

+ancestors_cache_add(MemberOfConfig *config, const void *key, void *value)

 {

 	PLHashEntry *e;

 #if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)

@@ -3322,7 +3220,7 @@ ancestors_cache_add(const void *key, void *value)

 	}

 #endif

-	e = PL_HashTableAdd(group_ancestors_hashtable, key, value);

+	e = PL_HashTableAdd(config->ancestors_cache, key, value);

 #if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)

 	if (start) {

@@ -3360,10 +3258,11 @@ int memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data)

 		goto bail;

 	}

-        /* Check if the entry has not already been fixed */

+    /* Check if the entry has not already been fixed */

 	ndn = slapi_sdn_get_ndn(sdn);

-	if (ndn && fixup_entry_hashtable && PL_HashTableLookupConst(fixup_entry_hashtable, (void*) ndn)) {

-		slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_fix_memberof_callback: Entry %s already fixed up\n", ndn);

+	if (ndn && config->fixup_cache && PL_HashTableLookupConst(config->fixup_cache, (void*) ndn)) {

+		slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM,

+            "memberof_fix_memberof_callback: Entry %s already fixed up\n", ndn);

 		goto bail;

 	}

@@ -3383,9 +3282,9 @@ int memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data)

 #if MEMBEROF_CACHE_DEBUG

 			slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_fix_memberof_callback: This is NOT a group %s\n", ndn);

 #endif

-			ht_grp = ancestors_cache_lookup((const void *) ndn);

+			ht_grp = ancestors_cache_lookup(config, (const void *) ndn);

 			if (ht_grp) {

-				if (ancestors_cache_remove((const void *) ndn)) {

+				if (ancestors_cache_remove(config, (const void *) ndn)) {

 					slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_fix_memberof_callback: free cached values for %s\n", ndn);

 					ancestor_hashtable_entry_free(ht_grp);

 					slapi_ch_free((void **) &ht_grp);

@@ -3400,6 +3299,7 @@ int memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data)

 			}

 		}

 	}

+

 	/* If we found some groups, replace the existing memberOf attribute

 	 * with the found values.  */

 	if (groups && slapi_valueset_count(groups))

@@ -3439,9 +3339,9 @@ int memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data)

 	slapi_valueset_free(groups);

 	/* records that this entry has been fixed up */

-	if (fixup_entry_hashtable) {

+	if (config->fixup_cache) {

 		dn_copy = slapi_ch_strdup(ndn);

-		if (PL_HashTableAdd(fixup_entry_hashtable, dn_copy, dn_copy) == NULL) {

+		if (PL_HashTableAdd(config->fixup_cache, dn_copy, dn_copy) == NULL) {

 			slapi_log_err(SLAPI_LOG_FATAL, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_fix_memberof_callback: "

 				"failed to add dn (%s) in the fixup hashtable; NSPR error - %d\n",

 				dn_copy, PR_GetError());

@@ -3539,150 +3439,8 @@ memberof_add_objectclass(char *auto_add_oc, const char *dn)

 	return rc;

 }

-static PRIntn memberof_hash_compare_keys(const void *v1, const void *v2)

-{

-	PRIntn rc;

-	if (0 == strcasecmp((const char *) v1, (const char *) v2)) {

-		rc = 1;

-	} else {

-		rc = 0;

-	}

-	return rc;

-}

-

-static PRIntn memberof_hash_compare_values(const void *v1, const void *v2)

-{

-	PRIntn rc;

-	if ((char *) v1 == (char *) v2) {

-		rc = 1;

-	} else {

-		rc = 0;

-	}

-	return rc;

-}

-

-/*

- *  Hashing function using Bernstein's method

- */

-static PLHashNumber memberof_hash_fn(const void *key)

-{

-    PLHashNumber hash = 5381;

-    unsigned char *x = (unsigned char *)key;

-    int c;

-

-    while ((c = *x++)){

-        hash = ((hash << 5) + hash) ^ c;

-    }

-    return hash;

-}

-

-/* allocates the plugin hashtable

- * This hash table is used by operation and is protected from

- * concurrent operations with the memberof_lock (if not usetxn, memberof_lock

- * is not implemented and the hash table will be not used.

- *

- * The hash table contains all the DN of the entries for which the memberof

- * attribute has been computed/updated during the current operation

- *

- * hash table should be empty at the beginning and end of the plugin callback

- */

-static PLHashTable *hashtable_new()

-{

-	if (!usetxn) {

-		return NULL;

-	}

-

-	return PL_NewHashTable(MEMBEROF_HASHTABLE_SIZE,

-		memberof_hash_fn,

-		memberof_hash_compare_keys,

-		memberof_hash_compare_values, NULL, NULL);

-}

-/* this function called for each hash node during hash destruction */

-static PRIntn fixup_hashtable_remove(PLHashEntry *he, PRIntn index, void *arg)

-{

-	char *dn_copy;

-

-	if (he == NULL) {

-		return HT_ENUMERATE_NEXT;

-	}

-	dn_copy = (char*) he->value;

-	slapi_ch_free_string(&dn_copy);

-

-	return HT_ENUMERATE_REMOVE;

-}

-

-static void fixup_hashtable_empty(char *msg)

-{

-	if (fixup_entry_hashtable) {

-		PL_HashTableEnumerateEntries(fixup_entry_hashtable, fixup_hashtable_remove, msg);

-	}

-}

-

-

-/* allocates the plugin hashtable

- * This hash table is used by operation and is protected from

- * concurrent operations with the memberof_lock (if not usetxn, memberof_lock

- * is not implemented and the hash table will be not used.

- *

- * The hash table contains all the DN of the entries for which the memberof

- * attribute has been computed/updated during the current operation

- *

- * hash table should be empty at the beginning and end of the plugin callback

- */

-

-static

-void ancestor_hashtable_entry_free(memberof_cached_value *entry)

-{

-	int i;

-	for (i = 0; entry[i].valid; i++) {

-		slapi_ch_free((void **) &entry[i].group_dn_val);

-		slapi_ch_free((void **) &entry[i].group_ndn_val);

-	}

-	/* Here we are at the ending element containing the key */

-	slapi_ch_free((void**) &entry[i].key);

-}

-/* this function called for each hash node during hash destruction */

-static PRIntn ancestor_hashtable_remove(PLHashEntry *he, PRIntn index, void *arg)

+int

+memberof_use_txn()

 {

-	memberof_cached_value *group_ancestor_array;

-

-	if (he == NULL)

-		return HT_ENUMERATE_NEXT;

-

-

-	group_ancestor_array = (memberof_cached_value *) he->value;

-	ancestor_hashtable_entry_free(group_ancestor_array);

-	slapi_ch_free((void **)&group_ancestor_array);

-

-	return HT_ENUMERATE_REMOVE;

+    return usetxn;

 }

-

-static void ancestor_hashtable_empty(char *msg)

-{

-#if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)

-	long int start;

-	struct timespec tsnow;

-#endif

-

-	if (group_ancestors_hashtable) {

-		cache_stat.total_enumerate++;

-#if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)

-		if (clock_gettime(CLOCK_REALTIME, &tsnow) != 0) {

-			start = 0;

-		} else {

-			start = tsnow.tv_nsec;

-		}

-#endif

-		PL_HashTableEnumerateEntries(group_ancestors_hashtable, ancestor_hashtable_remove, msg);

-

-#if defined(DEBUG) && defined(HAVE_CLOCK_GETTIME)

-		if (start) {

-			if (clock_gettime(CLOCK_REALTIME, &tsnow) == 0) {

-				cache_stat.cumul_duration_enumerate += (tsnow.tv_nsec - start);

-			}

-		}

-#endif

-	}

-

-}

-

diff --git a/ldap/servers/plugins/memberof/memberof.h b/ldap/servers/plugins/memberof/memberof.h

index 9a3a6a25d..a01c4d247 100644

--- a/ldap/servers/plugins/memberof/memberof.h

+++ b/ldap/servers/plugins/memberof/memberof.h

@@ -62,8 +62,22 @@ typedef struct memberofconfig {

 	int skip_nested;

 	int fixup_task;

 	char *auto_add_oc;

+	PLHashTable *ancestors_cache;

+	PLHashTable *fixup_cache;

 } MemberOfConfig;

+/* The key to access the hash table is the normalized DN

+ * The normalized DN is stored in the value because:

+ *  - It is used in slapi_valueset_find

+ *  - It is used to fill the memberof_get_groups_data.group_norm_vals

+ */

+typedef struct _memberof_cached_value

+{

+    char *key;

+    char *group_dn_val;

+    char *group_ndn_val;

+    int valid;

+} memberof_cached_value;

 /*

  * functions

@@ -88,5 +102,8 @@ int memberof_apply_config (Slapi_PBlock *pb, Slapi_Entry* entryBefore, Slapi_Ent

 void *memberof_get_plugin_id(void);

 void memberof_release_config(void);

 PRUint64 get_plugin_started(void);

+void ancestor_hashtable_entry_free(memberof_cached_value *entry);

+PLHashTable *hashtable_new();

+int memberof_use_txn();

 #endif	/* _MEMBEROF_H_ */

diff --git a/ldap/servers/plugins/memberof/memberof_config.c b/ldap/servers/plugins/memberof/memberof_config.c

index c3474bf2c..3cc7c4d9c 100644

--- a/ldap/servers/plugins/memberof/memberof_config.c

+++ b/ldap/servers/plugins/memberof/memberof_config.c

@@ -14,12 +14,12 @@

  * memberof_config.c - configuration-related code for memberOf plug-in

  *

  */

-

+#include "plhash.h"

 #include <plstr.h>

-

 #include "memberof.h"

 #define MEMBEROF_CONFIG_FILTER "(objectclass=*)"

+#define MEMBEROF_HASHTABLE_SIZE 1000

 /*

  * The configuration attributes are contained in the plugin entry e.g.

@@ -33,7 +33,9 @@

 /*

  * function prototypes

- */ 

+ */

+static void fixup_hashtable_empty( MemberOfConfig *config, char *msg);

+static void ancestor_hashtable_empty(MemberOfConfig *config, char *msg);

 static int memberof_validate_config (Slapi_PBlock *pb, Slapi_Entry* entryBefore, Slapi_Entry* e, 

 										 int *returncode, char *returntext, void *arg);

 static int memberof_search (Slapi_PBlock *pb, Slapi_Entry* entryBefore, Slapi_Entry* e, 

@@ -48,7 +50,7 @@ static int memberof_search (Slapi_PBlock *pb, Slapi_Entry* entryBefore, Slapi_En

 /* This is the main configuration which is updated from dse.ldif.  The

  * config will be copied when it is used by the plug-in to prevent it

  * being changed out from under a running memberOf operation. */

-static MemberOfConfig theConfig = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};

+static MemberOfConfig theConfig = {0};

 static Slapi_RWLock *memberof_config_lock = 0;

 static int inited = 0;

@@ -696,6 +698,12 @@ memberof_copy_config(MemberOfConfig *dest, MemberOfConfig *src)

 {

 	if (dest && src)

 	{

+        /* Allocate our caches here since we only copy the config at the start of an op */

+        if (memberof_use_txn() == 1){

+            dest->ancestors_cache = hashtable_new();

+            dest->fixup_cache = hashtable_new();