From 16e2c0e99b574b6d552e0d61d112a4821cf9c7dc Mon Sep 17 00:00:00 2001 From: Joseph Hickey Date: Mon, 14 Sep 2020 16:10:34 -0400 Subject: [PATCH] Fix #886, library table management Apply the appid/taskid pattern to Library resources. No real logic change - just putting repeated logic into inline functions. --- fsw/cfe-core/src/es/cfe_es_api.c | 37 +++++ fsw/cfe-core/src/es/cfe_es_apps.c | 33 ++-- fsw/cfe-core/src/es/cfe_es_global.h | 90 ++++++++++ fsw/cfe-core/src/inc/cfe_es.h | 24 +++ fsw/cfe-core/unit-test/es_UT.c | 248 +++++++++++++++------------- fsw/cfe-core/unit-test/es_UT.h | 1 + 6 files changed, 302 insertions(+), 131 deletions(-) diff --git a/fsw/cfe-core/src/es/cfe_es_api.c b/fsw/cfe-core/src/es/cfe_es_api.c index 5da8b905e..895cca897 100644 --- a/fsw/cfe-core/src/es/cfe_es_api.c +++ b/fsw/cfe-core/src/es/cfe_es_api.c @@ -1685,6 +1685,25 @@ int32 CFE_ES_AppID_ToIndex(uint32 AppId, uint32 *Idx) return CFE_SUCCESS; } +/* + * A conversion function to obtain an index value correlating to a LibID + * This is a zero based value that can be used for indexing into a table. + */ +int32 CFE_ES_LibID_ToIndex(uint32 LibId, uint32 *Idx) +{ + if (LibId >= CFE_PLATFORM_ES_MAX_LIBRARIES) + { + return CFE_ES_BAD_ARGUMENT; /* these do not have a dedicated error */ + } + + /* + * Currently this is a direct/simple pass through. + * Will evolve in a future rev to make it more safe. + */ + *Idx = LibId; + return CFE_SUCCESS; +} + /* * A conversion function to obtain an index value correlating to an TaskID * This is a zero based value that can be used for indexing into a table. @@ -1777,6 +1796,24 @@ CFE_ES_AppRecord_t *CFE_ES_LocateAppRecordByID(uint32 AppID) return AppRecPtr; } +extern CFE_ES_LibRecord_t* CFE_ES_LocateLibRecordByID(uint32 LibID) +{ + CFE_ES_LibRecord_t *LibRecPtr; + uint32 Idx; + + if (CFE_ES_LibID_ToIndex(LibID, &Idx) == CFE_SUCCESS) + { + LibRecPtr = &CFE_ES_Global.LibTable[Idx]; + } + else + { + LibRecPtr = NULL; + } + + return LibRecPtr; +} + + /* * Note - this gets the table entry pointer but does not dereference or * otherwise check/validate said pointer, as that would have to be done while diff --git a/fsw/cfe-core/src/es/cfe_es_apps.c b/fsw/cfe-core/src/es/cfe_es_apps.c index 1c3e4f30e..ae8dddcb1 100644 --- a/fsw/cfe-core/src/es/cfe_es_apps.c +++ b/fsw/cfe-core/src/es/cfe_es_apps.c @@ -567,7 +567,8 @@ int32 CFE_ES_LoadLibrary(uint32 *LibraryIdPtr, CFE_ES_LibRecord_t * LibSlotPtr; size_t StringLength; int32 Status; - uint32 CheckSlot; + uint32 LibIndex; + uint32 PendingLibId; osal_id_t ModuleId; bool IsModuleLoaded; @@ -576,7 +577,7 @@ int32 CFE_ES_LoadLibrary(uint32 *LibraryIdPtr, * (currently sized to OS_MAX_API_NAME, but not assuming that will always be) */ StringLength = strlen(LibName); - if (StringLength >= sizeof(CFE_ES_Global.LibTable[0].LibName)) + if (StringLength >= sizeof(LibSlotPtr->LibName)) { return CFE_ES_BAD_ARGUMENT; } @@ -585,16 +586,17 @@ int32 CFE_ES_LoadLibrary(uint32 *LibraryIdPtr, ** Allocate an ES_LibTable entry */ IsModuleLoaded = false; - LibSlotPtr = NULL; FunctionPointer = NULL; ModuleId = OS_OBJECT_ID_UNDEFINED; + PendingLibId = 0xFFFFFFFF; Status = CFE_ES_ERR_LOAD_LIB; /* error that will be returned if no slots found */ CFE_ES_LockSharedData(__func__,__LINE__); - for ( CheckSlot = 0; CheckSlot < CFE_PLATFORM_ES_MAX_LIBRARIES; CheckSlot++ ) + LibSlotPtr = CFE_ES_Global.LibTable; + for ( LibIndex = 0; LibIndex < CFE_PLATFORM_ES_MAX_LIBRARIES; ++LibIndex ) { - if (CFE_ES_Global.LibTable[CheckSlot].RecordUsed) + if (CFE_ES_LibRecordIsUsed(LibSlotPtr)) { - if (strcmp(CFE_ES_Global.LibTable[CheckSlot].LibName, LibName) == 0) + if (strcmp(LibSlotPtr->LibName, LibName) == 0) { /* * Indicate to caller that the library is already loaded. @@ -603,29 +605,34 @@ int32 CFE_ES_LoadLibrary(uint32 *LibraryIdPtr, * Do nothing more; not logging this event as it may or may * not be an error. */ - *LibraryIdPtr = CheckSlot; + *LibraryIdPtr = CFE_ES_LibRecordGetID(LibSlotPtr); Status = CFE_ES_LIB_ALREADY_LOADED; break; } } - else if (LibSlotPtr == NULL) + else if (PendingLibId == 0xFFFFFFFF) { /* Remember list position as possible place for new entry. */ - LibSlotPtr = &CFE_ES_Global.LibTable[CheckSlot]; - *LibraryIdPtr = CheckSlot; + PendingLibId = LibIndex; Status = CFE_SUCCESS; } else { /* No action */ } + + ++LibSlotPtr; } if (Status == CFE_SUCCESS) { + /* reset back to the saved index that was free */ + LibSlotPtr = CFE_ES_LocateLibRecordByID(PendingLibId); + /* reserve the slot while still under lock */ strcpy(LibSlotPtr->LibName, LibName); - LibSlotPtr->RecordUsed = true; + CFE_ES_LibRecordSetUsed(LibSlotPtr, PendingLibId); + *LibraryIdPtr = PendingLibId; } CFE_ES_UnlockSharedData(__func__,__LINE__); @@ -761,8 +768,8 @@ int32 CFE_ES_LoadLibrary(uint32 *LibraryIdPtr, OS_ModuleUnload( ModuleId ); } - /* Release Slot - No need to lock as it is resetting just a single bool value */ - LibSlotPtr->RecordUsed = false; + /* Release Slot - No need to lock as it is resetting just a single value */ + CFE_ES_LibRecordSetFree(LibSlotPtr); } return(Status); diff --git a/fsw/cfe-core/src/es/cfe_es_global.h b/fsw/cfe-core/src/es/cfe_es_global.h index 5165687e1..5ba27b515 100644 --- a/fsw/cfe-core/src/es/cfe_es_global.h +++ b/fsw/cfe-core/src/es/cfe_es_global.h @@ -166,6 +166,17 @@ extern CFE_ES_ResetData_t *CFE_ES_ResetDataPtr; */ extern CFE_ES_AppRecord_t* CFE_ES_LocateAppRecordByID(uint32 AppID); +/** + * @brief Locate the Library table entry correlating with a given Lib ID. + * + * This only returns a pointer to the table entry and does _not_ + * otherwise check/validate the entry. + * + * @param[in] LibID the Lib ID to locate + * @return pointer to Library Table entry for the given Lib ID + */ +extern CFE_ES_LibRecord_t* CFE_ES_LocateLibRecordByID(uint32 LibID); + /** * @brief Locate the task table entry correlating with a given task ID. * @@ -273,6 +284,85 @@ static inline bool CFE_ES_AppRecordIsMatch(const CFE_ES_AppRecord_t *AppRecPtr, CFE_ES_AppRecordGetID(AppRecPtr) == AppID); } +/** + * @brief Check if a Library record is in use or free/empty + * + * This routine checks if the Lib table entry is in use or if it is free + * + * As this dereferences fields within the record, global data must be + * locked prior to invoking this function. + * + * @param[in] LibRecPtr pointer to Lib table entry + * @returns true if the entry is in use/configured, or false if it is free/empty + */ +static inline bool CFE_ES_LibRecordIsUsed(const CFE_ES_LibRecord_t *LibRecPtr) +{ + return (LibRecPtr->RecordUsed); +} + +/** + * @brief Get the ID value from a Library table entry + * + * This routine converts the table entry back to an abstract ID. + * + * @param[in] LibRecPtr pointer to Lib table entry + * @returns LibID of entry + */ +static inline uint32 CFE_ES_LibRecordGetID(const CFE_ES_LibRecord_t *LibRecPtr) +{ + /* + * The initial implementation does not store the ID in the entry; + * the ID is simply the zero-based index into the table. + */ + return (LibRecPtr - CFE_ES_Global.LibTable); +} + +/** + * @brief Marks a Library table entry as used (not free) + * + * This sets the internal field(s) within this entry, and marks + * it as being associated with the given Lib ID. + * + * @param[in] LibRecPtr pointer to Lib table entry + * @param[in] LibID the Lib ID of this entry + */ +static inline void CFE_ES_LibRecordSetUsed(CFE_ES_LibRecord_t *LibRecPtr, uint32 LibID) +{ + LibRecPtr->RecordUsed = true; +} + +/** + * @brief Set a Library record table entry free (not used) + * + * This clears the internal field(s) within this entry, and allows the + * memory to be re-used in the future. + * + * @param[in] LibRecPtr pointer to Lib table entry + */ +static inline void CFE_ES_LibRecordSetFree(CFE_ES_LibRecord_t *LibRecPtr) +{ + LibRecPtr->RecordUsed = false; +} + +/** + * @brief Check if a Library record is a match for the given LibID + * + * This routine confirms that the previously-located record is valid + * and matches the expected Lib ID. + * + * As this dereferences fields within the record, global data must be + * locked prior to invoking this function. + * + * @param[in] LibRecPtr pointer to Lib table entry + * @param[in] LibID expected Lib ID + * @returns true if the entry matches the given Lib ID + */ +static inline bool CFE_ES_LibRecordIsMatch(const CFE_ES_LibRecord_t *LibRecPtr, uint32 LibID) +{ + return (LibRecPtr != NULL && CFE_ES_LibRecordIsUsed(LibRecPtr) && + CFE_ES_LibRecordGetID(LibRecPtr) == LibID); +} + /** * @brief Get the ID value from an Task table entry * diff --git a/fsw/cfe-core/src/inc/cfe_es.h b/fsw/cfe-core/src/inc/cfe_es.h index dafab5211..72fea6d98 100644 --- a/fsw/cfe-core/src/inc/cfe_es.h +++ b/fsw/cfe-core/src/inc/cfe_es.h @@ -178,6 +178,30 @@ static inline uint32 CFE_ES_ResourceID_FromInteger(unsigned long Value) */ int32 CFE_ES_AppID_ToIndex(uint32 AppID, uint32 *Idx); +/** + * @brief Obtain an index value correlating to an ES Library ID + * + * This calculates a zero based integer value that may be used for indexing + * into a local resource table/array. + * + * Index values are only guaranteed to be unique for resources of the same + * type. For instance, the indices corresponding to two [valid] Library + * IDs will never overlap, but the index of an Library and a library ID + * may be the same. Furthermore, indices may be reused if a resource is + * deleted and re-created. + * + * @note There is no inverse of this function - indices cannot be converted + * back to the original LibID value. The caller should retain the original ID + * for future use. + * + * @param[in] LibID Library ID to convert + * @param[out] Idx Buffer where the calculated index will be stored + * + * @return Execution status, see @ref CFEReturnCodes + * @retval #CFE_SUCCESS @copybrief CFE_SUCCESS + */ +int32 CFE_ES_LibID_ToIndex(uint32 LibID, uint32 *Idx); + /** * @brief Obtain an index value correlating to an ES Task ID * diff --git a/fsw/cfe-core/unit-test/es_UT.c b/fsw/cfe-core/unit-test/es_UT.c index 8baa76ccb..be84309aa 100644 --- a/fsw/cfe-core/unit-test/es_UT.c +++ b/fsw/cfe-core/unit-test/es_UT.c @@ -416,6 +416,7 @@ void UtTest_Setup(void) UT_ADD_TEST(TestInit); UT_ADD_TEST(TestStartupErrorPaths); UT_ADD_TEST(TestApps); + UT_ADD_TEST(TestLibs); UT_ADD_TEST(TestERLog); UT_ADD_TEST(TestTask); UT_ADD_TEST(TestPerf); @@ -937,7 +938,6 @@ void TestApps(void) int Return; int j; CFE_ES_AppInfo_t AppInfo; - char LongLibraryName[sizeof(CFE_ES_Global.LibTable[0].LibName)+1]; uint32 Id; CFE_ES_TaskRecord_t *UtTaskRecPtr; CFE_ES_AppRecord_t *UtAppRecPtr; @@ -1174,123 +1174,6 @@ void TestApps(void) "CFE_ES_AppCreate", "Module unload failure after entry point lookup failure"); - /* Test shared library loading and initialization where the initialization - * routine returns an error - */ - ES_ResetUnitTest(); - UT_SetDummyFuncRtn(-444); - Return = CFE_ES_LoadLibrary(&Id, - "filename", - "EntryPoint", - "LibName"); - UT_Report(__FILE__, __LINE__, - Return == -444 && - UT_PrintfIsInHistory(UT_OSP_MESSAGES[UT_OSP_SHARED_LIBRARY_INIT]), - "CFE_ES_LoadLibrary", - "Load shared library initialization failure"); - - - /* Test Load library returning an error on a too long library name */ - memset(&LongLibraryName[0], 'a', sizeof(CFE_ES_Global.LibTable[0].LibName)); - LongLibraryName[sizeof(CFE_ES_Global.LibTable[0].LibName)] = '\0'; - Return = CFE_ES_LoadLibrary(&Id, - "filename", - "EntryPoint", - &LongLibraryName[0]); - UT_Report(__FILE__, __LINE__, - Return == CFE_ES_BAD_ARGUMENT, - "CFE_ES_LoadLibrary", - "Load shared library bad argument (library name too long)"); - -#ifdef jphfix - /* Test successful shared library loading and initialization */ - UT_InitData(); - UT_SetDummyFuncRtn(OS_SUCCESS); - Return = CFE_ES_LoadLibrary(&Id, - "/cf/apps/tst_lib.bundle", - "TST_LIB_Init", - "TST_LIB"); - UT_Report(__FILE__, __LINE__, - Return == CFE_SUCCESS && - CFE_ES_ResourceId < CFE_PLATFORM_ES_MAX_LIBRARIES && - CFE_ES_Global.LibTable[Id].RecordUsed == true, - "CFE_ES_LoadLibrary", - "successful"); - - /* Try loading same library again, should return the DUPLICATE code */ - Return = CFE_ES_LoadLibrary(&Id, - "/cf/apps/tst_lib.bundle", - "TST_LIB_Init", - "TST_LIB"); - UT_Report(__FILE__, __LINE__, - Return == CFE_ES_LIB_ALREADY_LOADED && - Id < CFE_PLATFORM_ES_MAX_LIBRARIES && - CFE_ES_Global.LibTable[Id].RecordUsed == true, - "CFE_ES_LoadLibrary", - "Duplicate"); -#endif - /* Test shared library loading and initialization where the library - * fails to load - */ - ES_ResetUnitTest(); - UT_SetDeferredRetcode(UT_KEY(OS_ModuleLoad), 1, -1); - Return = CFE_ES_LoadLibrary(&Id, - "/cf/apps/tst_lib.bundle", - "TST_LIB_Init", - "TST_LIB"); - UT_Report(__FILE__, __LINE__, - Return == CFE_ES_ERR_LOAD_LIB, - "CFE_ES_LoadLibrary", - "Load shared library failure"); - - /* Test shared library loading and initialization where the library - * entry point symbol cannot be found - */ - ES_ResetUnitTest(); - UT_SetDeferredRetcode(UT_KEY(OS_SymbolLookup), 1, -1); - Return = CFE_ES_LoadLibrary(&Id, - "/cf/apps/tst_lib.bundle", - "TST_LIB_Init", - "TST_LIB"); - UT_Report(__FILE__, __LINE__, - Return == CFE_ES_ERR_LOAD_LIB, - "CFE_ES_LoadLibrary", - "Could not find library initialization symbol"); - - /* Test shared library loading and initialization where the library - * initialization function fails and then must be cleaned up - */ - ES_ResetUnitTest(); - UT_SetForceFail(UT_KEY(OS_remove), OS_ERROR); /* for coverage of error path */ - UT_SetForceFail(UT_KEY(dummy_function), -555); - Return = CFE_ES_LoadLibrary(&Id, - "/cf/apps/tst_lib.bundle", - "dummy_function", - "TST_LIB"); - UT_Report(__FILE__, __LINE__, - Return == -555, - "CFE_ES_LoadLibrary", - "Library initialization function failure"); - - /* Test shared library loading and initialization where there are no - * library slots available - */ - ES_ResetUnitTest(); - for (j = 0; j < CFE_PLATFORM_ES_MAX_LIBRARIES; j++) - { - CFE_ES_Global.LibTable[j].RecordUsed = true; - } - - Return = CFE_ES_LoadLibrary(&Id, - "filename", - "EntryPoint", - "LibName"); - UT_Report(__FILE__, __LINE__, - Return == CFE_ES_ERR_LOAD_LIB && - UT_PrintfIsInHistory(UT_OSP_MESSAGES[UT_OSP_LIBRARY_SLOTS]), - "CFE_ES_LoadLibrary", - "No free library slots"); - /* Test scanning and acting on the application table where the timer * expires for a waiting application */ @@ -1979,6 +1862,135 @@ void TestApps(void) "Get OS information failures"); } +void TestLibs(void) +{ + CFE_ES_LibRecord_t *UtLibRecPtr; + char LongLibraryName[sizeof(UtLibRecPtr->LibName)+1]; + uint32 Id; + uint32 j; + int32 Return; + + /* Test shared library loading and initialization where the initialization + * routine returns an error + */ + ES_ResetUnitTest(); + UT_SetDummyFuncRtn(-444); + Return = CFE_ES_LoadLibrary(&Id, + "filename", + "EntryPoint", + "LibName"); + UT_Report(__FILE__, __LINE__, + Return == -444 && + UT_PrintfIsInHistory(UT_OSP_MESSAGES[UT_OSP_SHARED_LIBRARY_INIT]), + "CFE_ES_LoadLibrary", + "Load shared library initialization failure"); + + + /* Test Load library returning an error on a too long library name */ + memset(&LongLibraryName[0], 'a', sizeof(UtLibRecPtr->LibName)); + LongLibraryName[sizeof(UtLibRecPtr->LibName)] = '\0'; + Return = CFE_ES_LoadLibrary(&Id, + "filename", + "EntryPoint", + &LongLibraryName[0]); + UT_Report(__FILE__, __LINE__, + Return == CFE_ES_BAD_ARGUMENT, + "CFE_ES_LoadLibrary", + "Load shared library bad argument (library name too long)"); + + /* Test successful shared library loading and initialization */ + UT_InitData(); + UT_SetDummyFuncRtn(OS_SUCCESS); + Return = CFE_ES_LoadLibrary(&Id, + "/cf/apps/tst_lib.bundle", + "TST_LIB_Init", + "TST_LIB"); + UT_Report(__FILE__, __LINE__, + Return == CFE_SUCCESS, + "CFE_ES_LoadLibrary", + "successful"); + + UtLibRecPtr = CFE_ES_LocateLibRecordByID(Id); + UtAssert_True(UtLibRecPtr != NULL, "CFE_ES_LoadLibrary() return valid ID"); + UtAssert_True(CFE_ES_LibRecordIsUsed(UtLibRecPtr), "CFE_ES_LoadLibrary() record used"); + + /* Try loading same library again, should return the DUPLICATE code */ + Return = CFE_ES_LoadLibrary(&Id, + "/cf/apps/tst_lib.bundle", + "TST_LIB_Init", + "TST_LIB"); + UT_Report(__FILE__, __LINE__, + Return == CFE_ES_LIB_ALREADY_LOADED, + "CFE_ES_LoadLibrary", + "Duplicate"); + UtAssert_True(Id == CFE_ES_LibRecordGetID(UtLibRecPtr), "CFE_ES_LoadLibrary() returned previous ID"); + + /* Test shared library loading and initialization where the library + * fails to load + */ + ES_ResetUnitTest(); + UT_SetDeferredRetcode(UT_KEY(OS_ModuleLoad), 1, -1); + Return = CFE_ES_LoadLibrary(&Id, + "/cf/apps/tst_lib.bundle", + "TST_LIB_Init", + "TST_LIB"); + UT_Report(__FILE__, __LINE__, + Return == CFE_ES_ERR_LOAD_LIB, + "CFE_ES_LoadLibrary", + "Load shared library failure"); + + /* Test shared library loading and initialization where the library + * entry point symbol cannot be found + */ + ES_ResetUnitTest(); + UT_SetDeferredRetcode(UT_KEY(OS_SymbolLookup), 1, -1); + Return = CFE_ES_LoadLibrary(&Id, + "/cf/apps/tst_lib.bundle", + "TST_LIB_Init", + "TST_LIB"); + UT_Report(__FILE__, __LINE__, + Return == CFE_ES_ERR_LOAD_LIB, + "CFE_ES_LoadLibrary", + "Could not find library initialization symbol"); + + /* Test shared library loading and initialization where the library + * initialization function fails and then must be cleaned up + */ + ES_ResetUnitTest(); + UT_SetForceFail(UT_KEY(OS_remove), OS_ERROR); /* for coverage of error path */ + UT_SetForceFail(UT_KEY(dummy_function), -555); + Return = CFE_ES_LoadLibrary(&Id, + "/cf/apps/tst_lib.bundle", + "dummy_function", + "TST_LIB"); + UT_Report(__FILE__, __LINE__, + Return == -555, + "CFE_ES_LoadLibrary", + "Library initialization function failure"); + + /* Test shared library loading and initialization where there are no + * library slots available + */ + ES_ResetUnitTest(); + UtLibRecPtr = CFE_ES_Global.LibTable; + for (j = 0; j < CFE_PLATFORM_ES_MAX_LIBRARIES; j++) + { + CFE_ES_LibRecordSetUsed(UtLibRecPtr, j); + ++UtLibRecPtr; + } + + Return = CFE_ES_LoadLibrary(&Id, + "filename", + "EntryPoint", + "LibName"); + UT_Report(__FILE__, __LINE__, + Return == CFE_ES_ERR_LOAD_LIB && + UT_PrintfIsInHistory(UT_OSP_MESSAGES[UT_OSP_LIBRARY_SLOTS]), + "CFE_ES_LoadLibrary", + "No free library slots"); + +} + void TestERLog(void) { int Return; diff --git a/fsw/cfe-core/unit-test/es_UT.h b/fsw/cfe-core/unit-test/es_UT.h index bbeb5c55f..806fadf65 100644 --- a/fsw/cfe-core/unit-test/es_UT.h +++ b/fsw/cfe-core/unit-test/es_UT.h @@ -333,5 +333,6 @@ void TestESMempool(void); void TestSysLog(void); void TestGenericCounterAPI(void); +void TestLibs(void); #endif /* _es_ut_h_ */