issue #117, Resolved cppcheck warning

[avan989]

Describe the contribution
Resolve PSP cpp check warning

Testing performed
Steps taken to test the contribution:

1. cppcheck --force --inline-suppr --std=c99 --language=c --error-exitcode=1 --enable=warning,performance,portability,style --inconclusive psp/fsw 2>psp.txt

2. Verify warning is gone

3. make prep

4. make

5. make install

6. verify cfs still runs.

System(s) tested on:

• Hardware
• Ubuntu 18.04
• CFS 6.6

Contributor Info
Anh Van, NASA Goddard

[skliper]

See the requested changes I put in from the issue commit.

[avan989]

Could single line it - what does cppcheck say if you do strncpy(Taskname, "TBD\0", 16)

You get the warning: The buffer 'TaskName' may not be null-terminated after the call to strncpy().
I think you would have to use a different function call (snprintf?) if you want it to be done in one line.

[skliper]

Actually since it's just copying a constant, I'd vote to just strcpy(Taskname, "TBD");. Does that work?

[avan989]

yes. Updated.

[skliper]

Should this be strcpy?

[jphickey]

This code is merely a placeholder. The exception processing in PC-RTEMS is not really implemented, and clearly bits and pieces were at one point copied from the vxworks psp.

My suggestion is to remove the entire block from lines 133-150, and just set CFE_PSP_ExceptionReasonString to something like UNIMPLEMENTED.

Note that the test in the if statement on line 140 is totally bogus because TaskName is a local stack array, it can never be NULL.

[skliper]

CCB may want these to be OS_DEBUG so it has zero impact on current behavior unless debug is enabled (assuming this passes cppcheck).

[avan989]

Adding #ifdef OS_DEBUG_PRINTF ... will still cause warning.

[skliper]

Great point, didn't realize OS_DEBUG was a no-op if OS_DEBUG_PRINTF is not defined. How about:

if (result != whatever_the_success_code_is)
  OS_printf("xyz_function didn't return success (%d)\n", return_code);

[jphickey]

I have several complaints about this code - we should clean it up properly rather than just squelching the cppcheck warnings about it. After all, the point of running cppcheck is to actually improve the code, not just to keep the tool quiet.

Two issues:

1. the calls to the sub-functions are the same, in the same order, just with a different (hard-coded!) parameter, which should be equal to the RestartType value to begin with. So can we just call e.g.

CFE_PSP_InitCDS(RestartType)

and so on, and get rid of the if statement and else branch completely?

2. Failure of any of these memory initialization routines will almost certainly cause CFE to fail later on. If any failure occurs, it should not just print it, but stop the process and return the value to the caller. Also, the caller should check the value and do a CFE_PSP_Panic() if it fails. Currently it ignores it.

[jphickey]

I have several complaints about this code - we should clean it up properly rather than just squelching the cppcheck warnings about it. After all, the point of running cppcheck is to actually improve the code, not just to keep the tool quiet.

Two issues:

1. the calls to the sub-functions are the same, in the same order, just with a different (hard-coded!) parameter, which should be equal to the RestartType value to begin with. So can we just call e.g.

CFE_PSP_InitCDS(RestartType)

and so on, and get rid of the if statement and else branch completely?

2. Failure of any of these memory initialization routines will almost certainly cause CFE to fail later on. If any failure occurs, it should not just print it, but stop the process and return the value to the caller. Also, the caller should check the value and do a CFE_PSP_Panic() if it fails. Currently it ignores it.

[jphickey]

This code is merely a placeholder. The exception processing in PC-RTEMS is not really implemented, and clearly bits and pieces were at one point copied from the vxworks psp.

My suggestion is to remove the entire block from lines 133-150, and just set CFE_PSP_ExceptionReasonString to something like UNIMPLEMENTED.

Note that the test in the if statement on line 140 is totally bogus because TaskName is a local stack array, it can never be NULL.

[avan989]

Updated.
cfe_psp_exception.c - removed block of code and replace with "Not Implemented"
cfe_psp_memory.c - Return if error
pc-linux/src/cfe_psp_start.c - check return and call cfe_psp_panic() if failed.

[avan989]

Updated.

[skliper]

CCB 20191218 - Reviewed and approved for IC