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

ES missing branch in CFE_ES_RegisterCDSEx, overwrite success check doesn't make sense #1929

Closed
skliper opened this issue Sep 2, 2021 · 2 comments · Fixed by #2329
Closed

ES missing branch in CFE_ES_RegisterCDSEx, overwrite success check doesn't make sense #1929

skliper opened this issue Sep 2, 2021 · 2 comments · Fixed by #2329
Labels
coverage enhancement

Comments

@skliper
Copy link
Contributor

skliper commented Sep 2, 2021

Is your feature request related to a problem? Please describe.
I suspect there were code changes that made the check redundant/pointless since RegUpdateStatus only gets set if Status == CFE_SUCCESS before this point, so there's no way for it to not be success and for Status to also not be success.

     443         [ +  + ]:         14 :     if (RegUpdateStatus != CFE_SUCCESS)
     444                 :            :     {
     445                 :          2 :         CFE_ES_WriteToSysLog("%s: Failed to update CDS Registry (Stat=0x%08X)\n", __func__,
     446                 :            :                              (unsigned int)RegUpdateStatus);
     447                 :            : 
     448                 :            :         /*
     449                 :            :          * Return failure only if this was the primary error,
     450                 :            :          * do not overwrite a preexisting error.
     451                 :            :          */
     452         [ +  - ]:          2 :         if (Status == CFE_SUCCESS)
     453                 :            :         {
     454                 :          2 :             Status = RegUpdateStatus;
     455                 :            :         }
     456                 :            :     }

here:

cFE/modules/es/fsw/src/cfe_es_cds.c

Lines 443 to 456 in e5d4ed9

if (RegUpdateStatus != CFE_SUCCESS)
{
CFE_ES_WriteToSysLog("%s: Failed to update CDS Registry (Stat=0x%08X)\n", __func__,
(unsigned int)RegUpdateStatus);
/*
* Return failure only if this was the primary error,
* do not overwrite a preexisting error.
*/
if (Status == CFE_SUCCESS)
{
Status = RegUpdateStatus;
}
}

Describe the solution you'd like
Really collapses back down into just one status... no point for two.

Describe alternatives you've considered
None

Additional context
None

Requester Info
Jacob Hageman - NASA/GSFC
@skliper skliper mentioned this issue Sep 6, 2021
Merged
@thnkslprpt thnkslprpt mentioned this issue May 6, 2023
Merged
@thnkslprpt
Copy link
Contributor

I suspect there were code changes that made the check redundant...

@skliper

@thnkslprpt thnkslprpt mentioned this issue May 6, 2023
Merged
2 tasks
@avan989
Copy link
Contributor

avan989 commented Jun 12, 2023

code coverage is not possible.

As stated in the description, it is not possible to have RegUpdateStatus != CFE_SUCESS and Status != CFE_SUCCESS with the current implementation.

dzbaker added a commit that referenced this issue Nov 5, 2024
@dzbaker
Merge pull request #2329 from thnkslprpt:fix-1929-remove-redundant-st…
012ca67 
…atus-check-in-CFE_ES_RegisterCDSEx

Fix #1929, Remove redundant status check in CFE_ES_RegisterCDSEx()
@dzbaker dzbaker mentioned this issue Nov 5, 2024
Merged
2 tasks
@dzbaker dzbaker closed this as completed in 5236e56 Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
coverage enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix #1929, Remove redundant status check in CFE_ES_RegisterCDSEx() thnkslprpt/cFE
3 participants
@thnkslprpt @avan989 @skliper