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

cFS-Caelum Review, CFS-41: EVS and FS #1294

Closed
wants to merge 3 commits into from

Conversation

astrogeco
Copy link
Contributor

@astrogeco astrogeco commented Apr 11, 2021

Description

This is a "bookkeeping" Pull Request meant for the cFS-Caelum Review code inspection (full-scale code review).

This PR is solely focused on "CFS-41". For more info see this readme

The Included files are

https://github.com/astrogeco/cFE/blob/caelum-code-review-cfs41/.gitignore

# EVS

# EVS
!modules/evs/fsw/**/**
!modules/core_api/fsw/inc/cfe_evs*
!modules/core_private/fsw/inc/cfe_evs_log_typedef.h
!modules/evs/CMakeLists.txt

# FS
!cfe_fs*
!modules/fs/fsw/*
!modules/fs/CMakeLists.txt

Objectives

  1. This review starts on 04/12/2021 and ends on 04/16/2021.

  2. Dispositions of findings is on 04/19/2021.

  3. See also "The Power of 10" rules for safety-critical code. https://en.wikipedia.org/wiki/The_Power_of_10:_Rules_for_Developing_Safety-Critical_Code#:~:text=The%20Power%20of%2010%20Rules,to%20review%20or%20statically%20analyze

  4. NOTE: Don't spend too much time over coding standard violations. The Static Code Analysis tool will enforce the coding standards. This code is developed by GSFC, so GSFC coding standards will be enforced for this code base.

Notes

If there's anything else that is observed as a repeated pattern, feel free to document as a general commen

There are several places that would trigger warnings with some common compilers/warning options. It would be nice to follow rule 10 in "The Power of 10".

Quick summary/references for currently enforced settings on the FSW
Compiler options (note -Wall and -Werror) -

add_compile_options(
-std=c99 # Target the C99 standard (without gcc extensions)
-pedantic # Issue all the warnings demanded by strict ISO C
-Wall # Warn about most questionable operations
-Wstrict-prototypes # Warn about missing prototypes
-Wwrite-strings # Warn if not treating string literals as "const"
-Wpointer-arith # Warn about suspicious pointer operations
-Wcast-align # Warn about casts that increase alignment requirements
-Werror # Treat warnings as errors (code should be clean)
)

cppcheck -

cppcheck_common_opts="--force --inline-suppr --std=c99 --language=c --enable=warning,performance,portability,style --suppress=variableScope --inconclusive"

CodeQL -

- name: Initialize CodeQL
uses: github/codeql-action/init@v1
with:
languages: c
queries: +security-extended, security-and-quality

CodeSonar - currently using default set for cFE, extending to JPL and MISRA is future work

For CodeQL and CodeSonar we don't eliminate all warnings, but we do analyze and disposition them all (plan to report dispositions as part of certification package)

This approach is compliant with the latest GSFC-582 standard (that is still going through review). Happy to discuss any additional settings that you have concerns about.

@astrogeco astrogeco force-pushed the caelum-code-review-cfs41 branch 2 times, most recently from 68a627a to 2a22870 Compare April 11, 2021 17:56
modules/evs/fsw/inc/cfe_evs_msg.h Show resolved Hide resolved
modules/evs/fsw/inc/cfe_evs_msg.h Show resolved Hide resolved
modules/evs/fsw/src/cfe_evs.c Show resolved Hide resolved
modules/evs/fsw/src/cfe_evs.c Show resolved Hide resolved
modules/evs/fsw/src/cfe_evs.c Show resolved Hide resolved
modules/evs/fsw/src/cfe_evs_task.c Show resolved Hide resolved
modules/evs/fsw/src/cfe_evs_task.c Show resolved Hide resolved
modules/evs/fsw/src/cfe_evs_task.c Show resolved Hide resolved
modules/evs/fsw/src/cfe_evs_utils.c Show resolved Hide resolved
modules/evs/fsw/src/cfe_evs_utils.c Show resolved Hide resolved
@skliper
Copy link
Contributor

skliper commented Apr 20, 2021

Review closed 4/19. For any future comments please open a new issue.

@skliper skliper closed this Apr 20, 2021
@skliper skliper changed the title cFS-Caelum Review, EVS and FS, CFS-41 cFS-Caelum Review, CFS-41: EVS and FS Apr 26, 2021
@skliper skliper added the review label Sep 24, 2021
@astrogeco astrogeco deleted the caelum-code-review-cfs41 branch January 24, 2022 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants