CFE_TBL_Load doesn't reset LoadInProgress when called on a locked table

[nmullane]

Describe the bug
When calling CFE_TBL_Load on a table that is locked it correctly returns the status CFE_TBL_INFO_TABLE_LOCKED.

cFE/modules/tbl/fsw/src/cfe_tbl_internal.c

Lines 971 to 976 in c4ae5b2

	if (LockStatus)
	{
	Status = CFE_TBL_INFO_TABLE_LOCKED;
	CFE_ES_WriteToSysLog("%s: Unable to update locked table Handle=%d\n", __func__, TblHandle);
	}

However, this branch skips over the function CFE_TBL_NotifyTblUsersOfUpdate which resets the variable RegRecPtr->LoadInProgress. Without this variable being reset to CFE_TBL_NO_LOAD_IN_PROGRESS, all future calls to CFE_TBL_Load will fail.

cFE/modules/tbl/fsw/src/cfe_tbl_internal.c

Lines 1029 to 1047 in c4ae5b2

	void CFE_TBL_NotifyTblUsersOfUpdate(CFE_TBL_RegistryRec_t *RegRecPtr)
	{
	CFE_TBL_Handle_t AccessIterator;
	/* Reset Load in Progress Values */
	RegRecPtr->LoadInProgress = CFE_TBL_NO_LOAD_IN_PROGRESS;
	RegRecPtr->TimeOfLastUpdate = CFE_TIME_GetTime();
	/* Clear notification of pending load (as well as NO LOAD) and notify everyone of update */
	RegRecPtr->LoadPending = false;
	RegRecPtr->TableLoadedOnce = true;
	AccessIterator = RegRecPtr->HeadOfAccessList;
	while (AccessIterator != CFE_TBL_END_OF_LIST)
	{
	CFE_TBL_Global.Handles[AccessIterator].Updated = true;
	AccessIterator = CFE_TBL_Global.Handles[AccessIterator].NextLink;
	}
	}

Once the table address is correctly released, all subsequent calls to CFE_TBL_Load will still fail and return CFE_TBL_ERR_LOAD_IN_PROGRESS even though the last load failed when the table was locked.

cFE/modules/tbl/fsw/src/cfe_tbl_api.c

Lines 745 to 753 in c4ae5b2

	/* Loads by an Application are not allowed if a table load is already in progress */
	if (RegRecPtr->LoadInProgress != CFE_TBL_NO_LOAD_IN_PROGRESS)
	{
	CFE_EVS_SendEventWithAppID(CFE_TBL_LOAD_IN_PROGRESS_ERR_EID, CFE_EVS_EventType_ERROR,
	CFE_TBL_Global.TableTaskAppId, "%s: Load already in progress for '%s'", AppName,
	RegRecPtr->Name);
	return CFE_TBL_ERR_LOAD_IN_PROGRESS;
	}

This can be fixed by calling CFE_TBL_Manage before trying to load data.

To Reproduce
On my fork for #1734 the test TestReleaseAddress in the file modules/cfe_testcase/src/tbl_content_access_test.c requires a call to CFE_TBL_Manage before we can load data to the table after releasing the address for it. The following code snippet is from this file.

    /* Attempt to load while address is locked */
    LoadTable(&TestTable, CFE_SUCCESS);
    UtAssert_INT32_EQ(CFE_TBL_GetAddress(&TblPtr, CFE_FT_Global.TblHandle), CFE_TBL_INFO_UPDATED);
    LoadTable(&TestTable, CFE_TBL_INFO_TABLE_LOCKED);

    /* Release and try again */
    UtAssert_INT32_EQ(CFE_TBL_ReleaseAddress(CFE_FT_Global.TblHandle), CFE_SUCCESS);
    CFE_TBL_Manage(CFE_FT_Global.TblHandle);
    LoadTable(&TestTable, CFE_SUCCESS);

Expected behavior
After a failed table load because the table was locked, I should be able to release the address of a table and then call CFE_TBL_Load without needing to call CFE_TBL_Manage in between.

System observed on:

• Hardware: PC
• OS: Ubuntu 20.04

Reporter Info
Niall Mullane - GSFC 582 Intern

[skliper]

Labeling as "bug" (for now at least). In practice there really is very low or minimal impact since loading while holding the address violates the API (and is rejected as expected) and also apps should be calling CFE_TBL_Manage regularly, after which the in progress status gets cleared. So even if an app does experience this "bug" it's somewhat self-healing, although a valid load may get rejected before the status is cleared. Impact would really depend on the app implementation, and the "right way" is really to not load with the address acquired to begin with. Could imagine some complex management scheme where GetAddres/Load/ReleaseAddress/Load/Manage would never end up loading, but really apps shouldn't be doing this. Either way, not currently targeting a fix for Caelum... could as future work clear the status on error or similar.

[zanzaben]

Will also need to update the functional test that currently is working around this.