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

Clean up unused error codes/checks #416

Closed
dmknutsen opened this issue Nov 16, 2023 · 0 comments · Fixed by #417
Closed

Clean up unused error codes/checks #416

dmknutsen opened this issue Nov 16, 2023 · 0 comments · Fixed by #417

Comments

@dmknutsen
Copy link
Contributor

Checklist (Please check before submitting)

  • [x ] I reviewed the Contributing Guide.
  • [x ] I reviewed the CF README.md file to see if the feature is in the major future work.
  • [x ] 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.
Update code to fix 2 issues identified by IV&V below:

Issue #1:
The code in question is in cf_cfdp_s.c lines 747 and 752.

CF_CFDP_S_SendEof(t) on line 747 does not return the error code CF_SEND_PDU_ERROR (checked by the else if on line 752).
CF_CFDP_S_SendEof() merely returns the result of CF_CFDP_SendEof(), and in that function, it only returns status codes CFE_SUCCESS and CF_SEND_PDU_NO_BUF_AVAIL_ERROR.

If the code is checking for CF_SEND_PDU_ERROR on line 752, but function CF_CFDP_SendEof() isn’t returning that code, then is there potentially a missing check somewhere? Perhaps there was an intent to implement/return that error code within CF_CFDP_SendEof() but it didn’t happen.

There is also an issue with the code coverage. Gcov is reporting 100%, and that is probably because the unit test file cf_cfdp_s_tests.c is written to test both error codes CF_SEND_PDU_NO_BUF_AVAIL_ERROR and CF_SEND_PDU_ERROR, using a stubbed version of CF_CFDP_SendEof(). Even though CF_CFDP_SendEof() does not actually return CF_SEND_PDU_ERROR.

Issue #2:

The code in question is in cf_cfdp_s.c lines 196-201.

CF_CFDP_SendFd() on line 196 ONLY returns CFE_SUCCESS. There is a code comment (cf_cfdp.c line 371) that says “/* this should check if any encoding error occurred /”, however there is no subsequent check. This *missing check would’ve returned the CF_SEND_PDU_ERROR code that is checked on line 201.
Line 197 checks for CF_SEND_PDU_NO_BUF_AVAIL_ERROR. As we know from the first point above, CF_CFDP_SendFd() ONLY returns CFE_SUCCESS. But actually this check is redundant.

Explanation: The error code CF_SEND_PDU_NO_BUF_AVAIL_ERROR is typically set when a call to CF_CFDP_ConstructPduHeader() fails [references: cf_cfdp.c line 327-334, 426-433, 484-488, and 517-524]. Within CF_CFDP_S_SendFileData(), at the beginning, there is already a call to CF_CFDP_ConstructPduHeader() and a check for its failure on line 122. Therefore, there is no need to check for CF_SEND_PDU_NO_BUF_AVAIL_ERROR again within the same function.

Code coverage - similar comment above about the unit testing both error codes CF_SEND_PDU_NO_BUF_AVAIL_ERROR and CF_SEND_PDU_ERROR, using a stubbed version of CF_CFDP_SendFd().

Describe the solution you'd like
At one point the CF 'send' routines could return CF_SEND_PDU_ERROR. This is no longer the case and the code should be cleaned up to remove it. The second issue is valid as well, both unnecessary checks should be removed - or additional checks within CF_CFDP_SendFd should be added.

Requester Info
Dan Knutsen
NASA Goddard

@dmknutsen dmknutsen self-assigned this Nov 16, 2023
avan989 pushed a commit to avan989/CF that referenced this issue Nov 24, 2023
avan989 pushed a commit to avan989/CF that referenced this issue Nov 24, 2023
clean up cf_cfdp_s.c for unused error code
avan989 pushed a commit to avan989/CF that referenced this issue Nov 27, 2023
Fix nasa#416, clean up cf_cfdp_s.c for unused error code and update unit
test
@dmknutsen dmknutsen removed their assignment Nov 30, 2023
dzbaker added a commit that referenced this issue Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant