Fix #564, consolidate delete finalization code

Remove repetitive clearing of the global ID and unlocking global table and replace with common implementation in the idmap code. In addition to removing repeated logic, this makes deletion more like an inverse of creation, and provides a common/single point where additiona deletion-related logic can be added.
nasa · Aug 18, 2020 · 2cc7dc5 · 2cc7dc5
1 parent 8cfd6fe
commit 2cc7dc5
Show file tree

Hide file tree

Showing 14 changed files with 77 additions and 88 deletions.
diff --git a/src/os/shared/inc/os-shared-idmap.h b/src/os/shared/inc/os-shared-idmap.h
@@ -296,6 +296,19 @@ int32 OS_ObjectIdAllocateNew(uint32 idtype, const char *name, uint32 *array_inde
  ------------------------------------------------------------------*/
 int32 OS_ObjectIdFinalizeNew(int32 operation_status, OS_common_record_t *record, uint32 *outid);
 
+/*----------------------------------------------------------------
+   Function: OS_ObjectIdFinalizeDelete
+
+    Purpose: Completes a delete operation
+             If the operation was successful, the OSAL ID is deleted and returned to the pool
+             If the operation was unsuccessful, no operation is performed.
+             The global table is unlocked for future operations
+
+    Returns: OS_SUCCESS on success, or relevant error code
+ ------------------------------------------------------------------*/
+int32 OS_ObjectIdFinalizeDelete(int32 operation_status, OS_common_record_t *record);
+
+
 /*----------------------------------------------------------------
    Function: OS_ObjectIdRefcountDecr
 

diff --git a/src/os/shared/src/osapi-binsem.c b/src/os/shared/src/osapi-binsem.c
@@ -157,14 +157,8 @@ int32 OS_BinSemDelete (uint32 sem_id)
    {
       return_code = OS_BinSemDelete_Impl(local_id);
 
-      /* Free the entry in the master table now while still locked */
-      if (return_code == OS_SUCCESS)
-      {
-         /* Only need to clear the ID as zero is the "unused" flag */
-         record->active_id = 0;
-      }
-
-      OS_Unlock_Global(LOCAL_OBJID_TYPE);
+      /* Complete the operation via the common routine */
+      return_code = OS_ObjectIdFinalizeDelete(return_code, record);
    }
 
    return return_code;

diff --git a/src/os/shared/src/osapi-countsem.c b/src/os/shared/src/osapi-countsem.c
@@ -150,14 +150,8 @@ int32 OS_CountSemDelete (uint32 sem_id)
    {
       return_code = OS_CountSemDelete_Impl(local_id);
 
-      /* Free the entry in the master table now while still locked */
-      if (return_code == OS_SUCCESS)
-      {
-         /* Only need to clear the ID as zero is the "unused" flag */
-         record->active_id = 0;
-      }
-
-      OS_Unlock_Global(LOCAL_OBJID_TYPE);
+      /* Complete the operation via the common routine */
+      return_code = OS_ObjectIdFinalizeDelete(return_code, record);
    }
 
    return return_code;

diff --git a/src/os/shared/src/osapi-dir.c b/src/os/shared/src/osapi-dir.c
@@ -187,14 +187,8 @@ int32 OS_DirectoryClose(uint32 dir_id)
    {
        return_code = OS_DirClose_Impl(local_id);
 
-       /* Free the entry in the master table now while still locked */
-       if (return_code == OS_SUCCESS)
-       {
-           /* Only need to clear the ID as zero is the "unused" flag */
-           record->active_id = 0;
-       }
-
-       OS_Unlock_Global(LOCAL_OBJID_TYPE);
+       /* Complete the operation via the common routine */
+       return_code = OS_ObjectIdFinalizeDelete(return_code, record);
    }
 
    return return_code;

diff --git a/src/os/shared/src/osapi-file.c b/src/os/shared/src/osapi-file.c
@@ -223,14 +223,8 @@ int32 OS_close (uint32 filedes)
     {
         return_code = OS_GenericClose_Impl(local_id);
 
-        /* Free the entry in the master table now while still locked */
-        if (return_code == OS_SUCCESS)
-        {
-            /* Only need to clear the ID as zero is the "unused" flag */
-            record->active_id = 0;
-        }
-
-        OS_Unlock_Global(LOCAL_OBJID_TYPE);
+        /* Complete the operation via the common routine */
+        return_code = OS_ObjectIdFinalizeDelete(return_code, record);
     }
 
     return return_code;

