From 1bf0574b2952564ca14a98721b75a2667fe098e2 Mon Sep 17 00:00:00 2001 From: Joseph Hickey Date: Tue, 18 Aug 2020 16:02:36 -0400 Subject: [PATCH 1/2] Fix #565, propagate return code from OS_TaskRegister_Impl() If this routine fails then return the error to the caller, which will also prevent the task from starting. --- src/os/shared/src/osapi-task.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/os/shared/src/osapi-task.c b/src/os/shared/src/osapi-task.c index 912b0ff86..bb78f1623 100644 --- a/src/os/shared/src/osapi-task.c +++ b/src/os/shared/src/osapi-task.c @@ -116,9 +116,10 @@ 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 + + if (return_code != OS_SUCCESS) { *entrypt = NULL; } From 2cc7dc508e0bab7961071361ea2d548801caec3e Mon Sep 17 00:00:00 2001 From: Joseph Hickey Date: Tue, 18 Aug 2020 15:15:05 -0400 Subject: [PATCH 2/2] 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. --- src/os/shared/inc/os-shared-idmap.h | 13 ++++++++++ src/os/shared/src/osapi-binsem.c | 10 ++------ src/os/shared/src/osapi-countsem.c | 10 ++------ src/os/shared/src/osapi-dir.c | 10 ++------ src/os/shared/src/osapi-file.c | 10 ++------ src/os/shared/src/osapi-idmap.c | 22 +++++++++++++++++ src/os/shared/src/osapi-module.c | 10 ++------ src/os/shared/src/osapi-mutex.c | 10 ++------ src/os/shared/src/osapi-queue.c | 10 ++------ src/os/shared/src/osapi-task.c | 24 ++++++------------- src/os/shared/src/osapi-time.c | 6 ++--- src/os/shared/src/osapi-timebase.c | 10 ++------ .../shared/src/coveragetest-task.c | 5 ++-- src/ut-stubs/osapi-utstub-idmap.c | 15 ++++++++++++ 14 files changed, 77 insertions(+), 88 deletions(-) diff --git a/src/os/shared/inc/os-shared-idmap.h b/src/os/shared/inc/os-shared-idmap.h index 2be4d37ec..663f2438f 100644 --- 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 index 8fcd41b6e..3784aa652 100644 --- 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 index 01b2b4520..42f61d410 100644 --- 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 index e17b9b6c5..fe799bb5b 100644 --- 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 index 88780a208..007c25bb6 100644 --- 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 index cb4b52563..2d172bf3d 100644 --- 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 index bf8daff18..e730ac7cb 100644 --- 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 index 4f2bce392..eb35e079e 100644 --- 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 index 7cfdcee50..b1e104152 100644 --- 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 index 912b0ff86..663a74334 100644 --- 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 index 646454e3b..31b76c8df 100644 --- 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 index 1ed2dfb5a..44b5d9f97 100644 --- 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 index 96b6c2225..0a01acb8c 100644 --- 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 index 2766d9402..d9ae7b64a 100644 --- 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()