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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Mar 21, 2024
Merged

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

merged 2 commits into from
Mar 21, 2024

Conversation

thnkslprpt
Copy link
Contributor

Checklist

Describe the contribution

Main change here was to pull out the logical blocks of work into helper functions.

Additional Changes
Name Size Validation:
Simplified the size checks to just a single check and report (old, new). Can revert this change if the extra verbosity is desired.

Previously, if any of the name size checks failed (and CFE_TBL_ERR_INVALID_SIZE Status was set), the code would then still run on to verify the specific table options flags (and re-assign another error code on top of CFE_TBL_ERR_INVALID_SIZE if this failed). Have updated this logic to only enter the TblOptionsFlags checks if the size checks pass successfully. No need to continue execution if error status already set.

Main Registration Block Control Flow:
Previously the code would search for an access descriptor even if already in error status here. This is unnecessary if already in error status so have changed this conditional to just check for == CFE_SUCCESS (link).

In 3 locations inside the this same main table registration block there were status guards of the form if (Status & CFE_SEVERITY_BITMASK) != CFE_SEVERITY_ERROR).

I have reduced the nesting in this section, and simplified these checks to just check for == CFE_SUCCESS. The only non-negative 'warning' status possible there was CFE_TBL_WARN_DUPLICATE, which was already being filtered out anyway. Also, these warning (i.e. non-success, non-error) status/return codes are intended to be deprecated anyway.

Minor Simplification:
Simplified this true/false assignment to this.

Note there are a few open issues already for specific issues inside CFE_TBL_Register:

Testing performed
GitHub CI actions all passing successfully (incl. Build + Run, Unit/Functional Tests etc.). Local testing also conducted to confirm all variables and syslog strings printing as expected etc.

Expected behavior changes
Minor control-flow changes noted above. In general this PR will:

  • make unit testing much simpler/easier
  • ease future maintenance
  • reduce technical debt
  • ease use of cFE by developers

System(s) tested on
Debian GNU/Linux 11 (bullseye)
Current main branch of cFS bundle.

Contributor Info
Avi Weiss @thnkslprpt

github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 24, 2023
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@jphickey
Copy link
Contributor

Thanks @thnkslprpt !! Good work here, I like this .... sorry things sit so long. Reviewing this now and hopefully we can merge it, as the existing TBL code is really hard to work with.

@thnkslprpt thnkslprpt force-pushed the fix-2386-split-up-cfe-tbl-register branch from 580ed70 to 368cdc4 Compare March 12, 2024 21:32
@thnkslprpt
Copy link
Contributor Author

Thanks @thnkslprpt !! Good work here, I like this .... sorry things sit so long. Reviewing this now and hopefully we can merge it, as the existing TBL code is really hard to work with.

@jphickey
No problem at all Joe. We all do what we can with the time we have available.
I'll be interested to hear your feedback on this. FYI - I rebased it to main just now.
Cheers

github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 12, 2024
View reviewed changes
modules/tbl/fsw/src/cfe_tbl_internal.c
Comment on lines +1528 to +1540
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 == *RegIndxPtr))
{
*TblHandlePtr = AccessIndex;
}
else
{
AccessIndex = CFE_TBL_Global.Handles[AccessIndex].NextLink;
}
}

Check warning

Code scanning / CodeQL-coding-standard

Unbounded loop Warning

This loop does not have a fixed bound.
modules/tbl/fsw/src/cfe_tbl_internal.c
*
*-----------------------------------------------------------------*/