diff --git a/src/os/shared/src/osapi-idmap.c b/src/os/shared/src/osapi-idmap.c
@@ -769,6 +769,28 @@ int32 OS_ObjectIdFinalizeNew(int32 operation_status, OS_common_record_t *record,
     return operation_status;
 } /* end OS_ObjectIdFinalizeNew */
 
+/*----------------------------------------------------------------
+   Function: OS_ObjectIdFinalizeDelete
+
+    Purpose: Helper routine, not part of OSAL public API.
+             See description in prototype
+ ------------------------------------------------------------------*/
+int32 OS_ObjectIdFinalizeDelete(int32 operation_status, OS_common_record_t *record)
+{
+    uint32 idtype = OS_ObjectIdToType_Impl(record->active_id);
+
+    /* Clear the OSAL ID if successful - this returns the record to the pool */
+    if (operation_status == OS_SUCCESS)
+    {
+        record->active_id = 0;
+    }
+
+    /* Either way we must unlock the object type */
+    OS_Unlock_Global(idtype);
+
+    return operation_status;
+}
+
 
 /*----------------------------------------------------------------
  *

diff --git a/src/os/shared/src/osapi-module.c b/src/os/shared/src/osapi-module.c
@@ -289,14 +289,8 @@ int32 OS_ModuleUnload ( uint32 module_id )
          */
         return_code = OS_ModuleUnload_Impl(local_id);
 
-        if (return_code == OS_SUCCESS)
-        {
-            /* Clear the ID to zero */
-            record->active_id = 0;
-        }
-
-        /* Unlock the global from OS_ObjectIdGetAndLock() */
-        OS_Unlock_Global(LOCAL_OBJID_TYPE);
+        /* Complete the operation via the common routine */
+        return_code = OS_ObjectIdFinalizeDelete(return_code, record);
     }
 
     return return_code;

diff --git a/src/os/shared/src/osapi-mutex.c b/src/os/shared/src/osapi-mutex.c
@@ -145,14 +145,8 @@ int32 OS_MutSemDelete (uint32 sem_id)
    {
       return_code = OS_MutSemDelete_Impl(local_id);
 
-      /* Free the entry in the master table now while still locked */
-      if (return_code == OS_SUCCESS)
-      {
-         /* Only need to clear the ID as zero is the "unused" flag */
-         record->active_id = 0;
-      }
-
-      OS_Unlock_Global(LOCAL_OBJID_TYPE);
+      /* Complete the operation via the common routine */
+      return_code = OS_ObjectIdFinalizeDelete(return_code, record);
    }
 
    return return_code;

diff --git a/src/os/shared/src/osapi-queue.c b/src/os/shared/src/osapi-queue.c
@@ -156,14 +156,8 @@ int32 OS_QueueDelete (uint32 queue_id)
    {
       return_code = OS_QueueDelete_Impl(local_id);
 
-      /* Free the entry in the master table now while still locked */
-      if (return_code == OS_SUCCESS)
-      {
-         /* Only need to clear the ID as zero is the "unused" flag */
-         record->active_id = 0;
-      }
-
-      OS_Unlock_Global(LOCAL_OBJID_TYPE);
+      /* Complete the operation via the common routine */
+      return_code = OS_ObjectIdFinalizeDelete(return_code, record);
    }
 
    return return_code;

diff --git a/src/os/shared/src/osapi-task.c b/src/os/shared/src/osapi-task.c
@@ -45,6 +45,7 @@
  * User defined include files
  */
 #include "os-shared-task.h"
+#include "os-shared-common.h"
 #include "os-shared-idmap.h"
 
 
