From 59ebf6618126547f3861fbef0b9a268f40ccb2bd Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Tue, 13 Dec 2022 09:41:34 -0500 Subject: [PATCH 3/3] Issue 5413 - Allow mutliple MemberOf fixup tasks with different bases/filters Description: A change was made to only allow a single fixup task at a time, but there are cases where you would want to run mutliple tasks but on different branches/filters. Now we maintain a linked list of bases/filters of the current running tasks to monitor this. relates: https://github.com/389ds/389-ds-base/issues/5413 Reviewed by: tbordaz(Thanks!) --- .../suites/memberof_plugin/fixup_test.py | 5 +- ldap/servers/plugins/memberof/memberof.c | 101 ++++++++++++++---- 2 files changed, 85 insertions(+), 21 deletions(-) diff --git a/dirsrvtests/tests/suites/memberof_plugin/fixup_test.py b/dirsrvtests/tests/suites/memberof_plugin/fixup_test.py index 9566e144c..d5369439f 100644 --- a/dirsrvtests/tests/suites/memberof_plugin/fixup_test.py +++ b/dirsrvtests/tests/suites/memberof_plugin/fixup_test.py @@ -59,12 +59,15 @@ def test_fixup_task_limit(topo): with pytest.raises(ldap.UNWILLING_TO_PERFORM): memberof.fixup(DEFAULT_SUFFIX) + # Add second task but on different suffix which should be allowed + memberof.fixup("ou=people," + DEFAULT_SUFFIX) + # Wait for first task to complete task.wait() # Add new task which should be allowed now memberof.fixup(DEFAULT_SUFFIX) - + if __name__ == '__main__': # Run isolated diff --git a/ldap/servers/plugins/memberof/memberof.c b/ldap/servers/plugins/memberof/memberof.c index f3f817f89..a5f48d2c0 100644 --- a/ldap/servers/plugins/memberof/memberof.c +++ b/ldap/servers/plugins/memberof/memberof.c @@ -52,7 +52,6 @@ static Slapi_DN* _pluginDN = NULL; MemberOfConfig *qsortConfig = 0; static int usetxn = 0; static int premodfn = 0; -static PRBool fixup_running = PR_FALSE; static PRLock *fixup_lock = NULL; static int32_t fixup_progress_count = 0; static int64_t fixup_progress_elapsed = 0; @@ -65,6 +64,15 @@ typedef struct _memberofstringll void *next; } memberofstringll; +typedef struct _fixup_ll +{ + Slapi_DN *sdn; + char *filter_str; + void *next; +} mo_fixup_ll; + +static mo_fixup_ll *fixup_list = NULL; + typedef struct _memberof_get_groups_data { MemberOfConfig *config; @@ -438,6 +446,15 @@ memberof_postop_close(Slapi_PBlock *pb __attribute__((unused))) PR_DestroyLock(fixup_lock); fixup_lock = NULL; + mo_fixup_ll *fixup_task = fixup_list; + while (fixup_task != NULL) { + mo_fixup_ll *tmp = fixup_task; + fixup_task = fixup_task->next; + slapi_sdn_free(&tmp->sdn); + slapi_ch_free_string(&tmp->filter_str); + slapi_ch_free((void**)&tmp); + } + slapi_log_err(SLAPI_LOG_TRACE, MEMBEROF_PLUGIN_SUBSYSTEM, "<-- memberof_postop_close\n"); return 0; @@ -2817,7 +2834,6 @@ memberof_fixup_task_thread(void *arg) } PR_Lock(fixup_lock); - fixup_running = PR_TRUE; fixup_progress_count = 0; fixup_progress_elapsed = slapi_current_rel_time_t(); fixup_start_time = slapi_current_rel_time_t(); @@ -2849,11 +2865,10 @@ memberof_fixup_task_thread(void *arg) /* Mark this as a task operation */ configCopy.fixup_task = 1; configCopy.task = task; - + Slapi_DN *sdn = slapi_sdn_new_dn_byref(td->dn); if (usetxn) { - Slapi_DN *sdn = slapi_sdn_new_dn_byref(td->dn); Slapi_Backend *be = slapi_be_select_exact(sdn); - slapi_sdn_free(&sdn); + if (be) { fixup_pb = slapi_pblock_new(); slapi_pblock_set(fixup_pb, SLAPI_BACKEND, be); @@ -2894,14 +2909,37 @@ done: fixup_progress_count, slapi_current_rel_time_t() - fixup_start_time); slapi_task_inc_progress(task); + /* Cleanup task linked list */ + PR_Lock(fixup_lock); + mo_fixup_ll *prev = NULL; + for (mo_fixup_ll *curr = fixup_list; curr; curr = curr->next) { + mo_fixup_ll *next = curr->next; + if (slapi_sdn_compare(curr->sdn, sdn) == 0 && + strcasecmp(curr->filter_str, td->filter_str) == 0) + { + /* free current code */ + slapi_sdn_free(&curr->sdn); + slapi_ch_free_string(&curr->filter_str); + slapi_ch_free((void**)&curr); + + /* update linked list */ + if (prev == NULL) { + /* first node */ + fixup_list = next; + } else { + prev->next = next; + } + break; + } + prev = curr; + } + PR_Unlock(fixup_lock); + slapi_sdn_free(&sdn); + /* this will queue the destruction of the task */ slapi_task_finish(task, rc); slapi_task_dec_refcount(task); - PR_Lock(fixup_lock); - fixup_running = PR_FALSE; - PR_Unlock(fixup_lock); - slapi_log_err(SLAPI_LOG_INFO, MEMBEROF_PLUGIN_SUBSYSTEM, "memberof_fixup_task_thread - Memberof task finished (processed %d entries in %ld seconds)\n", fixup_progress_count, slapi_current_rel_time_t() - fixup_start_time); @@ -2919,23 +2957,13 @@ memberof_task_add(Slapi_PBlock *pb, int rv = SLAPI_DSE_CALLBACK_OK; task_data *mytaskdata = NULL; Slapi_Task *task = NULL; + Slapi_DN *sdn = NULL; char *bind_dn; const char *filter; const char *dn = 0; *returncode = LDAP_SUCCESS; - PR_Lock(fixup_lock); - if (fixup_running) { - PR_Unlock(fixup_lock); - *returncode = LDAP_UNWILLING_TO_PERFORM; - slapi_log_err(SLAPI_LOG_ERR, MEMBEROF_PLUGIN_SUBSYSTEM, - "memberof_task_add - there is already a fixup task running\n"); - rv = SLAPI_DSE_CALLBACK_ERROR; - goto out; - } - PR_Unlock(fixup_lock); - /* get arg(s) */ if ((dn = slapi_entry_attr_get_ref(e, "basedn")) == NULL) { *returncode = LDAP_OBJECT_CLASS_VIOLATION; @@ -2949,6 +2977,39 @@ memberof_task_add(Slapi_PBlock *pb, goto out; } + PR_Lock(fixup_lock); + sdn = slapi_sdn_new_dn_byval(dn); + if (fixup_list == NULL) { + fixup_list = (mo_fixup_ll *)slapi_ch_calloc(1, sizeof(mo_fixup_ll)); + fixup_list->sdn = sdn; + fixup_list->filter_str = slapi_ch_strdup(filter); + } else { + for (mo_fixup_ll *fixup_task = fixup_list; fixup_task; fixup_task = fixup_task->next) { + if (slapi_sdn_compare(sdn, fixup_task->sdn) == 0 && + strcasecmp(filter, fixup_task->filter_str) == 0) + { + /* Found an identical running task, reject it */ + PR_Unlock(fixup_lock); + slapi_log_err(SLAPI_LOG_ERR, MEMBEROF_PLUGIN_SUBSYSTEM, + "memberof_task_add - there is already an identical fixup task running: base: %s filter: %s\n", + slapi_sdn_get_dn(sdn), filter); + slapi_sdn_free(&sdn); + *returncode = LDAP_UNWILLING_TO_PERFORM; + rv = SLAPI_DSE_CALLBACK_ERROR; + goto out; + } + } + /* Add the new task DN to the top of the list */ + mo_fixup_ll *head = fixup_list; + mo_fixup_ll *new_task = (mo_fixup_ll *)slapi_ch_calloc(1, sizeof(mo_fixup_ll)); + new_task->sdn = sdn; + new_task->filter_str = slapi_ch_strdup(filter); + new_task->next = head; + fixup_list = new_task; + } + PR_Unlock(fixup_lock); + + /* setup our task data */ slapi_pblock_get(pb, SLAPI_REQUESTOR_DN, &bind_dn); mytaskdata = (task_data *)slapi_ch_malloc(sizeof(task_data)); -- 2.38.1