Open jphickey opened 3 years ago
To summarize - the original subject that brought up this issue was about CFE_SB_TransmitMsgValidate()
:
CFE_SB_TransmitMsg
and CFE_SB_TransmitBuffer
The questions are:
There is also a similar issue described in #1194 where a common helper routine is invoked from a context with known-good inputs, and so the error checking on the output is skipped. There are other places where the same function is invoked where the input is not known to be good - so the range check inside the function is intended for those cases. But it is called from multiple contexts - and the question there is whether the caller needs to check for error returns even though it is not possible for it to fail in the current context where the input is known to be good.
To be honest I don't think we had different opinions on this (after discussion). As an internal function it can do validation as a "helper" to the APIs, but doesn't require validation on the parameters that are guaranteed valid by the calling function. I thought the open issue was more for setting outputs on errors - bf8f481e2d87c13e59e2a32827f24dbd13c27394.
Really just 2.
I see #1194 as a separate issue (even from 2.), can we ignore possible returns when using an API from an internal function. We'd never do this for a system call even if we know it should never error, or likely any API from outside the core... can we really ignore it for our APIs? Implementations change...
CCB:2021-03-17:
Action forward for this issue - document internal checks.
Is your feature request related to a problem? Please describe. As discussed in #1197 we need to have some consensus on the proper level of argument checking for internal helper functions.
Sometimes internal functions have tests to validate their inputs (range check etc) on behalf of the caller, in the case where several public APIs need to repeat the same tests -- putting these in a helper can reduce repeated code and make all APIs consistent in their validation (a good thing).
But in other cases the helper is invoked from contexts where the inputs are never out of range or pointers can never be NULL. Testing for such inputs can be redundant.
Describe the solution you'd like Need to confirm/reach consensus on whether functions like CFE_SB_TransmitMsgValidate() in CFE SB need to validate all their arguments. Probably should better document which args are tested and why - and if there are limitations on other args (e.g. certain args are assumed to be non-null).
Additional context This just causes some confusion during review and probably some additional comments/documentation could help.
See thread here: https://github.com/nasa/cFE/pull/1197#discussion_r585070230
Requester Info Joseph Hickey, Vantage Systems, Inc.