@@ -116,7 +117,7 @@ static int32 OS_TaskPrepare(uint32 task_id, osal_task_entry *entrypt)
 
    if (return_code == OS_SUCCESS)
    {
-      OS_TaskRegister_Impl(task_id);
+       return_code = OS_TaskRegister_Impl(task_id);
    }
    else
    {
@@ -269,24 +270,14 @@ int32 OS_TaskDelete (uint32 task_id)
 
       return_code = OS_TaskDelete_Impl(local_id);
 
-      /* Free the entry in the master table now while still locked */
-      if (return_code == OS_SUCCESS)
-      {
-         /* Only need to clear the ID as zero is the "unused" flag */
-         record->active_id = 0;
-      }
-      else
-      {
-         delete_hook = NULL;
-      }
-
-      OS_Unlock_Global(LOCAL_OBJID_TYPE);
+      /* Complete the operation via the common routine */
+      return_code = OS_ObjectIdFinalizeDelete(return_code, record);
    }
 
    /*
    ** Call the thread Delete hook if there is one.
    */
-   if (delete_hook != NULL)
+   if (return_code == OS_SUCCESS && delete_hook != NULL)
    {
       delete_hook();
    }
@@ -312,9 +303,8 @@ void OS_TaskExit()
    task_id = OS_TaskGetId_Impl();
    if (OS_ObjectIdGetById(OS_LOCK_MODE_GLOBAL, LOCAL_OBJID_TYPE, task_id, &local_id, &record) == OS_SUCCESS)
    {
-      /* Only need to clear the ID as zero is the "unused" flag */
-      record->active_id = 0;
-      OS_Unlock_Global(LOCAL_OBJID_TYPE);
+      /* Complete the operation via the common routine */
+      OS_ObjectIdFinalizeDelete(OS_SUCCESS, record);
    }
 
    /* call the implementation */

diff --git a/src/os/shared/src/osapi-time.c b/src/os/shared/src/osapi-time.c
@@ -459,12 +459,10 @@ int32 OS_TimerDelete(uint32 timer_id)
         local->next_ref = local_id;
         local->prev_ref = local_id;
 
-        /* Clear the ID to zero */
-        record->active_id = 0;
-
         OS_TimeBaseUnlock_Impl(local->timebase_ref);
 
-        OS_Unlock_Global(OS_OBJECT_TYPE_OS_TIMECB);
+        /* Complete the operation via the common routine */
+        return_code = OS_ObjectIdFinalizeDelete(return_code, record);
     }
 
     /*

diff --git a/src/os/shared/src/osapi-timebase.c b/src/os/shared/src/osapi-timebase.c
@@ -260,14 +260,8 @@ int32 OS_TimeBaseDelete(uint32 timer_id)
     {
         return_code = OS_TimeBaseDelete_Impl(local_id);
 
-        /* Free the entry in the master table now while still locked */
-        if (return_code == OS_SUCCESS)
-        {
-            /* Clear the ID to zero */
-            record->active_id = 0;
-        }
-
-        OS_Unlock_Global(OS_OBJECT_TYPE_OS_TIMEBASE);
+        /* Complete the operation via the common routine */
+        return_code = OS_ObjectIdFinalizeDelete(return_code, record);
     }
 
     return return_code;

diff --git a/src/unit-test-coverage/shared/src/coveragetest-task.c b/src/unit-test-coverage/shared/src/coveragetest-task.c
@@ -167,9 +167,8 @@ void Test_OS_TaskExit(void)
 
     OS_TaskExit();
 
-    /* TaskExit should have cleared the active_id */
-    UtAssert_True(utrec.active_id == 0, "utrec.active_id (%lu) == 0",
-            (unsigned long)utrec.active_id);
+    /* TaskExit should have called OS_ObjectIdFinalizeDelete to clear the active_id */
+    UtAssert_STUB_COUNT(OS_ObjectIdFinalizeDelete, 1);
 }
 void Test_OS_TaskDelay(void)
 {

diff --git a/src/ut-stubs/osapi-utstub-idmap.c b/src/ut-stubs/osapi-utstub-idmap.c
@@ -179,6 +179,21 @@ int32 OS_ObjectIdFinalizeNew(int32 operation_status, OS_common_record_t *record,
     return Status;
 }
 
+/*****************************************************************************
+ *
+ * Stub function for OS_ObjectIdFinalizeDelete()
+ *
+ *****************************************************************************/
+int32 OS_ObjectIdFinalizeDelete(int32 operation_status, OS_common_record_t *record)
+{
+    int32 Status;
+
+    Status = UT_DEFAULT_IMPL_RC(OS_ObjectIdFinalizeDelete, operation_status);
+
+    return Status;
+}
+
+
 /*****************************************************************************
  *
  * Stub function for OS_ObjectIdFindMatch()