From 11a7d435f988c626de5e8e4b15a12fdc87a4a436 Mon Sep 17 00:00:00 2001 From: Joseph Hickey Date: Fri, 2 Apr 2021 17:15:29 -0400 Subject: [PATCH] Fix #871, allow OSAL re-initialization Replaces the separate "Initialized" and "Shutdown" flags with a single state flag. This simplifies things and makes for a single source of truth for the state of OSAL globally. In particular this allows for: - Multiple invocations of OS_API_Init() - subsequent calls can be ignored - Deleting of any internal objects that did get created if OS_API_Init() fails (leaves system in same state as when it started) - Allows Re-initialization of OSAL after OS_ApplicationShutdown() - may be relevant when running unit tests several times without rebooting. --- src/os/posix/src/os-impl-console.c | 2 +- src/os/rtems/src/os-impl-console.c | 2 +- src/os/shared/inc/os-shared-common.h | 25 +++++++---- src/os/shared/src/osapi-common.c | 43 +++++++++++++++---- src/os/shared/src/osapi-idmap.c | 13 ++++-- src/os/shared/src/osapi-printf.c | 4 +- src/os/vxworks/src/os-impl-console.c | 2 +- .../shared/src/coveragetest-common.c | 21 +++++---- .../shared/src/coveragetest-idmap.c | 30 ++++++------- .../shared/src/coveragetest-printf.c | 4 +- 10 files changed, 95 insertions(+), 51 deletions(-) diff --git a/src/os/posix/src/os-impl-console.c b/src/os/posix/src/os-impl-console.c index 672770fc7..44da26742 100644 --- a/src/os/posix/src/os-impl-console.c +++ b/src/os/posix/src/os-impl-console.c @@ -100,7 +100,7 @@ static void *OS_ConsoleTask_Entry(void *arg) local = OS_OBJECT_TABLE_GET(OS_impl_console_table, token); /* Loop forever (unless shutdown is set) */ - while (OS_SharedGlobalVars.ShutdownFlag != OS_SHUTDOWN_MAGIC_NUMBER) + while (OS_SharedGlobalVars.GlobalState != OS_SHUTDOWN_MAGIC_NUMBER) { OS_ConsoleOutput_Impl(&token); sem_wait(&local->data_sem); diff --git a/src/os/rtems/src/os-impl-console.c b/src/os/rtems/src/os-impl-console.c index 7aa2622e3..08cac6524 100644 --- a/src/os/rtems/src/os-impl-console.c +++ b/src/os/rtems/src/os-impl-console.c @@ -123,7 +123,7 @@ static void OS_ConsoleTask_Entry(rtems_task_argument arg) local = OS_OBJECT_TABLE_GET(OS_impl_console_table, token); /* Loop forever (unless shutdown is set) */ - while (OS_SharedGlobalVars.ShutdownFlag != OS_SHUTDOWN_MAGIC_NUMBER) + while (OS_SharedGlobalVars.GlobalState != OS_SHUTDOWN_MAGIC_NUMBER) { OS_ConsoleOutput_Impl(&token); rtems_semaphore_obtain(local->data_sem, RTEMS_WAIT, RTEMS_NO_TIMEOUT); diff --git a/src/os/shared/inc/os-shared-common.h b/src/os/shared/inc/os-shared-common.h index 32927226f..c41354792 100644 --- a/src/os/shared/inc/os-shared-common.h +++ b/src/os/shared/inc/os-shared-common.h @@ -32,15 +32,23 @@ #include "os-shared-globaldefs.h" /* - * A "magic number" that when written to the "ShutdownFlag" member - * of the global state structure indicates an active shutdown request. + * Flag values for the "GlobalState" member the global state structure */ -#define OS_SHUTDOWN_MAGIC_NUMBER 0xABADC0DE +#define OS_INIT_MAGIC_NUMBER 0xBE57C0DE /**< Indicates that OS_API_Init() has been successfully run */ +#define OS_SHUTDOWN_MAGIC_NUMBER 0xABADC0DE /**< Indicates that a system shutdown request is pending */ /* Global variables that are common between implementations */ struct OS_shared_global_vars { - bool Initialized; + /* + * Tracks whether OS_API_Init() has been called or if + * there is a shutdown request pending. + * + * After boot/first startup this should have 0 (from BSS clearing) + * After OS_API_Init() is called this has OS_INIT_MAGIC_NUMBER + * After OS_ApplicationShutdown() this has OS_SHUTDOWN_MAGIC_NUMBER + */ + volatile uint32 GlobalState; /* * The console device ID used for OS_printf() calls @@ -48,13 +56,12 @@ struct OS_shared_global_vars osal_id_t PrintfConsoleId; /* - * PrintfEnabled and ShutdownFlag are marked "volatile" + * PrintfEnabled and GlobalState are marked "volatile" * because they are updated and read by different threads */ - volatile bool PrintfEnabled; - volatile uint32 ShutdownFlag; - uint32 MicroSecPerTick; - uint32 TicksPerSecond; + volatile bool PrintfEnabled; + uint32 MicroSecPerTick; + uint32 TicksPerSecond; /* * The event handler is an application-defined callback diff --git a/src/os/shared/src/osapi-common.c b/src/os/shared/src/osapi-common.c index 2543d9887..8f0bd55ea 100644 --- a/src/os/shared/src/osapi-common.c +++ b/src/os/shared/src/osapi-common.c @@ -58,9 +58,8 @@ #include "os-shared-time.h" OS_SharedGlobalVars_t OS_SharedGlobalVars = { - .Initialized = false, + .GlobalState = 0, .PrintfEnabled = false, - .ShutdownFlag = 0, .MicroSecPerTick = 0, /* invalid, _must_ be set by implementation init */ .TicksPerSecond = 0, /* invalid, _must_ be set by implementation init */ .EventHandler = NULL, @@ -112,13 +111,29 @@ int32 OS_API_Init(void) osal_objtype_t idtype; uint32 microSecPerSec; - if (OS_SharedGlobalVars.Initialized != false) + /* + * If OSAL is already initialized, not really a big issue, just return. + * This is not typically expected though, so its worth a debug statement. + * + * However this can validly occur when running tests on some platforms + * without a reset/reload between invocations. + */ + if (OS_SharedGlobalVars.GlobalState == OS_INIT_MAGIC_NUMBER) { - OS_DEBUG("WARNING: BUG - initialization function called multiple times\n"); - return OS_ERROR; + OS_DEBUG("NOTE: ignored redundant OS_API_Init() call\n"); + return OS_SUCCESS; } - OS_SharedGlobalVars.Initialized = true; + /* Wipe global state structure to be sure everything is clean */ + memset(&OS_SharedGlobalVars, 0, sizeof(OS_SharedGlobalVars)); + + /* Reset debug to default level if enabled */ +#if defined(OSAL_CONFIG_DEBUG_PRINTF) + OS_SharedGlobalVars.DebugLevel = 1; +#endif + + /* Set flag that says OSAL has been initialized */ + OS_SharedGlobalVars.GlobalState = OS_INIT_MAGIC_NUMBER; /* Initialize the common table that everything shares */ return_code = OS_ObjectIdInit(); @@ -216,6 +231,18 @@ int32 OS_API_Init(void) (long)OS_SharedGlobalVars.TicksPerSecond); } + if (return_code != OS_SUCCESS) + { + /* + * Some part of init failed, so set global flag that says OSAL is in shutdown state. + * + * In particular if certain internal resources (such as the console utility task) + * were created, this should cause those tasks to self-exit such that the system + * is ultimately returned to the same state it started in. + */ + OS_SharedGlobalVars.GlobalState = OS_SHUTDOWN_MAGIC_NUMBER; + } + return (return_code); } /* end OS_API_Init */ @@ -359,7 +386,7 @@ void OS_IdleLoop() * In most "real" embedded systems, this will never happen. * However it will happen in debugging situations (CTRL+C, etc). */ - while (OS_SharedGlobalVars.ShutdownFlag != OS_SHUTDOWN_MAGIC_NUMBER) + while (OS_SharedGlobalVars.GlobalState != OS_SHUTDOWN_MAGIC_NUMBER) { OS_IdleLoop_Impl(); } @@ -377,7 +404,7 @@ void OS_ApplicationShutdown(uint8 flag) { if (flag == true) { - OS_SharedGlobalVars.ShutdownFlag = OS_SHUTDOWN_MAGIC_NUMBER; + OS_SharedGlobalVars.GlobalState = OS_SHUTDOWN_MAGIC_NUMBER; } /* diff --git a/src/os/shared/src/osapi-idmap.c b/src/os/shared/src/osapi-idmap.c index 758f74307..ab3a71883 100644 --- a/src/os/shared/src/osapi-idmap.c +++ b/src/os/shared/src/osapi-idmap.c @@ -324,7 +324,11 @@ int32 OS_ObjectIdTransactionInit(OS_lock_mode_t lock_mode, osal_objtype_t idtype { memset(token, 0, sizeof(*token)); - if (OS_SharedGlobalVars.Initialized == false) + /* + * Confirm that OSAL has been fully initialized before allowing any transactions + */ + if (OS_SharedGlobalVars.GlobalState != OS_INIT_MAGIC_NUMBER && + OS_SharedGlobalVars.GlobalState != OS_SHUTDOWN_MAGIC_NUMBER) { return OS_ERROR; } @@ -333,7 +337,7 @@ int32 OS_ObjectIdTransactionInit(OS_lock_mode_t lock_mode, osal_objtype_t idtype * only "exclusive" locks allowed after shutdown request (this is mode used for delete). * All regular ops will be blocked. */ - if (OS_SharedGlobalVars.ShutdownFlag == OS_SHUTDOWN_MAGIC_NUMBER && lock_mode != OS_LOCK_MODE_EXCLUSIVE) + if (OS_SharedGlobalVars.GlobalState == OS_SHUTDOWN_MAGIC_NUMBER && lock_mode != OS_LOCK_MODE_EXCLUSIVE) { return OS_ERR_INCORRECT_OBJ_STATE; } @@ -1211,7 +1215,10 @@ int32 OS_ObjectIdAllocateNew(osal_objtype_t idtype, const char *name, OS_object_ { int32 return_code; - if (OS_SharedGlobalVars.ShutdownFlag == OS_SHUTDOWN_MAGIC_NUMBER) + /* + * No new objects can be created after Shutdown request + */ + if (OS_SharedGlobalVars.GlobalState == OS_SHUTDOWN_MAGIC_NUMBER) { return OS_ERR_INCORRECT_OBJ_STATE; } diff --git a/src/os/shared/src/osapi-printf.c b/src/os/shared/src/osapi-printf.c index a21f55b36..c2aede39d 100644 --- a/src/os/shared/src/osapi-printf.c +++ b/src/os/shared/src/osapi-printf.c @@ -258,7 +258,7 @@ void OS_printf(const char *String, ...) BUGCHECK((String) != NULL, ) - if (!OS_SharedGlobalVars.Initialized) + if (OS_SharedGlobalVars.GlobalState != OS_INIT_MAGIC_NUMBER) { /* * Catch some historical mis-use of the OS_printf() call. @@ -277,7 +277,7 @@ void OS_printf(const char *String, ...) * If debugging is not enabled, then this message will be silently * discarded. */ - OS_DEBUG("BUG: OS_printf() called before init: %s", String); + OS_DEBUG("BUG: OS_printf() called when OSAL not initialized: %s", String); } else if (OS_SharedGlobalVars.PrintfEnabled) { diff --git a/src/os/vxworks/src/os-impl-console.c b/src/os/vxworks/src/os-impl-console.c index 9fc250814..a6b816492 100644 --- a/src/os/vxworks/src/os-impl-console.c +++ b/src/os/vxworks/src/os-impl-console.c @@ -107,7 +107,7 @@ int OS_VxWorks_ConsoleTask_Entry(int arg) local = OS_OBJECT_TABLE_GET(OS_impl_console_table, token); /* Loop forever (unless shutdown is set) */ - while (OS_SharedGlobalVars.ShutdownFlag != OS_SHUTDOWN_MAGIC_NUMBER) + while (OS_SharedGlobalVars.GlobalState != OS_SHUTDOWN_MAGIC_NUMBER) { OS_ConsoleOutput_Impl(&token); if (semTake(local->datasem, WAIT_FOREVER) == ERROR) diff --git a/src/unit-test-coverage/shared/src/coveragetest-common.c b/src/unit-test-coverage/shared/src/coveragetest-common.c index 479105e2d..f8b3bd4e6 100644 --- a/src/unit-test-coverage/shared/src/coveragetest-common.c +++ b/src/unit-test-coverage/shared/src/coveragetest-common.c @@ -95,34 +95,37 @@ void Test_OS_API_Init(void) /* Execute Test */ Test_MicroSecPerTick = 0; Test_TicksPerSecond = 0; - OS_SharedGlobalVars.Initialized = false; + OS_SharedGlobalVars.GlobalState = 0; OSAPI_TEST_FUNCTION_RC(OS_API_Init(), OS_ERROR); + UtAssert_UINT32_EQ(OS_SharedGlobalVars.GlobalState, OS_SHUTDOWN_MAGIC_NUMBER); Test_MicroSecPerTick = 1000; Test_TicksPerSecond = 1000; - OS_SharedGlobalVars.Initialized = false; + OS_SharedGlobalVars.GlobalState = 0; OSAPI_TEST_FUNCTION_RC(OS_API_Init(), OS_SUCCESS); Test_MicroSecPerTick = 1000; Test_TicksPerSecond = 1001; - OS_SharedGlobalVars.Initialized = false; + OS_SharedGlobalVars.GlobalState = 0; OSAPI_TEST_FUNCTION_RC(OS_API_Init(), OS_SUCCESS); + UtAssert_UINT32_EQ(OS_SharedGlobalVars.GlobalState, OS_INIT_MAGIC_NUMBER); - /* Second call should return ERROR */ - OSAPI_TEST_FUNCTION_RC(OS_API_Init(), OS_ERROR); + /* Second call should return SUCCESS (but is a no-op) */ + OSAPI_TEST_FUNCTION_RC(OS_API_Init(), OS_SUCCESS); + UtAssert_UINT32_EQ(OS_SharedGlobalVars.GlobalState, OS_INIT_MAGIC_NUMBER); /* other error paths */ - OS_SharedGlobalVars.Initialized = false; + OS_SharedGlobalVars.GlobalState = 0; UT_SetDefaultReturnValue(UT_KEY(OS_ObjectIdInit), -222); OSAPI_TEST_FUNCTION_RC(OS_API_Init(), -222); UT_ResetState(UT_KEY(OS_ObjectIdInit)); - OS_SharedGlobalVars.Initialized = false; + OS_SharedGlobalVars.GlobalState = 0; UT_SetDefaultReturnValue(UT_KEY(OS_API_Impl_Init), -333); OSAPI_TEST_FUNCTION_RC(OS_API_Init(), -333); UT_ResetState(UT_KEY(OS_API_Impl_Init)); - OS_SharedGlobalVars.Initialized = false; + OS_SharedGlobalVars.GlobalState = 0; UT_SetDefaultReturnValue(UT_KEY(OS_TaskAPI_Init), -444); OSAPI_TEST_FUNCTION_RC(OS_API_Init(), -444); UT_ResetState(UT_KEY(OS_TaskAPI_Init)); @@ -255,6 +258,8 @@ void Test_OS_IdleLoopAndShutdown(void) */ uint32 CallCount = 0; + OS_SharedGlobalVars.GlobalState = OS_INIT_MAGIC_NUMBER; + UT_SetHookFunction(UT_KEY(OS_IdleLoop_Impl), SetShutdownFlagHook, NULL); OS_IdleLoop(); diff --git a/src/unit-test-coverage/shared/src/coveragetest-idmap.c b/src/unit-test-coverage/shared/src/coveragetest-idmap.c index 83590c08b..70b6e7329 100644 --- a/src/unit-test-coverage/shared/src/coveragetest-idmap.c +++ b/src/unit-test-coverage/shared/src/coveragetest-idmap.c @@ -482,13 +482,13 @@ void Test_OS_ObjectIdGetById(void) OS_object_token_t token2; /* verify that the call returns ERROR when not initialized */ - OS_SharedGlobalVars.Initialized = false; + OS_SharedGlobalVars.GlobalState = 0; actual = OS_ObjectIdGetById(OS_LOCK_MODE_NONE, 0, OS_OBJECT_ID_UNDEFINED, &token1); expected = OS_ERROR; UtAssert_True(actual == expected, "OS_ObjectIdGetById(uninitialized) (%ld) == OS_ERROR", (long)actual); /* set "true" for the remainder of tests */ - OS_SharedGlobalVars.Initialized = true; + OS_SharedGlobalVars.GlobalState = OS_INIT_MAGIC_NUMBER; OS_ObjectIdCompose_Impl(OS_OBJECT_TYPE_OS_TASK, 1000, &refobjid); OS_ObjectIdToArrayIndex(OS_OBJECT_TYPE_OS_TASK, refobjid, &local_idx); @@ -516,17 +516,16 @@ void Test_OS_ObjectIdGetById(void) UtAssert_True(rptr->refcount == 0, "refcount (%u) == 0", (unsigned int)rptr->refcount); /* attempt to get non-exclusive lock during shutdown should fail */ - OS_SharedGlobalVars.ShutdownFlag = OS_SHUTDOWN_MAGIC_NUMBER; - expected = OS_ERR_INCORRECT_OBJ_STATE; - actual = OS_ObjectIdGetById(OS_LOCK_MODE_NONE, OS_OBJECT_TYPE_OS_TASK, refobjid, &token1); + OS_SharedGlobalVars.GlobalState = OS_SHUTDOWN_MAGIC_NUMBER; + expected = OS_ERR_INCORRECT_OBJ_STATE; + actual = OS_ObjectIdGetById(OS_LOCK_MODE_NONE, OS_OBJECT_TYPE_OS_TASK, refobjid, &token1); UtAssert_True(actual == expected, "OS_ObjectIdGetById() (%ld) == OS_ERR_INCORRECT_OBJ_STATE", (long)actual); - OS_SharedGlobalVars.ShutdownFlag = 0; + OS_SharedGlobalVars.GlobalState = OS_INIT_MAGIC_NUMBER; /* attempt to get lock for invalid type object should fail */ expected = OS_ERR_INCORRECT_OBJ_TYPE; actual = OS_ObjectIdGetById(OS_LOCK_MODE_NONE, 0xFFFF, refobjid, &token1); UtAssert_True(actual == expected, "OS_ObjectIdGetById() (%ld) == OS_ERR_INCORRECT_OBJ_TYPE", (long)actual); - OS_SharedGlobalVars.ShutdownFlag = 0; /* clear out state entry */ memset(&OS_global_task_table[local_idx], 0, sizeof(OS_global_task_table[local_idx])); @@ -702,10 +701,10 @@ void Test_OS_ObjectIdAllocateNew(void) actual = OS_ObjectIdGetByName(OS_LOCK_MODE_NONE, OS_OBJECT_TYPE_OS_TASK, "UT_alloc", &token); UtAssert_True(actual == expected, "OS_ObjectIdGetByName() (%ld) == OS_ERR_INCORRECT_OBJ_STATE", (long)actual); - OS_SharedGlobalVars.ShutdownFlag = OS_SHUTDOWN_MAGIC_NUMBER; - expected = OS_ERR_INCORRECT_OBJ_STATE; - actual = OS_ObjectIdAllocateNew(OS_OBJECT_TYPE_OS_TASK, "UT_alloc", &token); - OS_SharedGlobalVars.ShutdownFlag = 0; + OS_SharedGlobalVars.GlobalState = OS_SHUTDOWN_MAGIC_NUMBER; + expected = OS_ERR_INCORRECT_OBJ_STATE; + actual = OS_ObjectIdAllocateNew(OS_OBJECT_TYPE_OS_TASK, "UT_alloc", &token); + OS_SharedGlobalVars.GlobalState = OS_INIT_MAGIC_NUMBER; UtAssert_True(actual == expected, "OS_ObjectIdAllocate() (%ld) == OS_ERR_INCORRECT_OBJ_STATE", (long)actual); expected = OS_ERR_INCORRECT_OBJ_TYPE; @@ -773,8 +772,7 @@ void Test_OS_ObjectIdTransaction(void) UtAssert_STUB_COUNT(OS_Lock_Global_Impl, 0); /* shutdown will prevent transactions */ - OS_SharedGlobalVars.Initialized = true; - OS_SharedGlobalVars.ShutdownFlag = OS_SHUTDOWN_MAGIC_NUMBER; + OS_SharedGlobalVars.GlobalState = OS_SHUTDOWN_MAGIC_NUMBER; OSAPI_TEST_FUNCTION_RC(OS_ObjectIdTransactionInit(OS_LOCK_MODE_GLOBAL, OS_OBJECT_TYPE_OS_BINSEM, &token), OS_ERR_INCORRECT_OBJ_STATE); UtAssert_UINT32_EQ(token.lock_mode, OS_LOCK_MODE_NONE); @@ -793,7 +791,7 @@ void Test_OS_ObjectIdTransaction(void) UtAssert_STUB_COUNT(OS_Unlock_Global_Impl, 1); /* other cases for normal operating mode */ - OS_SharedGlobalVars.ShutdownFlag = 0; + OS_SharedGlobalVars.GlobalState = OS_INIT_MAGIC_NUMBER; OSAPI_TEST_FUNCTION_RC(OS_ObjectIdTransactionInit(OS_LOCK_MODE_GLOBAL, OS_OBJECT_TYPE_OS_COUNTSEM, &token), OS_SUCCESS); UtAssert_UINT32_EQ(token.lock_mode, OS_LOCK_MODE_GLOBAL); @@ -1081,10 +1079,10 @@ void Osapi_Test_Setup(void) /* * The OS_SharedGlobalVars is also used here, but set the - * "Initialized" field to true by default, as this is needed by most tests. + * "GlobalState" field to init by default, as this is needed by most tests. */ memset(&OS_SharedGlobalVars, 0, sizeof(OS_SharedGlobalVars)); - OS_SharedGlobalVars.Initialized = true; + OS_SharedGlobalVars.GlobalState = OS_INIT_MAGIC_NUMBER; } /* diff --git a/src/unit-test-coverage/shared/src/coveragetest-printf.c b/src/unit-test-coverage/shared/src/coveragetest-printf.c index e27780d1d..967781511 100644 --- a/src/unit-test-coverage/shared/src/coveragetest-printf.c +++ b/src/unit-test-coverage/shared/src/coveragetest-printf.c @@ -68,13 +68,13 @@ void Test_OS_printf(void) /* catch case where OS_printf called before init */ OS_SharedGlobalVars.PrintfConsoleId = OS_OBJECT_ID_UNDEFINED; - OS_SharedGlobalVars.Initialized = false; + OS_SharedGlobalVars.GlobalState = 0; OS_printf("UnitTest1"); UtAssert_True(OS_console_table[0].WritePos == 0, "WritePos (%lu) >= 0", (unsigned long)OS_console_table[0].WritePos); /* because printf is disabled, the call count should _not_ increase here */ - OS_SharedGlobalVars.Initialized = true; + OS_SharedGlobalVars.GlobalState = OS_INIT_MAGIC_NUMBER; OS_printf_disable(); OS_printf("UnitTest2"); UtAssert_True(OS_console_table[0].WritePos == 0, "WritePos (%lu) >= 0",