CFE_Status_t CFE_TBL_RestoreTableDataFromCDS(CFE_TBL_RegistryRec_t *RegRecPtr, const char *AppName, const char *Name,

Check notice

Code scanning / CodeQL

Function too long Note

CFE_TBL_RestoreTableDataFromCDS has too many lines (81, while 60 are allowed).
modules/tbl/fsw/src/cfe_tbl_internal.c
* See description in header file for argument/return detail
*
*-----------------------------------------------------------------*/
CFE_Status_t CFE_TBL_CheckForDuplicateRegistration(int16 *RegIndxPtr, const char *TblName,

Check notice

Code scanning / CodeQL-coding-standard

Function too long Note

CFE_TBL_CheckForDuplicateRegistration has too many lines (77, while 60 are allowed).
modules/tbl/fsw/src/cfe_tbl_internal.c
* See description in header file for argument/return detail
*
*-----------------------------------------------------------------*/
void CFE_TBL_RegisterWithCriticalTableRegistry(CFE_TBL_CritRegRec_t *CritRegRecPtr, CFE_TBL_RegistryRec_t *RegRecPtr,

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
modules/tbl/fsw/src/cfe_tbl_internal.c
*
*-----------------------------------------------------------------*/

CFE_Status_t CFE_TBL_RestoreTableDataFromCDS(CFE_TBL_RegistryRec_t *RegRecPtr, const char *AppName, const char *Name,

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
modules/tbl/fsw/src/cfe_tbl_internal.c
* See description in header file for argument/return detail
*
*-----------------------------------------------------------------*/
CFE_Status_t CFE_TBL_AllocateTableBuffer(CFE_TBL_RegistryRec_t *RegRecPtr, size_t Size)

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
modules/tbl/fsw/src/cfe_tbl_internal.c
* See description in header file for argument/return detail
*
*-----------------------------------------------------------------*/
CFE_Status_t CFE_TBL_CheckForDuplicateRegistration(int16 *RegIndxPtr, const char *TblName,

Check notice

Code scanning / CodeQL-coding-standard

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
modules/tbl/fsw/src/cfe_tbl_internal.c
* See description in header file for argument/return detail
*
*-----------------------------------------------------------------*/
CFE_Status_t CFE_TBL_ValidateTableOptions(const char *Name, uint16 TblOptionFlags)

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
modules/tbl/fsw/src/cfe_tbl_internal.c
* See description in header file for argument/return detail
*
*-----------------------------------------------------------------*/
CFE_Status_t CFE_TBL_ValidateTableSize(const char *Name, size_t Size, uint16 TblOptionFlags)

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
modules/tbl/fsw/src/cfe_tbl_internal.c
* See description in header file for argument/return detail
*
*-----------------------------------------------------------------*/
CFE_Status_t CFE_TBL_ValidateTableName(const char *Name)

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
@jphickey
Copy link
Contributor

I like this a whole lot better than what was in there. It's far more readable and easy to understand what's going on. The cyclic complexity measurement of CFE_TBL_Register() went from 52 to 20 after merging this change (still could be even better, but this is a huge improvement).

I've confirmed that everything seems to work as far as functional tests go - the only concern is that there are some missing lines in the coverage. I may be able to address that.

jphickey
jphickey approved these changes Mar 13, 2024
View reviewed changes
Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to fixup/check some coverage tests.

jphickey added a commit to thnkslprpt/cFE that referenced this pull request Mar 14, 2024
@jphickey
Patch nasa#2387, update for unit test coverage
77f3d29 
Corrects some missed branches and lines in the unit test
jphickey added a commit to thnkslprpt/cFE that referenced this pull request Mar 14, 2024
@jphickey
Patch nasa#2387, update for unit test coverage
4f94df0 
Corrects some missed branches and lines in the unit test
@jphickey jphickey force-pushed the fix-2386-split-up-cfe-tbl-register branch from 77f3d29 to 4f94df0 Compare March 14, 2024 14:02
jphickey added a commit to thnkslprpt/cFE that referenced this pull request Mar 14, 2024
@jphickey
Patch nasa#2387, update for unit test coverage
ddbf91f 
Corrects some missed branches and lines in the unit test
@jphickey jphickey force-pushed the fix-2386-split-up-cfe-tbl-register branch from 4f94df0 to ddbf91f Compare March 14, 2024 14:03
@jphickey
Fix nasa#2387, update for unit test coverage
8962f47 
Corrects some missed branches and lines in the unit test
@jphickey jphickey force-pushed the fix-2386-split-up-cfe-tbl-register branch from ddbf91f to 8962f47 Compare March 14, 2024 15:46
@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Mar 14, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 14, 2024
View reviewed changes
modules/tbl/ut-coverage/tbl_UT.c
@@ -3198,14 +3198,14 @@
UtAssert_StrnCmp(TblInfo1.LastFileLoaded, MyFilename, sizeof(TblInfo1.LastFileLoaded) - 4, "%s == %s, %ld",
TblInfo1.LastFileLoaded, MyFilename, (long)sizeof(TblInfo1.LastFileLoaded) - 4);

if(maxPathLenDiff >= 0)
if (maxPathLenDiff >= 0)

Check warning

Code scanning / CodeQL

Comparison result is always the same Warning

Comparison is always true because maxPathLenDiff >= 0.
@dzbaker dzbaker added CCB:Approved Indicates code review and approval by community CCB and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Mar 14, 2024
@thnkslprpt
Copy link
Contributor Author

I've confirmed that everything seems to work as far as functional tests go - the only concern is that there are some missing lines in the coverage. I may be able to address that.

Thanks for adding those coverage tests in.

dzbaker added a commit to nasa/cFS that referenced this pull request Mar 21, 2024
@dzbaker
IC: equuleus-rc1+dev8
20334c8 
*Combines:*

cFE equuleus-rc1+dev114
sample_app equuleus-rc1+dev46

**Includes:**

*cFE*
- nasa/cFE#2387
- nasa/cFE#2531

*sample_app*
- nasa/sample_app#218
- nasa/sample_app#216

Co-authored by: Joseph Hickey <[email protected]>
Co-authored by: Avi Weiss <[email protected]>
This was referenced Mar 21, 2024
Merged
Merged
@dzbaker dzbaker closed this in 8962f47 Mar 21, 2024
@dzbaker dzbaker merged commit d1c9d7d into nasa:main Mar 21, 2024
22 checks passed
dzbaker added a commit to nasa/cFS that referenced this pull request Mar 21, 2024
@dzbaker
IC: equuleus-rc1+dev8
5b678e2 
*Combines:*

cFE equuleus-rc1+dev114
sample_app equuleus-rc1+dev46

**Includes:**

*cFE*
- nasa/cFE#2387
- nasa/cFE#2531

*sample_app*
- nasa/sample_app#218
- nasa/sample_app#216

Co-authored by: Joseph Hickey <[email protected]>
Co-authored by: Avi Weiss <[email protected]>
@thnkslprpt thnkslprpt deleted the fix-2386-split-up-cfe-tbl-register branch March 22, 2024 02:49
This was referenced Mar 23, 2024
Closed
Open
@thnkslprpt thnkslprpt mentioned this pull request Aug 9, 2024
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jphickey jphickey jphickey approved these changes

Assignees
No one assigned
Labels
CCB:Approved Indicates code review and approval by community CCB enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Split up and simplify control flow in CFE_TBL_Register()
4 participants
@thnkslprpt @jphickey @dzbaker @chillfig