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

Fix #1338, check status of call to CFE_ES_CDS_CachePreload #1373

Merged

Conversation

jphickey
Copy link
Contributor

Describe the contribution
Confirm that the call returned CFE_SUCCESS before continuing.

Fixes #1338

Testing performed
Build and sanity check CFE, run all unit tests

Expected behavior changes
None

System(s) tested on
Ubuntu 20.04

Additional context
Note that the error/failure condition here is effectively dead code from the beginning. Not even coverage test can exercise it, because its a void function, and the args are largely fixed/constant values. So this now shows up as untested lines in the coverage report.

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Apr 16, 2021
Copy link
Contributor

@skliper skliper left a comment

Choose a reason for hiding this comment

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

If it can't fail, just comment as such and ignore the return. It's ignored in the one other calling location:

CFE_ES_CDS_CachePreload(&CDS->Cache, BdPtr, Offset, sizeof(CFE_ES_GenPoolBD_t));

@skliper
Copy link
Contributor

skliper commented Apr 18, 2021

... its a void function

I don't follow:

int32 CFE_ES_CDS_CachePreload(CFE_ES_CDS_AccessCache_t *Cache, const void *Source, size_t Offset, size_t Size);

The function cannot fail in this context
@jphickey jphickey force-pushed the fix-1338-check-status branch from 624d586 to 85bfba2 Compare April 20, 2021 13:44
@jphickey
Copy link
Contributor Author

Updated to just ignore the return of CFE_ES_CDS_CachePreload here. We still need to init the local status to CFE_SUCCESS, though (it had been doing that too).

Anyway, by "void function" I was referring to the parent, i.e. CFE_ES_ClearCDS() - there are no args to this function that coverage test can change, to get it to call CFE_ES_CDS_CachePreload any differently. It always calls it with the same args, and they are always good/within range so there is no way to get the failure path from this function.

@skliper
Copy link
Contributor

skliper commented Apr 20, 2021

Would it make sense to use sizeof the appropriate Cache->Data union member (instead of the underlying type)? Then it really could never get out of sync?

EDIT - this refers to the other case where it's used, since I think it's already done the "better" way here.

CFE_ES_CDS_CachePreload(&CDS->Cache, BdPtr, Offset, sizeof(CFE_ES_GenPoolBD_t));

EDIT1 - maybe (void) and add the comment this is explicitly ignored because it can't fail?

@astrogeco
Copy link
Contributor

astrogeco commented Apr 21, 2021

CCB:2021-04-21 APPROVED with changes

  • Function would never fail
  • See skliper's comment above

@astrogeco astrogeco added the CCB:Approved Indicates code review and approval by community CCB label Apr 21, 2021
@astrogeco astrogeco changed the base branch from main to integration-candidate April 28, 2021 01:08
@astrogeco astrogeco added IC:2021-04-27 and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Apr 28, 2021
@astrogeco astrogeco merged commit 069646d into nasa:integration-candidate Apr 28, 2021
astrogeco added a commit to nasa/cFS that referenced this pull request Apr 28, 2021
nasa/cFE#1379, memory pool pointer type
nasa/cFE#1289, ES child task functional test
nasa/cFE#1289, typo in macro name
nasa/cFE#1286, Remove broken BUILDDIR reference
nasa/cFE#1305, remove option for "osal_compatible"
nasa/cFE#1374, CFE_SUCCESS constant type
nasa/cFE#1316, Remove Unused Error Codes
nasa/cFE#1370, better warning about malformed startup line
nasa/cFE#1373, check status of call to `CFE_ES_CDS_CachePreload`
nasa/cFE#1384, update documentation for `CFE_ES_DeleteCDS`
astrogeco added a commit to nasa/cFS that referenced this pull request Apr 29, 2021
Combines:

nasa/cFE#1431
nasa/osal#975
nasa/sample_lib#61

Includes:

nasa/cFE#1379, memory pool pointer type
nasa/cFE#1289, ES child task functional test
nasa/cFE#1289, typo in macro name
nasa/cFE#1286, Remove broken BUILDDIR reference
nasa/cFE#1305, remove option for "osal_compatible"
nasa/cFE#1374, CFE_SUCCESS constant type
nasa/cFE#1316, Remove Unused Error Codes
nasa/cFE#1370, better warning about malformed startup line
nasa/cFE#1373, check status of call to `CFE_ES_CDS_CachePreload`
nasa/cFE#1384, update documentation for `CFE_ES_DeleteCDS`
nasa/cFE#1385, exception logic when app/task is not found
nasa/cFE#1372, error if alignment size not a power of two
nasa/cFE#1368, remove unneeded CFE_ES_SYSLOG_APPEND macro
nasa/cFE#1382, improve documentation for resourceID patterns
nasa/cFE#1371, assert `CFE_RESOURCEID_MAX` is a bitmask

nasa/osal#972, update documentation for read/write
nasa/osal#966, add "handler" feature to utassert stub API
nasa/osal#953, Adds local makefile and bundle/local unit test actions with coverage verification
nasa/osal#971, socket accept using incorrect record
nasa/osal#959, move async console option

nasa/sample_lib#60, replace direct ref to ArgPtr with macro
@jphickey jphickey deleted the fix-1338-check-status branch May 14, 2021 14:23
@skliper skliper added this to the 7.0.0 milestone Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:Approved Indicates code review and approval by community CCB CFS-40
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CFE_ES_ClearCDS: check status before while loop to zero
3 participants