From cbe71d7e4901232eaa423b9dc55dba9401c05bec Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Fri, 13 Oct 2017 07:09:08 -0400 Subject: [PATCH] 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: firstyear & tbordaz(Thanks!!) (cherry picked from commit 184b8a164f4ed456c72d58038aa9a0d512be61fa) --- ldap/servers/plugins/memberof/memberof.c | 326 +++--------------------- ldap/servers/plugins/memberof/memberof.h | 17 ++ ldap/servers/plugins/memberof/memberof_config.c | 166 +++++++++++- 3 files changed, 210 insertions(+), 299 deletions(-) diff --git a/ldap/servers/plugins/memberof/memberof.c b/ldap/servers/plugins/memberof/memberof.c index a0f997ddf..a23c52abe 100644 --- a/ldap/servers/plugins/memberof/memberof.c +++ b/ldap/servers/plugins/memberof/memberof.c @@ -48,14 +48,11 @@ static Slapi_PluginDesc pdesc = {"memberof", VENDOR, 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; +static Slapi_DN* _pluginDN = NULL; 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,6 @@ 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; @@ -164,14 +149,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 ***/ @@ -344,11 +324,6 @@ 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; @@ -356,9 +331,6 @@ 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) { @@ -413,13 +385,13 @@ memberof_postop_start(Slapi_PBlock *pb) goto bail; } -/* + /* * TODO: start up operation actor thread * need to get to a point where server failure - * or shutdown doesn't hose our operations - * so we should create a task entry that contains + * or shutdown doesn't hose our operations + * so we should create a task entry that contains * all required information to complete the operation - * then the tasks can be restarted safely if + * then the tasks can be restarted safely if * interrupted */ @@ -451,18 +423,7 @@ memberof_postop_close(Slapi_PBlock *pb __attribute__((unused))) 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; @@ -524,7 +485,7 @@ 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; @@ -553,9 +514,6 @@ 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 */ @@ -563,7 +521,6 @@ 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; } @@ -583,7 +540,6 @@ memberof_postop_del(Slapi_PBlock *pb) } } } - memberof_unlock(); bail: memberof_free_config(&configCopy); } @@ -776,7 +732,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); @@ -918,7 +874,7 @@ 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; @@ -944,8 +900,6 @@ 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)) { @@ -1010,7 +964,6 @@ memberof_postop_modrdn(Slapi_PBlock *pb) } } } - memberof_unlock(); bail: memberof_free_config(&configCopy); } @@ -1166,7 +1119,7 @@ memberof_postop_modify(Slapi_PBlock *pb) if (memberof_oktodo(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); @@ -1209,8 +1162,6 @@ memberof_postop_modify(Slapi_PBlock *pb) if (interested) { int op = slapi_mod_get_operation(smod); - memberof_lock(); - /* the modify op decides the function */ switch (op & ~LDAP_MOD_BVALUES) { case LDAP_MOD_ADD: { @@ -1221,7 +1172,6 @@ memberof_postop_modify(Slapi_PBlock *pb) "Error (%d)\n", slapi_sdn_get_dn(sdn), ret); slapi_mod_done(next_mod); - memberof_unlock(); goto bail; } break; @@ -1239,7 +1189,6 @@ memberof_postop_modify(Slapi_PBlock *pb) "Error (%d)\n", slapi_sdn_get_dn(sdn), ret); slapi_mod_done(next_mod); - memberof_unlock(); goto bail; } } else { @@ -1250,7 +1199,6 @@ memberof_postop_modify(Slapi_PBlock *pb) "Error (%d)\n", slapi_sdn_get_dn(sdn), ret); slapi_mod_done(next_mod); - memberof_unlock(); goto bail; } } @@ -1265,7 +1213,6 @@ memberof_postop_modify(Slapi_PBlock *pb) "Error (%d)\n", slapi_sdn_get_dn(sdn), ret); slapi_mod_done(next_mod); - memberof_unlock(); goto bail; } break; @@ -1280,8 +1227,6 @@ memberof_postop_modify(Slapi_PBlock *pb) break; } } - - memberof_unlock(); } slapi_mod_done(next_mod); @@ -1336,7 +1281,7 @@ 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); @@ -1361,8 +1306,6 @@ 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)) { if ((ret = memberof_add_attr_list(pb, &configCopy, sdn, attr))) { @@ -1373,8 +1316,6 @@ memberof_postop_add(Slapi_PBlock *pb) } } } - - memberof_unlock(); memberof_free_config(&configCopy); } } @@ -2094,7 +2035,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; @@ -2191,14 +2132,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) { - slapi_log_err(SLAPI_LOG_FATAL, MEMBEROF_PLUGIN_SUBSYSTEM, "cache_ancestors: Failed to cache ancestor of %s\n", key); + 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); + 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 @@ -2283,8 +2224,7 @@ memberof_get_groups_r(MemberOfConfig *config, Slapi_DN *member_sdn, memberof_get merge_ancestors(&member_ndn_val, &member_data, data); if (!cached && member_data.use_cache) - cache_ancestors(&member_ndn_val, &member_data); - + cache_ancestors(config, &member_ndn_val, &member_data); slapi_value_free(&member_ndn_val); slapi_valueset_free(groupvals); @@ -2825,49 +2765,10 @@ 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; @@ -2933,9 +2834,6 @@ memberof_fixup_task_thread(void *arg) /* do real work */ rc = memberof_fix_memberof(&configCopy, task, td); - /* release the memberOf operation lock */ - memberof_unlock(); - done: if (usetxn && fixup_pb) { if (rc) { /* failed */ @@ -3100,7 +2998,7 @@ memberof_fix_memberof(MemberOfConfig *config, Slapi_Task *task, task_data *td) } 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) @@ -3118,7 +3016,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) { @@ -3133,7 +3031,7 @@ ancestors_cache_lookup(const char *ndn) return e; } 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) @@ -3151,7 +3049,8 @@ 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) { @@ -3164,7 +3063,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) @@ -3181,7 +3080,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) { @@ -3211,7 +3110,6 @@ memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data) const char *ndn; char *dn_copy; - /* * If the server is ordered to shutdown, stop the fixup and return an error. */ @@ -3222,7 +3120,7 @@ memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data) /* 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)) { + 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; } @@ -3240,12 +3138,13 @@ memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data) * so free this memory */ ndn = slapi_sdn_get_ndn(sdn); + #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); @@ -3297,11 +3196,11 @@ 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", + "failed to add dn (%s) in the fixup hashtable; NSPR error - %d\n", dn_copy, PR_GetError()); slapi_ch_free((void **)&dn_copy); /* let consider this as not a fatal error, it just skip an optimization */ @@ -3397,157 +3296,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 __attribute__((unused)), void *arg __attribute__((unused))) -{ - 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 __attribute__((unused)), void *arg __attribute__((unused))) -{ - 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; -} - -static void -ancestor_hashtable_empty(char *msg) +int +memberof_use_txn() { -#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 - } + return usetxn; } diff --git a/ldap/servers/plugins/memberof/memberof.h b/ldap/servers/plugins/memberof/memberof.h index 4833ce221..ba64e9dfa 100644 --- a/ldap/servers/plugins/memberof/memberof.h +++ b/ldap/servers/plugins/memberof/memberof.h @@ -64,8 +64,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 @@ -89,5 +103,8 @@ int memberof_apply_config(Slapi_PBlock *pb, Slapi_Entry *entryBefore, Slapi_Entr 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 c5ca4b137..3f22d95d6 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 - #include "memberof.h" #define MEMBEROF_CONFIG_FILTER "(objectclass=*)" +#define MEMBEROF_HASHTABLE_SIZE 1000 /* * The configuration attributes are contained in the plugin entry e.g. @@ -34,14 +34,16 @@ /* * function prototypes */ -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 __attribute__((unused)), - Slapi_Entry *entryBefore __attribute__((unused)), - Slapi_Entry *e __attribute__((unused)), - int *returncode __attribute__((unused)), - char *returntext __attribute__((unused)), - void *arg __attribute__((unused))) +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 __attribute__((unused)), + Slapi_Entry* entryBefore __attribute__((unused)), + Slapi_Entry* e __attribute__((unused)), + int *returncode __attribute__((unused)), + char *returntext __attribute__((unused)), + void *arg __attribute__((unused))) { return SLAPI_DSE_CALLBACK_OK; } @@ -52,7 +54,7 @@ memberof_search(Slapi_PBlock *pb __attribute__((unused)), /* 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; @@ -693,6 +695,13 @@ void 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(); + } + /* Check if the copy is already up to date */ if (src->groupattrs) { int i = 0, j = 0; @@ -787,6 +796,14 @@ memberof_free_config(MemberOfConfig *config) slapi_ch_free_string(&config->memberof_attr); memberof_free_scope(&(config->entryScopes), &config->entryScopeCount); memberof_free_scope(&(config->entryScopeExcludeSubtrees), &config->entryExcludeScopeCount); + if (config->fixup_cache) { + fixup_hashtable_empty(config, "memberof_free_config empty fixup_entry_hastable"); + PL_HashTableDestroy(config->fixup_cache); + } + if (config->ancestors_cache) { + ancestor_hashtable_empty(config, "memberof_free_config empty group_ancestors_hashtable"); + PL_HashTableDestroy(config->ancestors_cache); + } } } @@ -982,3 +999,130 @@ bail: return ret; } + + +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 + */ +PLHashTable *hashtable_new(int usetxn) +{ + 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 __attribute__((unused)), void *arg __attribute__((unused))) +{ + 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(MemberOfConfig *config, char *msg) +{ + if (config->fixup_cache) { + PL_HashTableEnumerateEntries(config->fixup_cache, 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 + */ + +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 __attribute__((unused)), void *arg __attribute__((unused))) +{ + 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; +} + +static void ancestor_hashtable_empty(MemberOfConfig *config, char *msg) +{ + if (config->ancestors_cache) { + PL_HashTableEnumerateEntries(config->ancestors_cache, ancestor_hashtable_remove, msg); + } +} -- 2.13.6