From 99dbba52eb45628c7f290e9ed3aeabb2a2a67db4 Mon Sep 17 00:00:00 2001

From: Mark Reynolds <mreynolds@redhat.com>

Date: Tue, 13 Dec 2022 09:41:34 -0500

Subject: [PATCH] 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 0b8cfe95c..a14617044 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