Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split up and simplify control flow in CFE_TBL_Register() #2386

Closed
2 tasks done
thnkslprpt opened this issue Jun 24, 2023 · 0 comments · Fixed by #2387
Closed
2 tasks done

Split up and simplify control flow in CFE_TBL_Register() #2386

thnkslprpt opened this issue Jun 24, 2023 · 0 comments · Fixed by #2387

Comments

@thnkslprpt
Copy link
Contributor

Checklist

  • I reviewed the Contributing Guide.
  • I performed a cursory search to see if the bug report is relevant, not redundant, nor in conflict with other tickets.

Describe the bug

Code snips

CFE_Status_t CFE_TBL_Register(CFE_TBL_Handle_t *TblHandlePtr, const char *Name, size_t Size, uint16 TblOptionFlags,
CFE_TBL_CallbackFuncPtr_t TblValidationFuncPtr)
{
CFE_TBL_AccessDescriptor_t *AccessDescPtr = NULL;
CFE_TBL_RegistryRec_t * RegRecPtr = NULL;
CFE_TBL_LoadBuff_t * WorkingBufferPtr;
CFE_TBL_CritRegRec_t * CritRegRecPtr = NULL;
int32 Status;
size_t NameLen;
int16 RegIndx;
CFE_ES_AppId_t ThisAppId;
char AppName[OS_MAX_API_NAME] = {"UNKNOWN"};
char TblName[CFE_TBL_MAX_FULL_NAME_LEN] = {""};
CFE_TBL_Handle_t AccessIndex;
if (TblHandlePtr == NULL || Name == NULL)
{
return CFE_TBL_BAD_ARGUMENT;
}
/* Check to make sure calling application is legit */
Status = CFE_ES_GetAppID(&ThisAppId);
if (Status == CFE_SUCCESS)
{
/* Assume we can't make a table and return a bad handle for now */
*TblHandlePtr = CFE_TBL_BAD_TABLE_HANDLE;
/* Make sure specified table name is not too long or too short */
NameLen = strlen(Name);
if ((NameLen > CFE_MISSION_TBL_MAX_NAME_LENGTH) || (NameLen == 0))
{
Status = CFE_TBL_ERR_INVALID_NAME;
/* Perform a buffer overrun safe copy of name for debug log message */
strncpy(TblName, Name, sizeof(TblName) - 1);
TblName[sizeof(TblName) - 1] = '\0';
CFE_ES_WriteToSysLog("%s: Table Name (%s) is bad length (%d)", __func__, TblName, (int)NameLen);
}
else
{
/* Generate application specific table name */
CFE_TBL_FormTableName(TblName, Name, ThisAppId);
/* Make sure the specified size is acceptable */
/* Single buffered tables are allowed to be up to CFE_PLATFORM_TBL_MAX_SNGL_TABLE_SIZE */
/* Double buffered tables are allowed to be up to CFE_PLATFORM_TBL_MAX_DBL_TABLE_SIZE */
if (Size == 0)
{
Status = CFE_TBL_ERR_INVALID_SIZE;
CFE_ES_WriteToSysLog("%s: Table %s has size of zero\n", __func__, Name);
}
else if ((Size > CFE_PLATFORM_TBL_MAX_SNGL_TABLE_SIZE) &&
((TblOptionFlags & CFE_TBL_OPT_BUFFER_MSK) == CFE_TBL_OPT_SNGL_BUFFER))
{
Status = CFE_TBL_ERR_INVALID_SIZE;
CFE_ES_WriteToSysLog("%s: Single Buffered Table '%s' has size %d > %d\n", __func__, Name, (int)Size,
CFE_PLATFORM_TBL_MAX_SNGL_TABLE_SIZE);
}
else if ((Size > CFE_PLATFORM_TBL_MAX_DBL_TABLE_SIZE) &&
((TblOptionFlags & CFE_TBL_OPT_BUFFER_MSK) == CFE_TBL_OPT_DBL_BUFFER))
{
Status = CFE_TBL_ERR_INVALID_SIZE;
CFE_ES_WriteToSysLog("%s: Dbl Buffered Table '%s' has size %d > %d\n", __func__, Name, (int)Size,
CFE_PLATFORM_TBL_MAX_DBL_TABLE_SIZE);
}
/* Verify Table Option settings are legal */
/* User defined table addresses are only legal for single buffered, dump-only, non-critical tables */
if ((TblOptionFlags & CFE_TBL_OPT_USR_DEF_MSK) == (CFE_TBL_OPT_USR_DEF_ADDR & CFE_TBL_OPT_USR_DEF_MSK))
{
if (((TblOptionFlags & CFE_TBL_OPT_BUFFER_MSK) == CFE_TBL_OPT_DBL_BUFFER) ||
((TblOptionFlags & CFE_TBL_OPT_LD_DMP_MSK) == CFE_TBL_OPT_LOAD_DUMP) ||
((TblOptionFlags & CFE_TBL_OPT_CRITICAL_MSK) == CFE_TBL_OPT_CRITICAL))
{
Status = CFE_TBL_ERR_INVALID_OPTIONS;
CFE_ES_WriteToSysLog("%s: User Def tbl '%s' cannot be dbl buff, load/dump or critical\n", __func__,
Name);
}
}
else if ((TblOptionFlags & CFE_TBL_OPT_LD_DMP_MSK) == CFE_TBL_OPT_DUMP_ONLY)
{
/* Dump Only tables cannot be double buffered, nor critical */
if (((TblOptionFlags & CFE_TBL_OPT_BUFFER_MSK) == CFE_TBL_OPT_DBL_BUFFER) ||
((TblOptionFlags & CFE_TBL_OPT_CRITICAL_MSK) == CFE_TBL_OPT_CRITICAL))
{
Status = CFE_TBL_ERR_INVALID_OPTIONS;
CFE_ES_WriteToSysLog("%s: Dump Only tbl '%s' cannot be double buffered or critical\n", __func__,
Name);
}
}
}
}
else /* Application ID was invalid */
{
CFE_ES_WriteToSysLog("%s: Bad AppId(%lu)\n", __func__, CFE_RESOURCEID_TO_ULONG(ThisAppId));
}
/* If input parameters appear acceptable, register the table */
if (Status == CFE_SUCCESS)
{
/* Lock Registry for update. This prevents two applications from */
/* trying to register/share tables at the same location at the same time */
CFE_TBL_LockRegistry();
/* Check for duplicate table name */
RegIndx = CFE_TBL_FindTableInRegistry(TblName);
/* Check to see if table is already in the registry */
if (RegIndx != CFE_TBL_NOT_FOUND)
{
/* Get pointer to Registry Record Entry to speed up processing */
RegRecPtr = &CFE_TBL_Global.Registry[RegIndx];
/* If this app previously owned the table, then allow them to re-register */
if (CFE_RESOURCEID_TEST_EQUAL(RegRecPtr->OwnerAppId, ThisAppId))
{
/* If the new table is the same size as the old, then no need to reallocate memory */
if (Size != RegRecPtr->Size)
{
/* If the new size is different, the old table must deleted */
/* but this function can't do that because it is probably shared */
/* and is probably still being accessed. Someone else will need */
/* to clean up this mess. */
Status = CFE_TBL_ERR_DUPLICATE_DIFF_SIZE;
CFE_ES_WriteToSysLog("%s: Attempt to register existing table ('%s') with different size(%d!=%d)\n",
__func__, TblName, (int)Size, (int)RegRecPtr->Size);
}
else
{
/* Warn calling application that this is a duplicate registration */
Status = CFE_TBL_WARN_DUPLICATE;
/* Find the existing access descriptor for the table */
/* and return the same handle that was returned previously */
AccessIndex = RegRecPtr->HeadOfAccessList;
while ((AccessIndex != CFE_TBL_END_OF_LIST) && (*TblHandlePtr == CFE_TBL_BAD_TABLE_HANDLE))
{
if ((CFE_TBL_Global.Handles[AccessIndex].UsedFlag == true) &&
CFE_RESOURCEID_TEST_EQUAL(CFE_TBL_Global.Handles[AccessIndex].AppId, ThisAppId) &&
(CFE_TBL_Global.Handles[AccessIndex].RegIndex == RegIndx))
{
*TblHandlePtr = AccessIndex;
}
else
{
AccessIndex = CFE_TBL_Global.Handles[AccessIndex].NextLink;
}
}
}
}
else /* Duplicate named table owned by another Application */
{
Status = CFE_TBL_ERR_DUPLICATE_NOT_OWNED;
CFE_ES_WriteToSysLog("%s: App(%lu) Registering Duplicate Table '%s' owned by App(%lu)\n", __func__,
CFE_RESOURCEID_TO_ULONG(ThisAppId), TblName,
CFE_RESOURCEID_TO_ULONG(RegRecPtr->OwnerAppId));
}
}
else /* Table not already in registry */
{
/* Locate empty slot in table registry */
RegIndx = CFE_TBL_FindFreeRegistryEntry();
}
/* Check to make sure we found a free entry in registry */
if (RegIndx == CFE_TBL_NOT_FOUND)
{
Status = CFE_TBL_ERR_REGISTRY_FULL;
CFE_ES_WriteToSysLog("%s: Registry full\n", __func__);
}
/* If this is a duplicate registration, no other work is required */
if (Status != CFE_TBL_WARN_DUPLICATE)
{
/* Search Access Descriptor Array for free Descriptor */
*TblHandlePtr = CFE_TBL_FindFreeHandle();
/* Check to make sure there was a handle available */
if (*TblHandlePtr == CFE_TBL_END_OF_LIST)
{
Status = CFE_TBL_ERR_HANDLES_FULL;
CFE_ES_WriteToSysLog("%s: No more free handles\n", __func__);
}
/* If no errors, then initialize the table registry entry */
/* and return the registry index to the caller as the handle */
if ((Status & CFE_SEVERITY_BITMASK) != CFE_SEVERITY_ERROR)
{
/* Get pointer to Registry Record Entry to speed up processing */
RegRecPtr = &CFE_TBL_Global.Registry[RegIndx];
/* Initialize Registry Record to default settings */
CFE_TBL_InitRegistryRecord(RegRecPtr);
if ((TblOptionFlags & CFE_TBL_OPT_USR_DEF_MSK) != (CFE_TBL_OPT_USR_DEF_ADDR & CFE_TBL_OPT_USR_DEF_MSK))
{
RegRecPtr->UserDefAddr = false;
/* Allocate the memory buffer(s) for the table and inactive table, if necessary */
Status = CFE_ES_GetPoolBuf(&RegRecPtr->Buffers[0].BufferPtr, CFE_TBL_Global.Buf.PoolHdl, Size);
if (Status < 0)
{
CFE_ES_WriteToSysLog("%s: 1st Buf Alloc GetPool fail Stat=0x%08X MemPoolHndl=0x%08lX\n",
__func__, (unsigned int)Status,
CFE_RESOURCEID_TO_ULONG(CFE_TBL_Global.Buf.PoolHdl));
}
else
{
/* Zero the memory buffer */
Status = CFE_SUCCESS;
memset(RegRecPtr->Buffers[0].BufferPtr, 0x0, Size);
}
}
else
{
/* Set buffer pointer to NULL for user defined address tables */
RegRecPtr->Buffers[0].BufferPtr = NULL;
RegRecPtr->UserDefAddr = true;
}
if (((TblOptionFlags & CFE_TBL_OPT_DBL_BUFFER) == CFE_TBL_OPT_DBL_BUFFER) &&
((Status & CFE_SEVERITY_BITMASK) != CFE_SEVERITY_ERROR))
{
/* Allocate memory for the dedicated secondary buffer */
Status = CFE_ES_GetPoolBuf(&RegRecPtr->Buffers[1].BufferPtr, CFE_TBL_Global.Buf.PoolHdl, Size);
if (Status < 0)
{
CFE_ES_WriteToSysLog("%s: 2nd Buf Alloc GetPool fail Stat=0x%08X MemPoolHndl=0x%08lX\n",
__func__, (unsigned int)Status,
CFE_RESOURCEID_TO_ULONG(CFE_TBL_Global.Buf.PoolHdl));
}
else
{
/* Zero the dedicated secondary buffer */
Status = CFE_SUCCESS;
memset(RegRecPtr->Buffers[1].BufferPtr, 0x0, Size);
}
RegRecPtr->ActiveBufferIndex = 0;
RegRecPtr->DoubleBuffered = true;
}
else /* Single Buffered Table */
{
RegRecPtr->DoubleBuffered = false;
RegRecPtr->ActiveBufferIndex = 0;
}
if ((Status & CFE_SEVERITY_BITMASK) != CFE_SEVERITY_ERROR)
{
/* Save the size of the table */
RegRecPtr->Size = Size;
/* Save the Callback function pointer */
RegRecPtr->ValidationFuncPtr = TblValidationFuncPtr;
/* Save Table Name in Registry */
strncpy(RegRecPtr->Name, TblName, sizeof(RegRecPtr->Name) - 1);
RegRecPtr->Name[sizeof(RegRecPtr->Name) - 1] = '\0';
/* Set the "Dump Only" flag to value based upon selected option */
if ((TblOptionFlags & CFE_TBL_OPT_LD_DMP_MSK) == CFE_TBL_OPT_DUMP_ONLY)
{
RegRecPtr->DumpOnly = true;
}
else
{
RegRecPtr->DumpOnly = false;
}
/* Initialize the Table Access Descriptor */
AccessDescPtr = &CFE_TBL_Global.Handles[*TblHandlePtr];
AccessDescPtr->AppId = ThisAppId;
AccessDescPtr->LockFlag = false;
AccessDescPtr->Updated = false;
if ((RegRecPtr->DumpOnly) && (!RegRecPtr->UserDefAddr))
{
/* Dump Only Tables are assumed to be loaded at all times */
/* unless the address is specified by the application. In */
/* that case, it isn't loaded until the address is specified */
RegRecPtr->TableLoadedOnce = true;
}
AccessDescPtr->RegIndex = RegIndx;
AccessDescPtr->PrevLink = CFE_TBL_END_OF_LIST; /* We are the head of the list */
AccessDescPtr->NextLink = CFE_TBL_END_OF_LIST; /* We are the end of the list */
AccessDescPtr->UsedFlag = true;
/* Make sure the Table Registry entry points to First Access Descriptor */
RegRecPtr->HeadOfAccessList = *TblHandlePtr;
/* If the table is a critical table, allocate space for it in the Critical Data Store */
/* OR locate its previous incarnation there and extract its previous contents */
if ((TblOptionFlags & CFE_TBL_OPT_CRITICAL_MSK) == CFE_TBL_OPT_CRITICAL)
{
/* Register a CDS under the table name and determine if the table already exists there */
Status = CFE_ES_RegisterCDSEx(&RegRecPtr->CDSHandle, Size, TblName, true);
if (Status == CFE_ES_CDS_ALREADY_EXISTS)
{
Status = CFE_TBL_GetWorkingBuffer(&WorkingBufferPtr, RegRecPtr, true);
if (Status != CFE_SUCCESS)
{
/* Unable to get a working buffer - this error is not really */
/* possible at this point during table registration. But we */
/* do need to handle the error case because if the function */
/* call did fail, WorkingBufferPtr would be a NULL pointer. */
CFE_ES_GetAppName(AppName, ThisAppId, sizeof(AppName));
CFE_ES_WriteToSysLog("%s: Failed to get work buffer for '%s.%s' (ErrCode=0x%08X)\n",
__func__, AppName, Name, (unsigned int)Status);
}
else
{
/* CDS exists for this table - try to restore the data */
Status = CFE_ES_RestoreFromCDS(WorkingBufferPtr->BufferPtr, RegRecPtr->CDSHandle);
if (Status != CFE_SUCCESS)
{
CFE_ES_GetAppName(AppName, ThisAppId, sizeof(AppName));
CFE_ES_WriteToSysLog("%s: Failed to recover '%s.%s' from CDS (ErrCode=0x%08X)\n",
__func__, AppName, Name, (unsigned int)Status);
}
}
if (Status != CFE_SUCCESS)
{
/* Treat a restore from existing CDS error the same as */
/* after a power-on reset (CDS was created but is empty) */
Status = CFE_SUCCESS;
}
else
{
/* Try to locate the associated information in the Critical Table Registry */
CFE_TBL_FindCriticalTblInfo(&CritRegRecPtr, RegRecPtr->CDSHandle);
if ((CritRegRecPtr != NULL) && (CritRegRecPtr->TableLoadedOnce))
{
strncpy(WorkingBufferPtr->DataSource, CritRegRecPtr->LastFileLoaded,
sizeof(WorkingBufferPtr->DataSource) - 1);
WorkingBufferPtr->DataSource[sizeof(WorkingBufferPtr->DataSource) - 1] = '\0';
WorkingBufferPtr->FileCreateTimeSecs = CritRegRecPtr->FileCreateTimeSecs;
WorkingBufferPtr->FileCreateTimeSubSecs = CritRegRecPtr->FileCreateTimeSubSecs;
strncpy(RegRecPtr->LastFileLoaded, CritRegRecPtr->LastFileLoaded,
sizeof(RegRecPtr->LastFileLoaded) - 1);
RegRecPtr->LastFileLoaded[sizeof(RegRecPtr->LastFileLoaded) - 1] = '\0';
RegRecPtr->TimeOfLastUpdate.Seconds = CritRegRecPtr->TimeOfLastUpdate.Seconds;
RegRecPtr->TimeOfLastUpdate.Subseconds = CritRegRecPtr->TimeOfLastUpdate.Subseconds;
RegRecPtr->TableLoadedOnce = CritRegRecPtr->TableLoadedOnce;
/* Compute the CRC on the specified table buffer */
WorkingBufferPtr->Crc = CFE_ES_CalculateCRC(
WorkingBufferPtr->BufferPtr, RegRecPtr->Size, 0, CFE_MISSION_ES_DEFAULT_CRC);
/* Make sure everyone who sees the table knows that it has been updated */
CFE_TBL_NotifyTblUsersOfUpdate(RegRecPtr);
/* Make sure the caller realizes the contents have been initialized */
Status = CFE_TBL_INFO_RECOVERED_TBL;
}
else
{
/* If an error occurred while trying to get the previous contents registry info, */
/* Log the error in the System Log and pretend like we created a new CDS */
CFE_ES_GetAppName(AppName, ThisAppId, sizeof(AppName));
CFE_ES_WriteToSysLog("%s: Failed to recover '%s.%s' info from CDS TblReg\n",
__func__, AppName, Name);
Status = CFE_SUCCESS;
}
}
/* Mark the table as critical for future reference */
RegRecPtr->CriticalTable = true;
}
if (Status == CFE_SUCCESS)
{
/* Find and initialize a free entry in the Critical Table Registry */
CFE_TBL_FindCriticalTblInfo(&CritRegRecPtr, CFE_ES_CDS_BAD_HANDLE);
if (CritRegRecPtr != NULL)
{
CritRegRecPtr->CDSHandle = RegRecPtr->CDSHandle;
strncpy(CritRegRecPtr->Name, TblName, sizeof(CritRegRecPtr->Name) - 1);
CritRegRecPtr->Name[sizeof(CritRegRecPtr->Name) - 1] = '\0';
CritRegRecPtr->FileCreateTimeSecs = 0;
CritRegRecPtr->FileCreateTimeSubSecs = 0;
CritRegRecPtr->LastFileLoaded[0] = '\0';
CritRegRecPtr->TimeOfLastUpdate.Seconds = 0;
CritRegRecPtr->TimeOfLastUpdate.Subseconds = 0;
CritRegRecPtr->TableLoadedOnce = false;
CFE_ES_CopyToCDS(CFE_TBL_Global.CritRegHandle, CFE_TBL_Global.CritReg);
}
else
{
CFE_ES_WriteToSysLog("%s: Failed to find a free Crit Tbl Reg Rec for '%s'\n", __func__,
RegRecPtr->Name);
}
/* Mark the table as critical for future reference */
RegRecPtr->CriticalTable = true;
}
else if (Status != CFE_TBL_INFO_RECOVERED_TBL)
{
CFE_ES_WriteToSysLog("%s: Failed to register '%s.%s' as a CDS (ErrCode=0x%08X)\n", __func__,
AppName, Name, (unsigned int)Status);
/* Notify caller that although they asked for it to be critical, it isn't */
Status = CFE_TBL_WARN_NOT_CRITICAL;
}
}
/* The last step of the registration process is claiming ownership. */
/* By making it the last step, other APIs do not have to lock registry */
/* to share the table or get its address because registry entries that */
/* are unowned are not checked to see if they match names, etc. */
RegRecPtr->OwnerAppId = ThisAppId;
}
}
}
/* Unlock Registry for update */
CFE_TBL_UnlockRegistry();
}
/* On Error conditions, notify ground of screw up */
if (Status < 0)
{
/* Make sure the returned handle is invalid when an error occurs */
*TblHandlePtr = CFE_TBL_BAD_TABLE_HANDLE;
/* Translate AppID of caller into App Name */
CFE_ES_GetAppName(AppName, ThisAppId, sizeof(AppName));
CFE_EVS_SendEventWithAppID(CFE_TBL_REGISTER_ERR_EID, CFE_EVS_EventType_ERROR, CFE_TBL_Global.TableTaskAppId,
"%s Failed to Register '%s', Status=0x%08X", AppName, TblName, (unsigned int)Status);
}
return Status;
}

Expected behavior
Split up CFE_TBL_Register, and try to simplify control flow and conditional logic a bit - it currently stands at 451 lines and several dozen branches...

Will make testing and future maintenance easier.

Reporter Info
Avi Weiss @thnkslprpt

thnkslprpt added a commit to thnkslprpt/cFE that referenced this issue Jun 24, 2023
dzbaker added a commit that referenced this issue Mar 21, 2024
…gister

Fix #2386, Split up and simplify control flow in CFE_TBL_Register()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants