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

Apps should check/verify msg BEFORE calling handler #82

Closed
3 tasks done
jphickey opened this issue Mar 21, 2023 · 0 comments · Fixed by #88
Closed
3 tasks done

Apps should check/verify msg BEFORE calling handler #82

jphickey opened this issue Mar 21, 2023 · 0 comments · Fixed by #88

Comments

@jphickey
Copy link
Contributor

Checklist (Please check before submitting)

  • I reviewed the Contributing Guide.
  • I reviewed the README file to see if the feature is in the major future work.
  • I performed a cursory search to see if the feature request is relevant, not redundant, nor in conflict with other tickets.

Is your feature request related to a problem? Please describe.
This remains an area with coding pattern discrepancies between CFE core and CFS apps, and also different between CFS apps to some degree as well.

The CFE core and sample app (which is supposed to be the example of "best practice") do validation on the message before calling the handler. For example:

        case SAMPLE_APP_NOOP_CC:
            if (SAMPLE_APP_VerifyCmdLength(&SBBufPtr->Msg, sizeof(SAMPLE_APP_NoopCmd_t)))
            {
                SAMPLE_APP_Noop((SAMPLE_APP_NoopCmd_t *)SBBufPtr);
            }

This is different from LC, which does a similar check, but done inside each handler, for example:

LC/fsw/src/lc_cmds.c

Lines 143 to 150 in 2f177ae

if (LC_VerifyMsgLength(&BufPtr->Msg, ExpectedLength))
{
/*
** Ignore AP sample requests if disabled at the application level
*/
if (LC_AppData.CurrentLCState != LC_STATE_DISABLED)
{
/*

Describe the solution you'd like
CFS Apps should follow the best practices/patterns set forth in the framework code. (there are reasons for the pattern being recommended practice)

Additional context
The pattern recommended in the framework (checking before calling, as done in sample_app) has several advantages:

  1. Each command handler function has a unique type-safe prototype, that cannot be interchanged with another handler without triggering a type mismatch compiler error.
  2. All typecasting/conversions are confined to one place, and it is nearby to the place that the verification is done - which eases maintainability because check and conversion are all in close proximity and any mismatches will be more visible.
  3. It spreads out the cyclomatic complexity. In the non-recommended pattern, there is a case where the length check fails, and the entire handler is essentially skipped. This adds to the cyclomatic complexity of every handler. In the recommended pattern, those checks are done prior to the invocation of the handler, so the handler can focus solely on its intended purpose - doing the command itself.

Requester Info
Joseph Hickey, Vantage Systems, Inc.

jphickey added a commit to jphickey/LC that referenced this issue Apr 12, 2023
Isolate the message verification and dispatch from the general message
processing.  Functions in the "cmds" file now strictly handle the
command content, and do not get involved in general validation.
jphickey added a commit to jphickey/LC that referenced this issue Apr 12, 2023
Isolate the message verification and dispatch from the general message
processing.  Functions in the "cmds" file now strictly handle the
command content, and do not get involved in general validation.
jphickey added a commit to jphickey/LC that referenced this issue Apr 17, 2023
Isolate the message verification and dispatch from the general message
processing.  Functions in the "cmds" file now strictly handle the
command content, and do not get involved in general validation.
jphickey added a commit to jphickey/LC that referenced this issue Apr 17, 2023
Isolate the message verification and dispatch from the general message
processing.  Functions in the "cmds" file now strictly handle the
command content, and do not get involved in general validation.
dzbaker added a commit that referenced this issue Apr 17, 2023
Fix #82, use separate dispatcher for messages
@dmknutsen dmknutsen added this to the Equuleus milestone May 26, 2023
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.

3 participants